Skip to content
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

fix: use correct aria roles for menu #283

Merged
merged 3 commits into from Oct 19, 2023

Conversation

codebutler
Copy link

@codebutler codebutler commented Sep 22, 2023

ARIA: listbox role documentation says:

The listbox role is used to identify an element that creates a list from which a user may select one or more static items, similar to the HTML <select> element. Unlike <select>, a listbox can contain images. Each child of a listbox should have a role of option.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 29, 2023

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 95bc70e:

Sandbox Source
chakra-react-select demo Configuration
chakra-react-select TS demo Configuration
chakra-react-select + next.js Configuration
chakra-react-select + next.js + typescript Configuration

@csandman
Copy link
Owner

csandman commented Oct 2, 2023

Hey, sorry for the late reply but thanks for submitting a PR!

Unfortunately, I don't really intend to merge any changes for accessibility unless the react-select team updates the base package. I'm very much in favor of using more valid aria roles for the combobox widget, however react-select has it's own implementation for screen reader accessibility. Specifically, they have an aria-live div where they update the text contents based on the current context of the select menu. Adding these roles without including some other required properties like aria-owns and aria-activedescendent will most likely not help with accessibility in most cases. If anything, it might create conflicting or repeated contents read out to the user, but I believe without those other required attributes nothing will be announced to the user based on this change.

If you want to see what I mean, you should try out whatever screenreader your system provides, or you can download for it, with this package before and after this change to see if there is any difference. For MacOS you can use VoiceOver and on Windows you can use JAWS or NVDA.

There is a lot of discourse over on the react-select repo about implementing a more standardized accessibility solution, but nothing has changed for a while. HOWEVER, someone recently made a PR to the react-select repo which would solve all of these problems, as it completely overhauls the aria attributes used throughout the subcomponents that make up React Select: JedWatson/react-select#5758

That includes adding dynamic IDs to each of the options which can be used in aria-activedescendant, as well as a bunch of other changes:

  1. Added 'aria-activedescendant' for input and functionality to calculate it;
  2. Added role 'option' and 'aria-selected' for option;
  3. Added role 'listbox' for menu;
  4. Added tests for 'aria-activedescendant';
  5. Changes in aria-live region:
  • the instructions how to use select will be announced only one time when user focuses the input for the first time.
  • instructions for menu or selected value will be announced only once after focusing them.
  • removed aria-live for focused option because currently with correct aria-attributes it will be announced by screenreader
  • natively as well as the status of this option (active or disabled).
  • separated ariaContext into ariaFocused, ariaResults, ariaGuidance to avoid announcing redundant information and higlight only current change.

The PR has also gotten some attention from at least one of the maintainers of React Select, which hopefully means it will me merged soon! If it does, I will most definitely update this package to ensure everything works properly and I'm not overriding their new built in role attributes with the role="button" on the options.

@codebutler
Copy link
Author

okay sounds good thanks

@codebutler codebutler closed this Oct 4, 2023
@csandman csandman reopened this Oct 19, 2023
@csandman csandman changed the base branch from main to feat/theming October 19, 2023 00:04
@csandman
Copy link
Owner

Actually, I've gone back on my prior decision haha. The listbox + option roles are valid on their own, and I've decided to preempt the merging of that PR into the core package by merging this into a feature branch I'm working on. I tested it out a bit, and this has no negative impact on the screenreader functionality as far as I can tell. Plus, I'm moving towards using some more Chakra pseudo-selector based styles on the Option component.

I decided I'd make this change anyway, so I figured I'd at least give you the credit by merging this first haha.

@csandman csandman merged commit a7e6efb into csandman:feat/theming Oct 19, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants