fix: GroupedList sets correct focus attributes on List #30484
Conversation
smhigley
requested review from
a team,
ThomasMichon and
spmonahan
as code owners
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 14d8d72:
|
🕵 fluentuiv8 No visual regressions between this PR and main |
📊 Bundle size report
Unchanged fixtures
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| GroupedList | mount | 42349 | 26380 | 2 | Possible regression |
| GroupedList | virtual-rerender | 20496 | 12827 | 2 | Possible regression |
| GroupedList | virtual-rerender-with-unmount | 51645 | 43790 | 2 | Possible regression |
All results
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 625 | 630 | 5000 | |
| Breadcrumb | mount | 1699 | 1674 | 1000 | |
| Checkbox | mount | 1707 | 1718 | 5000 | |
| CheckboxBase | mount | 1462 | 1478 | 5000 | |
| ChoiceGroup | mount | 2948 | 2992 | 5000 | |
| ComboBox | mount | 645 | 669 | 1000 | |
| CommandBar | mount | 6329 | 6299 | 1000 | |
| ContextualMenu | mount | 12708 | 12775 | 1000 | |
| DefaultButton | mount | 764 | 762 | 5000 | |
| DetailsRow | mount | 2220 | 2181 | 5000 | |
| DetailsRowFast | mount | 2243 | 2225 | 5000 | |
| DetailsRowNoStyles | mount | 2015 | 2084 | 5000 | |
| Dialog | mount | 2686 | 2689 | 1000 | |
| DocumentCardTitle | mount | 228 | 233 | 1000 | |
| Dropdown | mount | 1998 | 1994 | 5000 | |
| FocusTrapZone | mount | 1157 | 1176 | 5000 | |
| FocusZone | mount | 1139 | 1115 | 5000 | |
| GroupedList | mount | 42349 | 26380 | 2 | Possible regression |
| GroupedList | virtual-rerender | 20496 | 12827 | 2 | Possible regression |
| GroupedList | virtual-rerender-with-unmount | 51645 | 43790 | 2 | Possible regression |
| GroupedListV2 | mount | 226 | 222 | 2 | |
| GroupedListV2 | virtual-rerender | 206 | 214 | 2 | |
| GroupedListV2 | virtual-rerender-with-unmount | 232 | 231 | 2 | |
| IconButton | mount | 1136 | 1105 | 5000 | |
| Label | mount | 332 | 324 | 5000 | |
| Layer | mount | 2773 | 2751 | 5000 | |
| Link | mount | 385 | 385 | 5000 | |
| MenuButton | mount | 949 | 953 | 5000 | |
| MessageBar | mount | 21506 | 21535 | 5000 | |
| Nav | mount | 1958 | 1987 | 1000 | |
| OverflowSet | mount | 786 | 789 | 5000 | |
| Panel | mount | 1855 | 1814 | 1000 | |
| Persona | mount | 754 | 751 | 1000 | |
| Pivot | mount | 891 | 892 | 1000 | |
| PrimaryButton | mount | 839 | 855 | 5000 | |
| Rating | mount | 4622 | 4714 | 5000 | |
| SearchBox | mount | 943 | 909 | 5000 | |
| Shimmer | mount | 1901 | 1867 | 5000 | |
| Slider | mount | 1322 | 1332 | 5000 | |
| SpinButton | mount | 2906 | 2888 | 5000 | |
| Spinner | mount | 388 | 393 | 5000 | |
| SplitButton | mount | 1857 | 1848 | 5000 | |
| Stack | mount | 419 | 407 | 5000 | |
| StackWithIntrinsicChildren | mount | 859 | 836 | 5000 | |
| StackWithTextChildren | mount | 2674 | 2681 | 5000 | |
| SwatchColorPicker | mount | 6230 | 6215 | 5000 | |
| TagPicker | mount | 1498 | 1499 | 5000 | |
| Text | mount | 371 | 370 | 5000 | |
| TextField | mount | 919 | 948 | 5000 | |
| ThemeProvider | mount | 830 | 834 | 5000 | |
| ThemeProvider | virtual-rerender | 592 | 587 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1255 | 1293 | 5000 | |
| Toggle | mount | 630 | 616 | 5000 | |
| buttonNative | mount | 187 | 176 | 5000 |
| usePageCache={usePageCache} | ||
| onShouldVirtualize={onShouldVirtualize} | ||
| version={version} | ||
| renderEarly={true} |
There was a problem hiding this comment.
I'm a bit concerned about changing this default behavior as doing so broke TilesList (which is why it's not enabled by default now). This change will also affect DetailsList as GroupedList is used internally for rendering in that control in "grouped" mode. I know we strive to be accessible by default so I like this change in principle -- how confident are you that enabling this won't break GroupedList usage?
There was a problem hiding this comment.
I suppose it's impossible to really be sure, since GroupedList seems to get some pretty custom usage.
The only other potential approach I can think of would be to manually apply tabIndex={0} to the first GroupHeader, though that's equally buggy since it wouldn't help folks using custom GroupHeaders. It also seems potentially a little less reliable than letting the FocusZone take care of applying tabIndex.
Thoughts on which is better?
There was a problem hiding this comment.
Maybe the answer here to allow people to opt out of early rendering?
renderEarly={typeof props.renderEarly === 'boolean' ? props.renderEarly : true}
(perhaps there is a less verbose way of writing that).
The early rendering is a better option as it addresses the underlying issue of the DOM just not being present but it is also much more likely to break something using GroupedList as it changes the rendering behavior. tabIndex is better in that it is highly unlikely to break rendering.
Allowing folks to opt out will at least give people an easy change should they need it. That said, there is an argument that this is a breaking change and we should rather require users to opt in.
Not a serious suggestion: GroupedListV3: now with early rendering?
... actually that last suggestion got me thinking: is this an issue with GroupedListV2? The rendering is quite different so it may not be (though it still ultimately uses List).
| shouldEnterInnerZone={shouldEnterInnerZone} | ||
| className={css(this._classNames.root, focusZoneProps.className)} | ||
| > | ||
| <FocusRects /> |
There was a problem hiding this comment.
Do we need to make the same change for GroupedListV2?
3 participants
Previous Behavior
Related to #27002
GroupedList internally used List, because of the same bug in the linked issue, the internal FocusZone was not setting focus on the first row in the List.
Instead, focus went to the first tabbable item in the content of the List (usually the checkbox), which meant arrow key interaction and screen reader behavior was not correct.
Additionally, the focus outline was not showing up since GroupedList does not initiate FocusRects itself (this only repros if you create a sandbox with only GroupedList and no other Fluent controls, since other controls will initate FocusRects if included).
New Behavior
Sets the
renderEarlyflag on List, so FocusZone can set tabIndex on the first row in List. Also addsFocusRectsto GroupedList.Related Issue(s)