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(multiselect): 5880-multiselect-active-descendent #7908

Merged
merged 20 commits into from
Mar 18, 2021
Merged

fix(multiselect): 5880-multiselect-active-descendent #7908

merged 20 commits into from
Mar 18, 2021

Conversation

andreancardona
Copy link
Contributor

@andreancardona andreancardona commented Feb 26, 2021

Closes #5880

CHANGED:

  • Fixes MultiSelect active descendant a11y violation by adding a delete onKeyDown event handler to the clear input button
  • Adds visually hidden description To clear selection, press Delete

TESTING:

  • Interact with the MultiSelect component with a browser compatible screen reader (i.e. - VoiceOver test on Safari)

  • Make sure component is fully interactive with keyboard functionalities and descriptions are detailed enough for all users

@netlify
Copy link

netlify bot commented Feb 26, 2021

Deploy preview for carbon-elements ready!

Built with commit 443707a

https://deploy-preview-7908--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Feb 26, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 443707a

https://deploy-preview-7908--carbon-components-react.netlify.app

@andreancardona andreancardona marked this pull request as ready for review March 1, 2021 22:25
@jnm2377 jnm2377 changed the title [DRAFT]: fix(multiselect): 5880-multiselect-active-descendent fix(multiselect): 5880-multiselect-active-descendent Mar 1, 2021
@andreancardona andreancardona requested review from tay1orjones and removed request for jnm2377 March 1, 2021 22:29
@andreancardona andreancardona requested review from jnm2377 and tw15egan and removed request for joshblack and jnm2377 March 2, 2021 01:16
Base automatically changed from master to main March 8, 2021 16:35
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

So sorry for the delay on this one! Just a couple questions below - let me know if I'm off base.

packages/react/src/components/MultiSelect/MultiSelect.js Outdated Show resolved Hide resolved
packages/react/src/internal/keyboard/keys.js Outdated Show resolved Hide resolved
Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

Testing with NVDA in Firefox and JAWS 2020 in Chrome on Windows 10

  • If the input has selected options the ability to clear them with del is not described in either reader when the input receives focus, only the number of selected items are read along with the title/label like "multiselect title, 5, multiselect label menu"

@andreancardona andreancardona requested a review from a team as a code owner March 15, 2021 23:12
@jnm2377 jnm2377 requested a review from dakahn March 16, 2021 19:35
Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

Great work -- thanks for doing this

@kodiakhq kodiakhq bot merged commit ff5c467 into carbon-design-system:main Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Multiselect): A button must have "no interactive content descendant"
5 participants