-
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
base: master
Are you sure you want to change the base?
Conversation
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 634 | 653 | 5000 | |
Button | mount | 282 | 309 | 5000 | |
Field | mount | 1139 | 1144 | 5000 | |
FluentProvider | mount | 695 | 697 | 5000 | |
FluentProviderWithTheme | mount | 83 | 84 | 10 | |
FluentProviderWithTheme | virtual-rerender | 36 | 34 | 10 | |
FluentProviderWithTheme | virtual-rerender-with-unmount | 73 | 76 | 10 | |
MakeStyles | mount | 855 | 850 | 50000 | |
Persona | mount | 1745 | 1696 | 5000 | |
SpinButton | mount | 1359 | 1365 | 5000 | |
SwatchPicker | mount | 1489 | 1540 | 5000 |
@@ -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 2 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
Avatar Converged.badgeMask.normal.chromium.png | 5 | Changed |
Avatar Converged.badgeMask - RTL.normal.chromium.png | 0 | Removed |
@@ -104,9 +104,12 @@ export function useButtonTriggerSlot( | |||
searchString.current = ''; | |||
}, 500); | |||
|
|||
if (open) { | |||
moveToNextMatchingOptionWithSameCharacterHandling(); | |||
} |
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.
This change is because of the difference in the activedescendant being set when the listbox is rendered, before being opened. The first press of a letter should only open the listbox, and not also move to the second option starting with that letter.
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
.