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(select): communicate component state to screen reader #2947

Merged
merged 12 commits into from
Mar 2, 2020

Conversation

chasestarr
Copy link
Collaborator

Description

Improves the select component so that screen readers can successfully operate it. See #2929 for an approach that would make Select a semantic combobox, but may lead to unintended changes in behavior. This Diff seems to be a good middle ground between making it operational, but not changing functionality.

Scope

  • Patch: Bug Fix

@vercel
Copy link

vercel bot commented Feb 28, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/uber-ui-platform/baseweb/bjc2e5poo
✅ Preview: https://baseweb-git-select-menu-screen-reader-v2.uber-ui-platform.now.sh

@vercel vercel bot temporarily deployed to Preview February 28, 2020 23:48 Inactive
@@ -179,6 +179,7 @@ export type PropsT = {
};

export type SelectStateT = {
activeDescendant: ?string,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add these to the TS type definitions too?

@tajo
Copy link
Member

tajo commented Mar 2, 2020

This works pretty great overall. However; it seems that backspace doesn't work anymore.

@chasestarr
Copy link
Collaborator Author

This works pretty great overall. However; it seems that backspace doesn't work anymore.

Can you please provide some reproduction steps and also validate against master? I didn't make any changes related to backspace in this PR. It may be a separate regression

@nadiia
Copy link
Contributor

nadiia commented Mar 2, 2020

This works pretty great overall. However; it seems that backspace doesn't work anymore.

Do you mean to remove the selected value? It only works for multi I think.

@tajo
Copy link
Member

tajo commented Mar 2, 2020

Ah, it's not a regression. When you select a value you are not able to delete part of it to search for other options. That's a bit annoying.

@chasestarr chasestarr merged commit 05f8b41 into master Mar 2, 2020
@sandgraham sandgraham deleted the select-menu-screen-reader-v2 branch March 4, 2020 01:02
VladimirMilenko pushed a commit to VladimirMilenko/baseui that referenced this pull request Apr 2, 2020
* fix(menu): tell screenreader menu item is disabled

* feat(menu): add activedescendant change handler

* fix(select): apply active descendant to select input

* fix(select): apply listbox id to input controls

* fix(select): applies aria label with selected values

* fix(menu): update menu tests

* fix(select): conditionally apply listbox id

* fix(select): update snapshot

* chore(types): update typescript defs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants