Closed Ke1sy closed 9 months ago
Latest commit: b12107f7eec92acc447db371709a494855876c6d
The changes in this PR will be included in the next version bump.
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 b12107f7eec92acc447db371709a494855876c6d:
Sandbox | Source |
---|---|
react-codesandboxer-example | Configuration |
@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! :slightly_smiling_face:
Thanks so much @Ke1sy! I've done an initial pass of the PR, and everything looks good to me so far. However, to make the review process a little smoother, would you mind adding a few comments on the PR to explain some of the changes and additions?
@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 aria-
attributes for it's accessibility implementation.
For posterity, the ideal SR experience our team found includes the following definition for the ariaLiveMessages
prop (with the changes in this PR):
{
guidance: ({ context, isMulti }) => {
if (context === "input" && isMulti) {
return " Press left or right to manage selections";
}
if (context === "value") {
return " Press delete to remove the focused option";
}
return "";
}
}
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. https://github.com/JedWatson/react-select/assets/3422401/dedf70b3-4f73-4db0-8caa-71adfc441d9e
@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 aria-selected
) are read out as expected, but options that are below the selected option are not announced at all. Instead, the selected option value is constantly repeated on each arrow down.
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 aria-selected
that seems applicable to the issue you called out: https://a11ysupport.io/tech/aria/aria-selected_attribute.
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.
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 listbox
role, there is a pattern for using real grouped options. Specifically, if you add the role="group"
attribute to the Group
component, along with aria-labelledby={headerProps.id}
, screen readers are capable of keeping track of the fact that items are intentionally grouped, and when a new group is navigated to, the group heading is read out. This pattern could be nice for accurately reflecting the grouped state in an accessibly way, instead of only having that grouping available for visually-able people.
There is a complete example of this pattern in the W3C guide for the listbox
pattern: https://www.w3.org/WAI/ARIA/apg/patterns/listbox/examples/listbox-grouped/
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 role="listbox"
attribute being on the Menu
component, with the only direct descendant being the MenuList
component. This was easily fixed for me by moving that attribute to the MenuList
component. I can't see any reason why this wouldn't work, but correct me if I'm wrong.
This change would be very small requirement wise overall:
role="listbox"
attribute from the Menu
component to the MenuList
component (ensuring the aria-controls
attribute is still pointing at the right element).role="group"
element to the outer div wrapper of the Group
component.aria-labelledby
attribute on that same div using the id
that's passed to the GroupHeading
component.role="presentation"
attribute to the GroupHeading
wrapping div. I'm not sure if this part is actually necessary, but they have it in the example implementation, and it definitely couldn't hurt either way.https://github.com/JedWatson/react-select/assets/9214195/3163926b-6f5a-4020-9b6c-750323d068b9
The one other small thing I thought of, was adding the aria-multiselectable
attribute to whichever element ends up with the listbox
role. I'm not actually sure if this would affect anything screen reader wise, but it does appear to be a recommended attribute for the listbox widget.
@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. Thanks again for all the hard work that went into this.
@lukebennett88 th
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! @lukebennett88 you are right regarding the types, I rechecked and removed that line from tsconfig, hope we can merge this PR soon.
I'm so happy this awesome merge request got merged, I wanted to say thanks, because it made it unnecessary to use react-select-event
to us, and brought a lot of clarity in all our tests, thus greatly improving developer experience. Impressive. Thanks a lot
Hi there,
I'm getting Warning: Extra attributes from the server: aria-activedescendant
in Nextjs v14.1.0 and seract-select v5.8.0
Any help how i could fix this?
Hi there, I'm getting
Warning: Extra attributes from the server: aria-activedescendant
in Nextjs v14.1.0 and react-select v5.8.0
Same. Others too. Check out our conversation here: https://github.com/JedWatson/react-select/pull/5758#discussion_r1384745986
This PR resolves this issue: https://github.com/JedWatson/react-select/issues/5121
Video result using NVDA: https://alina-andreeva.tinytake.com/msc/ODczMTIyNF8yMjEzMjU5MA