Repository navigation
Conversation
4cedc15 to
974b177
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## fix/listbox-home-end #5263 +/- ##
=======================================================
Coverage ? 97.85%
=======================================================
Files ? 1012
Lines ? 32738
Branches ? 12130
=======================================================
Hits ? 32037
Misses ? 695
Partials ? 6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
974b177 to
bff0067
Compare
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the changes aligning type-ahead highlighting with arrow-key navigation.
No open issues 🎉
What changed in this PR
This PR aligns type-ahead with arrow-key navigation in Select, Multiselect, and OptionList, allowing disabled options to be highlighted so their state and reason can be announced.
Changes:
- Updates shared type-ahead matching to include disabled options by default.
- Preserves disabled-option skipping when a closed Select directly selects a match.
- Adds shared-hook and Select tests and removes the obsolete OptionList skipping test.
| File | Description |
|---|---|
src/select/internal.tsx |
Enables skipDisabled while the dropdown is closed. |
src/select/__tests__/type-ahead.test.tsx |
Tests closed selection and open highlighting behavior. |
src/option-list/__tests__/keyboard.test.tsx |
Removes the test asserting disabled options are skipped. |
src/internal/components/listbox/use-type-ahead.ts |
Uses highlightable-option matching and adds skipDisabled. |
src/internal/components/listbox/__tests__/use-type-ahead.test.ts |
Tests default disabled-option matching and explicit skipping. |
Review effort: Balanced
| const interactivityCheck = interactiveGroups ? isGroupInteractive : isInteractive; | ||
| const filter = (searchText: string) => | ||
| filterOptions(options, searchText, true).filter(option => interactivityCheck(option)); | ||
| const isMatchable = (option: DropdownOption) => |
There was a problem hiding this comment.
Are isMatchable and isHighlightable different conceptually?
This function is the only consumer of isHighlightable, so is this abstraction worth it, or would it be better to have it all here?, like
const isMatchable = (option: DropdownOption) =>
(interactiveGroups || option.type !== 'parent') && !(skipDisabled && option.disabled);
const filter = (searchText: string) => filterOptions(options, searchText, true).filter(isMatchable);
| value.current = newValue; | ||
|
|
||
| const nextHighlight = findMatchingOption(options, newValue, highlightedOption, interactiveGroups); | ||
| const nextHighlight = findMatchingOption(options, newValue, highlightedOption, interactiveGroups, skipDisabled); |
There was a problem hiding this comment.
That's quite a few positional parameters already, plus multiple of them are optional. Could we refactor the findMatchingOption into using named parameters?
| expect(listbox).toHaveAttribute('aria-activedescendant', wrapper.findOptionByValue('c')!.getElement().id); | ||
| }); | ||
|
|
||
| test('typing does not highlight a disabled option', () => { |
There was a problem hiding this comment.
Instead of removing this test, why not change it accordingly?
Personally, I find this test preferable to the ones added in src/internal/components/listbox/use-type-ahead.ts because it tests the component from the consumer's layer without being tied to any particular implementation.
… OptionList Up and Down reach disabled options, so their disabled state and reason are announced, but type-ahead skipped them. Type-ahead now matches the same options as Up and Down, including disabled ones. Choosing a disabled option is still ignored. When the Select dropdown is closed, type-ahead selects the match instead of highlighting it, so it keeps skipping disabled options there (skipDisabled), as a native select does.
bff0067 to
f38339d
Compare
Description
Up and Down reach disabled options, so their disabled state and reason are announced, but type-ahead skipped them. Type-ahead now matches the same options as Up and Down, including disabled ones, in Select, Multiselect and OptionList. Choosing a disabled option is still ignored. When the Select dropdown is closed, type-ahead selects the match instead of highlighting it, so it keeps skipping disabled options there (
skipDisabled), as a native select does.From the OptionList accessibility review, question 3 (type-ahead and disabled options).
Related links, issue #, if available: AfZkAA6G9O2X
How has this been tested?
Unit tests in
use-type-ahead.test.ts(reaches a disabled option; skips it withskipDisabled) and a newselect/__tests__/type-ahead.test.tsx(closed: selects the first enabled match; open: highlights the disabled match). The closed Select test fails withoutskipDisabled. The OptionList test that asserted the old skipping was removed. Unit tests for OptionList, Select, Multiselect, Autosuggest, Property filter and the shared list code pass, and eslint and tsc pass.Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.