New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Accessibility - screenreader announces "blank" while reading the options #5758
Accessibility - screenreader announces "blank" while reading the options #5758
Conversation
…that is not included into native aria messages
…tionId in constructor
🦋 Changeset detectedLatest commit: b12107f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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 b12107f:
|
…/react-select into accessibility-improvements
@lukebennett88 @JedWatson @dcousens @nderkim @Rall3n @Methuselah96 This PR contains the addition of vital ARIA attributes that significantly improve the UX for screen reader users. Please merge and release ASAP. Thanks! 🙂 |
Thanks so much @Ke1sy! |
}, press Down to open the menu, ${ | ||
isMulti ? ' press left to focus selected values' : '' | ||
}`; | ||
return isInitialFocus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is added to avoid hearing the instructions each time user's focus is moved back to the input. The instructions will be announced to user only on initial focusing of input.
@@ -98,25 +100,23 @@ export interface AriaLiveMessages< | |||
|
|||
export const defaultAriaLiveMessages = { | |||
guidance: (props: AriaGuidanceProps) => { | |||
const { isSearchable, isMulti, isDisabled, tabSelectsValue, context } = | |||
const { isSearchable, isMulti, tabSelectsValue, context, isInitialFocus } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isDisabled is removed because the screen reader will announce the default disabled message for the option after adding 'aria-activedescendant': "Blue unavailable 2 of 10"
const ScreenReaderText = ( | ||
<Fragment> | ||
<span id="aria-selection">{ariaSelected}</span> | ||
<span id="aria-context">{ariaContext}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I separeted ariaContext into 3 blocks to allow only sufficient information be announced depending on the change instead of announcing large block (ariaFocused, ariaResults, ariaGuidance) all the time which causes negative user experience
@@ -605,7 +652,6 @@ export default class Select< | |||
commonProps: any; // TODO | |||
initialTouchX = 0; | |||
initialTouchY = 0; | |||
instancePrefix = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved instancePrefix to state to be able to use it inside of getDerivedStateFromProps
@@ -441,6 +449,31 @@ function buildFocusableOptionsFromCategorizedOptions< | |||
); | |||
} | |||
|
|||
function buildFocusableOptionsWithIds<Option, Group extends GroupBase<Option>>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function calculates focusable options with id as id is needed in 'aria-activedescendant'.
this.state.focusedOption = focusableOptions[optionIndex]; | ||
this.state.focusedOptionId = getFocusedOptionId( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need update focusedOptionId
each time when focusedOption
is updated. This works if menuIsOpen on init
this.setState({ | ||
focusedOption, | ||
focusedOptionId: | ||
focusedOptionIndex > -1 ? this.getFocusedOptionId(focusedOption) : null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating of focusedOptionId (aria-activedescendant) during hover
newMenuOptionsState = { | ||
selectValue, | ||
focusedOption, | ||
focusedOptionId, | ||
focusableOptionsWithIds, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recalculation of focusableOptionsWithIds
and focusedOptionId
after some dependencies has changed (line 720)
@@ -921,6 +991,7 @@ export default class Select< | |||
this.setState({ | |||
focusedOption: options[nextFocus], | |||
focusedValue: null, | |||
focusedOptionId: this.getFocusedOptionId(options[nextFocus]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after keyboard navigation
| readonly OptionBooleanValue[]; | ||
} | ||
|
||
export const OPTIONS_GROUPED: readonly GroupedOption[] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this to test the 'aria-activedescendant' for grouped options as ther hve different format of id that includes the group index
@@ -844,6 +913,7 @@ export default class Select< | |||
inputIsHiddenAfterUpdate: false, | |||
focusedValue: null, | |||
focusedOption: focusableOptions[openAtIndex], | |||
focusedOptionId: this.getFocusedOptionId(focusableOptions[openAtIndex]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after search update
@lukebennett88 I added additional comments for changes, please feel free to ask if something else is not clear. Thanks! |
This PR is looking great! It would be awesome to see this package use more standard |
For posterity, the ideal SR experience our team found includes the following definition for the
You can test it live in this example (again, based off the changes in this PR). CC: @csandman |
@lukebennett88 Can we get this bad boy merged??? |
I just tested in Safari with VoiceOver, and it appears that this update is causing a regression. Prior to making a selection for the first time, every item is announced in the same manner. |
@lukebennett88 Thanks for your response. Since you posted your message, my team (@Ke1sy is a member) have been investigating the issue you reported and I want to share an update. To first summarize the issue... in Safari + VoiceOver only, when navigating through options in a listbox, the options above the selected option (which uses The changes @Ke1sy made involved removing much of the aria-live announcements and adding correct ARIA attributes/roles. The first rule of aria-live (I'm paraphrasing) is to not use aria-live unless you have to, preferring semantic markup and ARIA attributes/roles. So the direction we went with the PR is the right one. After doing some investigation, we found a number of articles calling out poor support in Safari for certain ARIA. Here is an article that calls out an issue with how Safari handles We have reviewed our approach and, as far as we can tell, we've applied the correct ARIA. Because this issue is only reproduceable in Safari + VoiceOver, coupled with the fact that there are numerous issues that others have logged online regarding Safari and how it doesn't handle ARIA properly, the only conclusion my team has been able to make is that it's an issue with Safari + VO. react-select has been avoiding this issue by using an excessive amount of aria-live which creates a very bad UX for all screen reader users, not just those using Safari + VoiceOver. An a11y expert has even documented it (https://toot.cafe/@aardrian/110709330614634546). So these changes are certainly a net positive. I understand it's hard to consider it acceptable that these changes result in a suboptimal (broken?) experience for Safari + VoiceOver users, but what do you do if/when that is because of an issue with Safari and/or VoiceOver? What would you recommend for a path forward? |
I asked for some advice on this, and someone pointed me to React Aria, which handles this by checking the user agent and falling back to using a live region for Apple devices: https://github.com/adobe/react-spectrum/blob/d334cde64e35cfeb1482dd396ab58652bef01022/packages/%40react-aria/combobox/src/useComboBox.ts#L250-L279 The React Aria team wrote a bit more about this here: https://react-spectrum.adobe.com/blog/building-a-combobox.html |
@lukebennett88 Thanks for eliciting some advice and sharing what react-aria is doing. My team will try to work on this soon. Stay tuned. |
packages/react-select/src/Select.tsx
Outdated
@@ -1624,6 +1719,8 @@ export default class Select< | |||
'aria-labelledby': this.props['aria-labelledby'], | |||
'aria-required': required, | |||
role: 'combobox', | |||
'aria-activedescendant': this.state.focusedOptionId || '', | |||
|
|||
...(menuIsOpen && { | |||
'aria-controls': this.getElementId('listbox'), | |||
'aria-owns': this.getElementId('listbox'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This aria-owns
isn't really necessary anymore and could be removed, but it doesn't really hurt.
When a combobox's popup is displayed, ensure the aria-controls attribute on the combobox element is set to the
id
of the popuplistbox
,tree
,grid
, ordialog
element. This is how the relationship between the element with thecombobox
role and the popup it controls is indicated. (Note: In older ARIA specs, this wasaria-owns
rather thanaria-controls
, so you may seearia-owns
in older combobox implementations. Thearia-owns
in the code should be updated toaria-controls
!)— https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/combobox_role
Another thought I had about this (I know this PR is still a WIP while the Safari + VoiceOver issues are sorted out), there is one other aria pattern that might make sense for this package. For the There is a complete example of this pattern in the W3C guide for the I actually gave it a shot, specifically with VoiceOver on Chrome, and found the results to work pretty well. The only problem I ran into was that Chrome always thought whatever group I was in was 1 of 1. I believe this is due to the This change would be very small requirement wise overall:
Screen.Recording.2023-10-18.at.9.35.04.PM.movThe one other small thing I thought of, was adding the |
@csandman thank you for suggestions, I will take a look what I can do |
@lukebennett88 @Ke1sy has updated this PR to fallback to an aria-live-based solution for Apple devices. Other devices will have the significantly improved screen reader UX that @Ke1sy implemented that uses correct ARIA/roles. So please review at your earliest convenience. The group enhancements that @csandman proposed are not included in this PR. They can be done in a follow-up PR. |
@mellis481 Can someone please approve and merge this? |
Just dropping in to say I've seen the PR has been updated to address the previous feedback and I will test and review when I get some free time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing job @Ke1sy, I've tested this out in VoiceOver and NVDA and it all looks good to me.
I only have one minor nitpick question regarding the types.
Thanks! |
@@ -1624,9 +1721,12 @@ export default class Select< | |||
'aria-labelledby': this.props['aria-labelledby'], | |||
'aria-required': required, | |||
role: 'combobox', | |||
'aria-activedescendant': this.isAppleDevice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When server-side rendering this hydration throws an error
Warning: Extra attributes from the server: aria-activedescendant
Setting either both to empty sting ''
or undefined
solves
'aria-activedescendant': this.isAppleDevice ? '' : this.state.focusedOptionId || ''
or
'aria-activedescendant': this.isAppleDevice ? undefined : this.state.focusedOptionId || undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any update on the issue i am also facing the error after update to 5.8.0?
I'm so happy this awesome merge request got merged, I wanted to say thanks, because it made it unnecessary to use |
This PR resolves this issue: #5121
Video result using NVDA:
https://alina-andreeva.tinytake.com/msc/ODczMTIyNF8yMjEzMjU5MA