-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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: standalone Listbox updates activedescendant correctly #31415
Open
smhigley
wants to merge
12
commits into
microsoft:master
Choose a base branch
from
smhigley:combo-activedesc
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
FluentProviderWithTheme | virtual-rerender | 36 | 35 | 10 | Possible regression |
FluentProviderWithTheme | virtual-rerender-with-unmount | 74 | 71 | 10 | Possible regression |
All results
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 632 | 626 | 5000 | |
Button | mount | 303 | 299 | 5000 | |
Field | mount | 1155 | 1137 | 5000 | |
FluentProvider | mount | 697 | 726 | 5000 | |
FluentProviderWithTheme | mount | 84 | 80 | 10 | |
FluentProviderWithTheme | virtual-rerender | 36 | 35 | 10 | Possible regression |
FluentProviderWithTheme | virtual-rerender-with-unmount | 74 | 71 | 10 | Possible regression |
MakeStyles | mount | 851 | 859 | 50000 | |
Persona | mount | 1748 | 1767 | 5000 | |
SpinButton | mount | 1413 | 1421 | 5000 | |
SwatchPicker | mount | 1572 | 1548 | 5000 |
fabricteam
reviewed
May 17, 2024
@@ -0,0 +1,7 @@ | |||
{ |
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.
🕵 fluentuiv9 Open the Visual Regressions report to inspect the affected screenshots
Avatar Converged 1 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
Avatar Converged.badgeMask.normal.chromium.png | 6 | Changed |
Card Converged 1 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
Card Converged.appearance - High Contrast.normal.chromium.png | 0 | Removed |
SwatchPicker Converged 1 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
SwatchPicker Converged.Shape.default.chromium.png | 0 | Added |
smhigley
commented
May 17, 2024
packages/react-components/react-combobox/src/components/Dropdown/useButtonTriggerSlot.ts
Show resolved
Hide resolved
smhigley
commented
May 17, 2024
packages/react-components/react-combobox/src/components/Listbox/Listbox.test.tsx
Show resolved
Hide resolved
smhigley
commented
May 24, 2024
packages/react-components/react-aria/src/activedescendant/useActiveDescendant.ts
Outdated
Show resolved
Hide resolved
ling1726
reviewed
May 27, 2024
packages/react-components/react-combobox/src/components/Listbox/useListboxStyles.styles.ts
Outdated
Show resolved
Hide resolved
ling1726
reviewed
May 27, 2024
packages/react-components/react-combobox/src/components/Listbox/useListbox.ts
Outdated
Show resolved
Hide resolved
ling1726
approved these changes
Jun 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to #31140 and #31235
Essentially this moves the initial
activeDescendantController
setting logic fromuseComboboxBaseState
touseListbox
, so it runs when the listbox is first renders instead of based on the change inopen
state.That both makes combobox/dropdown
aria-activedescendant
logic a bit more straightforward (the attribute is set if the referenced option exists), and also makes a standalone listbox work.It also moves a couple option lookup functions to the context, so that Listbox can access them (previously the
getOptionById
logic inonFocus
wasn't working when not a standalone widget, since it didn't have access to the parent combobox's option collection).Previous Behavior
The original issue was that listbox did not set the initial activedescendant. The first fix to that issue did so on focus, but that caused issues when clicking also set focus (and reset the activedescendant, potentially causing scroll & jumping).
New Behavior
There's no reason Listbox should not always have
aria-activedescendant
set while it's present on the page, so this change moves the initial activedescendant logic from the combobox (which handled it when opened) to the listbox (which handles it when initially rendered).There is a very subtle difference between the two, since the Listbox is rendered when the Combobox/Dropdown trigger gains focus, but before it is opened. This causes a minor difference in when
aria-activedescendant
is set, but does not cause any practical drawbacks to screen reader users. Some early ARIA combobox pattern work was based around the idea thataria-activedescendant
would always be set, so screen readers already handle logic on whether to read the activedescendant based onaria-expanded
.