-
Notifications
You must be signed in to change notification settings - Fork 54
fix: Improve aria-labels and Tab key navigation for <Select> and <Combobox>
#250
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
base: master
Are you sure you want to change the base?
Conversation
🟡 Heimdall Review Status
🟡
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
🟡
0/1
|
Denominator calculation
|
…e with Tab in Select
02a149f to
863921b
Compare
| () => ( | ||
| <Pressable disabled={disabled} onPress={() => setOpen((s) => !s)}> | ||
| <Pressable | ||
| accessible={customEndNode ? true : false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: hmm what if the end node is something that needs focus?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the purpose of this conditional value for the accessible prop! If a custom end node is passed in, we assume it may need to receive focus so accessible is set to true. The default end node is the <AnimatedCaret> which doesn't need to receive focus so it's false.
The green highlight is the accessible element in the mobile <DefaultSelectControl>. The end piece not highlighted is the default end node but pressing on it still opens the menu.
What changed? Why?
This PR addressees two accessibility related bugs:
<Select>/<Combobox>Root cause (required for bugfixes)
Bug 1
In
<DefaultSelectOption>, there was no key handler when theTabkey is pressed. Therefore, it followed the normal focus order logic and shifted focus to the next menu item.This was addressed by calling
event.preventDefault()on thekeypressevent and using custom logic. In<FocusTrap>anevent.defaultPreventedcheck is added to allow customers to override the<FocusTrap>keyboard behavior without needing to adjust thedisableFocusTrapprop.Bug 2
Select / Combobox and their corresponding default control components were only passing the
accessibilityLabelprop value toaria-label.This has been updated so
aria-labelreceives both theaccessibilityLabeland the selected value or placeholder as one concatenated string.UI changesTesting
How has it been tested?
Testing instructions
Illustrations/Icons Checklist
Required if this PR changes files under
packages/illustrations/**orpackages/icons/**Change management
type=routine
risk=low
impact=sev5
automerge=false