-
Notifications
You must be signed in to change notification settings - Fork 174
fix: Highlight the first select option by default if none are selected #3546
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: main
Are you sure you want to change the base?
Conversation
35fc74e
to
2826d18
Compare
@@ -73,6 +74,7 @@ export function useHighlightedOption<OptionType>({ | |||
{ | |||
setHighlightedIndexWithMouse: (index: number, moveFocus = false) => | |||
setHighlightedIndex(index, new HighlightType('mouse', moveFocus)), | |||
highlightFirstOptionWithMouse: () => moveHighlightFrom(1, -1, new HighlightType('mouse', true)), |
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.
We need this because index 0
may not be focusable. So we need to walk from the beginning to find the first one, which is what moveHighlightFrom
does.
9686381
to
a3a7b77
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3546 +/- ##
========================================
Coverage 96.55% 96.55%
========================================
Files 804 807 +3
Lines 23439 23474 +35
Branches 7753 8188 +435
========================================
+ Hits 22631 22666 +35
+ Misses 801 754 -47
- Partials 7 54 +47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2d1a4a1
to
fc5fe55
Compare
fc5fe55
to
05a88fe
Compare
Description
NVDA borks if there's no aria-activedescendant on a focused listbox. So we select the first one by default.
Had to modify a bunch of tests because of the "screen reader text" internal implementation logic that leads to
.textContent
being duplicated. Thankfully.toHaveTextContent()
seems to be immune to that, so I just switched to that instead.Related links, issue #, if available: AWSUI-60825
How has this been tested?
Fought and defeated the unit tests.
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.