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: upgrade downshift from 6.1.12 to 7.2.0, a11y fixes for single select and multiselect #252

Merged
merged 3 commits into from
Feb 21, 2023

Conversation

justynoh
Copy link
Contributor

@justynoh justynoh commented Feb 21, 2023

Supersedes #215

Problem

The single select component is not fully accessible according to the W3C WAI standard for comboboxes. Some bugs in the old implementation included:

  • Input aria defined outside of the input element itself, making it hard to find where the currently-selected option is said.
  • Overlapping stack and input within the combobox itself, which confuses screen readers by adding an additional -> step
  • Navigating to the combobox instantly toggling the menu, thus causing focus to jump around different components at unexpected times
  • Clear selection not announcing input clearing correctly.

This PR fixes these issues.

At the same time, I also took the chance to upgrade downshift, which means multiselect also had to be updated. Thus I took the chance to also fix some issues with the multiselect component.

  • Similar issue with input aria as single select
  • Toggle menu button not working as intended since no onClick is defined on the component
  • +x more not center-aligned

Solution

Upgrade downshift to 7.2.0. There are two breaking changes:

  • getComboboxProps has been removed from the return type of useCombobox as listed here, as a result of the wrapper element not receiving the combobox role attributes. Thus, the props given to getComboboxProps are passed to getInputProps instead.
  • the menu automatically opens when the combobox receives focus as listed here. However this was explicitly mentioned should not be the case during our a11y testing for FormSG. The workaround provided in the link above is used to get around this.

At the same time, in some places aria-hidden were added and some components moved around in order to fix the other bugs.

Additionally, moved menu toggling function to state reducer level since getToggleButtonProps are provided to the toggle buttons in both single and multi select.

@netlify
Copy link

netlify bot commented Feb 21, 2023

Deploy Preview for objective-bell-0ffbfb canceled.

Name Link
🔨 Latest commit a685fc2
🔍 Latest deploy log https://app.netlify.com/sites/objective-bell-0ffbfb/deploys/63f45adcc0efa500088370a9

@justynoh justynoh requested a review from karrui February 21, 2023 04:19
@justynoh justynoh requested a review from karrui February 21, 2023 05:48
Copy link
Collaborator

@karrui karrui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

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