Allow ariaDescription as a prop on IContextualMenuItem #15770
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@jspurlin as FYI |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 58dc09b:
|
Asset size changes
Baseline commit: f247986022979128560a07502bbe434093324c36 (build) |
added 3 commits
October 29, 2020 09:59
…c-react into sareiff/ariadescription
|
@joschect can you take a look some time this week please? We would like to fix our accessibility issue in office. Thanks! |
joschect
approved these changes
Nov 12, 2020
2 tasks
|
🎉 Handy links: |
2 tasks
SethDonohue
pushed a commit
to SethDonohue/fluentui
that referenced
this pull request
Nov 23, 2020
* Add support for ariaDescription in contextual menu items * Change files * Fix lint error * fix another lint error Co-authored-by: Savannah Bourgeois <sareiff@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request checklist
$ yarn changeDescription of changes
There are a few props that can be set on items to help give screen reader users more context. One of those is aria-DescribedBy which is a string of IDs that point to elements describing the item. The other is aria-Description which is often set as the description of the element or is placed into a hidden span and the ID of that span is added to the list of IDs in aria-DescribedBy. This second approach is better because it will merge the aria-DescribedBy IDs and concat them to be the aria-Description set on the element so you do not overwrite other aria-DescribedBy elements.
With this change, I add ariaDescribedBy and ariaDescription as options on IContextualMenuItem. If the user passes ariaDescription, we will generate a hidden span with the contents of that prop and set the ID of that span to the list of aria-DescribedBy.
Some more in-depth explanation on why I did it the way I did. Each of the contextual menu items is set up slightly different to account for the differences in content. Each of these has a keytip wrapper and if the user passes in keytips, the aria-DescribedBy is updated to point to the element housing the keytip. If we set aria-DescribedBy directly on the element, we would overwrite the possible aria-describedBy set by the keytip. We also can't rely on only passing in the merged aria-describedBy to the keytipProps since not all items have keytips.
Instead there are two approaches done here to match the current syntax. In the splitButton, we continue to set aria-describedBy to the merged aria properties and include keytipAttributes['aria-describedby']. In the menu button and the anchor, we get a build error if we try to do the same thing saying that 'aria-describedby' is not a prop on keytipAttributes and instead we need to pass the merged aria-describedBy into the contents as a native prop. Since we also pass it as part of the keytip props with {...keytipAttributes}, if the aria-describedBy exists on the keytipAttributes it will override the other instance and properly set the merged aria-describedBy to be the ones we passed in and include the keytip spans. If it does not exist on the keytipAttributes, the passed in value will remain.
I have added an example of different context menu types with ariaDescriptions and verified they are read out by the screen reader when "Hear advanced detail" is selected.
Lastly, I'm not completely sure I made my change file correctly and don't know the proper tags to add. Essentially I would like this to go into Fabric 7 and have it pushed to Fabric 8 once the release is complete. This is an accessibility issue which we need to fix in Office so we don't want to wait until the release of V8
Focus areas to test
Contextual menus, keytips on contextual menus, screen reader