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: a11y, enabled clear indicator to be accessible via keyboard #5850

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

yhy-1
Copy link
Contributor

@yhy-1 yhy-1 commented Jan 19, 2024

As discussed in #4988

note: Since focus needs to be set back to the select input, it announces that the "combo box has auto-complete select blank" instead of what is in the aria-live since focus takes precedence by the screen reader.

Copy link

changeset-bot bot commented Jan 19, 2024

🦋 Changeset detected

Latest commit: ced2a2a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
react-select Patch

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

Copy link

codesandbox-ci bot commented Jan 19, 2024

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 ced2a2a:

Sandbox Source
react-codesandboxer-example Configuration

background: 'none',
':focus': {
borderColor: !isFocused ? colors.primary : 'none',
outline: 'unset',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some focus css, this can be updated or removed

@@ -764,7 +764,8 @@ export default class Select<

let newAriaSelection = ariaSelection;

let hasKeptFocus = isFocused && prevWasFocused;
let hasKeptFocus =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensure that since we are setting focus back to select input after clearing the value, initial-input-focus is not called again.

@yhy-1 yhy-1 marked this pull request as draft January 19, 2024 22:01
@yhy-1 yhy-1 marked this pull request as ready for review January 22, 2024 19:09
@yhy-1
Copy link
Contributor Author

yhy-1 commented Jan 25, 2024

@lukebennett88 @JedWatson @dcousens @nderkim @Rall3n @Methuselah96 , would you mind reviewing this PR.

@yhy-1
Copy link
Contributor Author

yhy-1 commented Feb 12, 2024

@lukebennett88 @JedWatson @dcousens @nderkim @Rall3n @Methuselah96

Hi, would appreciate it if you have time to review this PR.

@mellis481
Copy link

@lukebennett88 @JedWatson @dcousens @nderkim @Rall3n @Methuselah96 Can you please review/approve this PR? It's a critical accessibility feature that countless teams need.

@jossmac
Copy link
Collaborator

jossmac commented Apr 3, 2024

I'm not an a11y expert, but I have some thoughts:

  • accessible patterns should be the default, rather than making consumers opt-in e.g. enableAccessibleClearIndicator
  • using hard-coded english for labels will become a problem e.g. aria-label="clear"
  • my initial instinct is that ESC press, when the dialog is closed, should clear the value. i feel like the clear button would introduce an unexpected tab stop; like, a combobox doesn't have a focusable button for invoking the menu, you just press DOWN|UP

EDIT: Turns out this is already implemented via DEL press. I think a better solution would be to add that information to the initial announcement (when isClearable).

@dcousens
Copy link
Collaborator

dcousens commented Apr 3, 2024

@yhy-1 as suggested by @jossmac, could you update your pull request to announce the DEL shortcut in the aria-live attribute? (see https://github.com/JedWatson/react-select/blob/06e34882638d1526b9f5a1238bb567a3e9460ce5/packages/react-select/src/components/LiveRegion.tsx)

@dcousens
Copy link
Collaborator

dcousens commented Apr 3, 2024

Sorry @yhy-1 that I missed your previous pings/notifications 💛

@yhy-1
Copy link
Contributor Author

yhy-1 commented Apr 17, 2024

@jossmac @dcousens Hi, there isn't a consensus on the accessible patterns for this clear button, that is why it was made optional props in this PR. You can read in the discussion at #4988. Our company had flagged this as a usability issue. We want the ability to enable this to be keyboard accessible but have no means to do so. I can update the hard-code aria-label to add the ability for an aria-label. Would this PR be accepted? The aria-live already consists of so much stuff, that it would eventually get lost and won't meet the requirement for our company.

@jossmac
Copy link
Collaborator

jossmac commented Apr 18, 2024

Thank you for detailing your requirements and constraints, providing context helps a great deal. However, I'm still not comfortable introducing an unexpected tab stop.

there isn't a consensus on the accessible patterns for this clear button

After a little research, there's a keyboard interaction spec on this behaviour for the combobox pattern:

Escape: Dismisses the popup if it is visible. Optionally, if the popup is hidden before Escape is pressed, clears the combobox.

@dcousens we should support the spec behaviour*. @yhy-1 This would meet the requirement for your company, right?

*Maybe in addition to Delete press? We'd need to prevent default on the Escape keyboard event, so consumers can respond accordingly when implementing modal dialog close behaviour etc.

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

4 participants