-
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
GroupedList: fix virtualization (unstable preview) #24460
GroupedList: fix virtualization (unstable preview) #24460
Conversation
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 b361a0c:
|
packages/react/src/components/GroupedList/GroupedListV2.base.tsx
Outdated
Show resolved
Hide resolved
📊 Bundle size report🤖 This report was generated against 9c8ab6e329157f54899f97eec6c7bc597e632f0e |
packages/react/src/components/GroupedList/GroupedListV2.base.tsx
Outdated
Show resolved
Hide resolved
packages/react/src/components/GroupedList/GroupedListV2.base.tsx
Outdated
Show resolved
Hide resolved
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
BaseButton | mount | 779 | 1202 | 5000 | Possible regression |
Breadcrumb | mount | 2421 | 2965 | 1000 | Possible regression |
Checkbox | mount | 2242 | 2649 | 5000 | Possible regression |
CheckboxBase | mount | 1945 | 2355 | 5000 | Possible regression |
ChoiceGroup | mount | 3982 | 4351 | 5000 | Possible regression |
ComboBox | mount | 822 | 1240 | 1000 | Possible regression |
DefaultButton | mount | 959 | 1403 | 5000 | Possible regression |
DetailsRow | mount | 3224 | 3618 | 5000 | Possible regression |
DetailsRowFast | mount | 3179 | 3623 | 5000 | Possible regression |
DetailsRowNoStyles | mount | 3029 | 3489 | 5000 | Possible regression |
Dialog | mount | 2644 | 3116 | 1000 | Possible regression |
DocumentCardTitle | mount | 148 | 587 | 1000 | Possible regression |
Dropdown | mount | 2832 | 3245 | 5000 | Possible regression |
FocusTrapZone | mount | 1601 | 2010 | 5000 | Possible regression |
FocusZone | mount | 1550 | 1946 | 5000 | Possible regression |
GroupedList | mount | 67 | 59732 | 2 | Possible regression |
GroupedList | virtual-rerender | 58 | 25310 | 2 | Possible regression |
GroupedList | virtual-rerender-with-unmount | 44 | 93833 | 2 | Possible regression |
GroupedListV2 | mount | 56 | 573 | 2 | Possible regression |
GroupedListV2 | virtual-rerender | 58 | 531 | 2 | Possible regression |
GroupedListV2 | virtual-rerender-with-unmount | 58 | 569 | 2 | Possible regression |
IconButton | mount | 1492 | 1909 | 5000 | Possible regression |
Label | mount | 304 | 748 | 5000 | Possible regression |
Layer | mount | 3849 | 4261 | 5000 | Possible regression |
Link | mount | 403 | 838 | 5000 | Possible regression |
MenuButton | mount | 1226 | 1677 | 5000 | Possible regression |
MessageBar | mount | 1895 | 2399 | 5000 | Possible regression |
Nav | mount | 2875 | 3324 | 1000 | Possible regression |
OverflowSet | mount | 935 | 1385 | 5000 | Possible regression |
Panel | mount | 2158 | 2541 | 1000 | Possible regression |
Persona | mount | 853 | 1288 | 1000 | Possible regression |
Pivot | mount | 1234 | 1660 | 1000 | Possible regression |
PrimaryButton | mount | 1098 | 1519 | 5000 | Possible regression |
SearchBox | mount | 1102 | 1520 | 5000 | Possible regression |
Shimmer | mount | 2479 | 2873 | 5000 | Possible regression |
Slider | mount | 1678 | 2125 | 5000 | Possible regression |
Spinner | mount | 385 | 803 | 5000 | Possible regression |
SplitButton | mount | 2695 | 3105 | 5000 | Possible regression |
Stack | mount | 431 | 865 | 5000 | Possible regression |
StackWithIntrinsicChildren | mount | 1939 | 2386 | 5000 | Possible regression |
TagPicker | mount | 2116 | 2656 | 5000 | Possible regression |
Text | mount | 371 | 737 | 5000 | Possible regression |
TextField | mount | 1124 | 1546 | 5000 | Possible regression |
ThemeProvider | mount | 1064 | 1441 | 5000 | Possible regression |
ThemeProvider | virtual-rerender | 661 | 1080 | 5000 | Possible regression |
ThemeProvider | virtual-rerender-with-unmount | 1661 | 2019 | 5000 | Possible regression |
Toggle | mount | 695 | 1088 | 5000 | Possible regression |
buttonNative | mount | 120 | 508 | 5000 | Possible regression |
All results
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
BaseButton | mount | 779 | 1202 | 5000 | Possible regression |
Breadcrumb | mount | 2421 | 2965 | 1000 | Possible regression |
Checkbox | mount | 2242 | 2649 | 5000 | Possible regression |
CheckboxBase | mount | 1945 | 2355 | 5000 | Possible regression |
ChoiceGroup | mount | 3982 | 4351 | 5000 | Possible regression |
ComboBox | mount | 822 | 1240 | 1000 | Possible regression |
CommandBar | mount | 9275 | 9692 | 1000 | |
ContextualMenu | mount | 11490 | 12024 | 1000 | |
DefaultButton | mount | 959 | 1403 | 5000 | Possible regression |
DetailsRow | mount | 3224 | 3618 | 5000 | Possible regression |
DetailsRowFast | mount | 3179 | 3623 | 5000 | Possible regression |
DetailsRowNoStyles | mount | 3029 | 3489 | 5000 | Possible regression |
Dialog | mount | 2644 | 3116 | 1000 | Possible regression |
DocumentCardTitle | mount | 148 | 587 | 1000 | Possible regression |
Dropdown | mount | 2832 | 3245 | 5000 | Possible regression |
FocusTrapZone | mount | 1601 | 2010 | 5000 | Possible regression |
FocusZone | mount | 1550 | 1946 | 5000 | Possible regression |
GroupedList | mount | 67 | 59732 | 2 | Possible regression |
GroupedList | virtual-rerender | 58 | 25310 | 2 | Possible regression |
GroupedList | virtual-rerender-with-unmount | 44 | 93833 | 2 | Possible regression |
GroupedListV2 | mount | 56 | 573 | 2 | Possible regression |
GroupedListV2 | virtual-rerender | 58 | 531 | 2 | Possible regression |
GroupedListV2 | virtual-rerender-with-unmount | 58 | 569 | 2 | Possible regression |
IconButton | mount | 1492 | 1909 | 5000 | Possible regression |
Label | mount | 304 | 748 | 5000 | Possible regression |
Layer | mount | 3849 | 4261 | 5000 | Possible regression |
Link | mount | 403 | 838 | 5000 | Possible regression |
MenuButton | mount | 1226 | 1677 | 5000 | Possible regression |
MessageBar | mount | 1895 | 2399 | 5000 | Possible regression |
Nav | mount | 2875 | 3324 | 1000 | Possible regression |
OverflowSet | mount | 935 | 1385 | 5000 | Possible regression |
Panel | mount | 2158 | 2541 | 1000 | Possible regression |
Persona | mount | 853 | 1288 | 1000 | Possible regression |
Pivot | mount | 1234 | 1660 | 1000 | Possible regression |
PrimaryButton | mount | 1098 | 1519 | 5000 | Possible regression |
Rating | mount | 6593 | 7088 | 5000 | |
SearchBox | mount | 1102 | 1520 | 5000 | Possible regression |
Shimmer | mount | 2479 | 2873 | 5000 | Possible regression |
Slider | mount | 1678 | 2125 | 5000 | Possible regression |
SpinButton | mount | 4302 | 4719 | 5000 | |
Spinner | mount | 385 | 803 | 5000 | Possible regression |
SplitButton | mount | 2695 | 3105 | 5000 | Possible regression |
Stack | mount | 431 | 865 | 5000 | Possible regression |
StackWithIntrinsicChildren | mount | 1939 | 2386 | 5000 | Possible regression |
StackWithTextChildren | mount | 4417 | 4801 | 5000 | |
SwatchColorPicker | mount | 10205 | 10618 | 5000 | |
TagPicker | mount | 2116 | 2656 | 5000 | Possible regression |
TeachingBubble | mount | 94821 | 99915 | 5000 | |
Text | mount | 371 | 737 | 5000 | Possible regression |
TextField | mount | 1124 | 1546 | 5000 | Possible regression |
ThemeProvider | mount | 1064 | 1441 | 5000 | Possible regression |
ThemeProvider | virtual-rerender | 661 | 1080 | 5000 | Possible regression |
ThemeProvider | virtual-rerender-with-unmount | 1661 | 2019 | 5000 | Possible regression |
Toggle | mount | 695 | 1088 | 5000 | Possible regression |
buttonNative | mount | 120 | 508 | 5000 | Possible regression |
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.
The scrolling looks so good! I just added a couple comments on a11y-specific attribute calculations. Other than those two things, it looks great!
groupedListId: item.groupId!, | ||
ariaLevel: level, | ||
ariaSetSize: groups ? groups.length : undefined, | ||
ariaPosInSet: flattenedIndex !== undefined ? flattenedIndex + 1 : undefined, |
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.
The aria-posinset
value is actually the index within the current level-specific group, not the index within all rows.
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.
The reaaaaaally annoying thing is that if this is within a Grouped DetailsList, the index is actually the flattenedIndex
, and the attribute is aria-rowindex
(and there's no aria-level
or aria-setsize
) 😂😭
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.
Made a follow up issue to address: #24795
and there's no aria-level or aria-setsize
We should probably remove them from the existing GroupedList implementation as well then :D
@@ -597,8 +597,10 @@ const DetailsListInner: React.ComponentType<IDetailsListInnerProps> = ( | |||
onBlur: focusZoneProps && focusZoneProps.onBlur ? focusZoneProps.onBlur : onBlur, | |||
}; | |||
|
|||
const FinalGroupedList = groups && groupProps?.groupedListAs ? groupProps.groupedListAs : GroupedList; |
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.
If someone uses the new GroupedList within DetailsList, it looks like it won't handle using the correct semantics for being used within a grid, instead of a treegrid.
I think it'd make sense to hold off on the DetailsList change until GroupedListV2 actually supports being used within DetailsList.
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.
What changes do we need to make to GroupedListV2
to make the semantics correct?
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.
Made a follow up issue to address this: #24796
722e69b
to
469a84b
Compare
/azp run |
Pull request contains merge conflicts. |
7a7b386
to
f43be17
Compare
f43be17
to
e71865e
Compare
Asset size changes
Over Tolerance (1024 B) Over Baseline Below Baseline New Removed 1 kB = 1000 B Baseline commit: 9c8ab6e329157f54899f97eec6c7bc597e632f0e (build) |
f7cc441
to
2263f4d
Compare
This change allows users to provide a custom GroupedList renderer like GroupedListV2.
Add GroupedListV2 tests Add DetailsList tests A dd support for ungrouped lists
- Add proper `getKey()` handling. - Remove selection dependency for "show all" and footer rendering.
- Rename to GroupedListV2_unstable - Update tests to use this name
e75bba3
to
b361a0c
Compare
* master: (28 commits) fix: use trigger prop for aria-haspopup (microsoft#24794) chore(react-dialog): scaffold DialogContent component (microsoft#24844) chore: Northstar screener should read from screenerStates.json (microsoft#24848) applying package updates (web components) Standardize focus treatment (microsoft#24771) Divider - allow default prop override (microsoft#24840) GroupedList: fix virtualization (unstable preview) (microsoft#24460) fix: Add explicit children prop to TeachingBubble to support React 18 (microsoft#24823) feat: Adds `visible` prop to `TableCellActions` (microsoft#24831) [Northstar][Dropdown] Fix styling mutation when merging themes (microsoft#24787) fix: export `tableCellActionsClassNames` from unstable (microsoft#24830) bugfix(react-dialog): Adds color style to DialogSurface (microsoft#24832) applying package updates Prevent group toggling from selecting the whole group (microsoft#24822) feat(react-textarea): Add shadow variant of filled appearance (microsoft#24512) applying package updates Adding lib-commonjs top-level entries to exports map (microsoft#24792) Created shim packages (microsoft#24780) feat(react-menu): replace keydown handlers by useARIAButtonShorthand on MenuItem (microsoft#24738) fix: update version mismatches triggered by v9 release (microsoft#24812) ...
…osoft#24460) * GroupedList: add new version to address virtualization issues Introduces a new component, GroupedListV2, that is a drop-in replacement for GroupedList. GroupedListV2 addresses a bug with the virtualization implementation in GroupedList. As it is a significant re-write of the internals we've decided to make it a new component so users can opt in to the new component/behavior as needed rather than risk significant breakage with the existing GroupedList implementation. --- Virtualization in GroupedList is powered by List and groups in GroupedList are nested Lists. When nested with two or more levels of groups issues can arise with virtualization that result in the vertical size of the Lists being miscalculated resulting in items not rendering, the scrollbar repeatedly resizing (causing the list to "jump" about), or both. List does work asynchronously which contributes to the issue itself and makes debugging practically impossible as even a simple GroupedList will contain many Lists all of which are virtualized and rendering async. To address this issue we are introducing GroupedListV2 which is a drop-in replacement for GropedList (V1) as it adheres to the same API. Internally GroupedListV2 flattens virtualization into a single List eliminating the virtualization bug described above and making the list easier to reason about and debug. * List: add conditional rendering option Adds support to conditionally render cells in List which helps when rendering flattend GroupedLists as we don't really know if we need to render certain parts of the list (e.g., footers) until we call the render function. Ensure GroupedList <--> GroupedListV2 compatibility. * DetailsList: allow for custom GroupedList renderer This change allows users to provide a custom GroupedList renderer like GroupedListV2. * Update @fluentui/react API snapshot Add GroupedListV2 tests Add DetailsList tests A dd support for ungrouped lists * add perf tests for groupedlist/groupedlistv2 * change files * better types and refactor render functions. * refactor grouped items * typescript * WIP debugging * fix issues from tests - Add proper `getKey()` handling. - Remove selection dependency for "show all" and footer rendering. * Mark GroupedListV2 as unstable * groupedlistv2: update naming - Rename to GroupedListV2_unstable - Update tests to use this name * update api snapshot * update groupedlistv2 import for perf-test * update snapshots * pr feedback * update test snapshot
…osoft#24460) * GroupedList: add new version to address virtualization issues Introduces a new component, GroupedListV2, that is a drop-in replacement for GroupedList. GroupedListV2 addresses a bug with the virtualization implementation in GroupedList. As it is a significant re-write of the internals we've decided to make it a new component so users can opt in to the new component/behavior as needed rather than risk significant breakage with the existing GroupedList implementation. --- Virtualization in GroupedList is powered by List and groups in GroupedList are nested Lists. When nested with two or more levels of groups issues can arise with virtualization that result in the vertical size of the Lists being miscalculated resulting in items not rendering, the scrollbar repeatedly resizing (causing the list to "jump" about), or both. List does work asynchronously which contributes to the issue itself and makes debugging practically impossible as even a simple GroupedList will contain many Lists all of which are virtualized and rendering async. To address this issue we are introducing GroupedListV2 which is a drop-in replacement for GropedList (V1) as it adheres to the same API. Internally GroupedListV2 flattens virtualization into a single List eliminating the virtualization bug described above and making the list easier to reason about and debug. * List: add conditional rendering option Adds support to conditionally render cells in List which helps when rendering flattend GroupedLists as we don't really know if we need to render certain parts of the list (e.g., footers) until we call the render function. Ensure GroupedList <--> GroupedListV2 compatibility. * DetailsList: allow for custom GroupedList renderer This change allows users to provide a custom GroupedList renderer like GroupedListV2. * Update @fluentui/react API snapshot Add GroupedListV2 tests Add DetailsList tests A dd support for ungrouped lists * add perf tests for groupedlist/groupedlistv2 * change files * better types and refactor render functions. * refactor grouped items * typescript * WIP debugging * fix issues from tests - Add proper `getKey()` handling. - Remove selection dependency for "show all" and footer rendering. * Mark GroupedListV2 as unstable * groupedlistv2: update naming - Rename to GroupedListV2_unstable - Update tests to use this name * update api snapshot * update groupedlistv2 import for perf-test * update snapshots * pr feedback * update test snapshot
* cherry-pick: GroupedList: fix virtualization (unstable preview) (#24460) * GroupedList: add new version to address virtualization issues Introduces a new component, GroupedListV2, that is a drop-in replacement for GroupedList. GroupedListV2 addresses a bug with the virtualization implementation in GroupedList. As it is a significant re-write of the internals we've decided to make it a new component so users can opt in to the new component/behavior as needed rather than risk significant breakage with the existing GroupedList implementation. --- Virtualization in GroupedList is powered by List and groups in GroupedList are nested Lists. When nested with two or more levels of groups issues can arise with virtualization that result in the vertical size of the Lists being miscalculated resulting in items not rendering, the scrollbar repeatedly resizing (causing the list to "jump" about), or both. List does work asynchronously which contributes to the issue itself and makes debugging practically impossible as even a simple GroupedList will contain many Lists all of which are virtualized and rendering async. To address this issue we are introducing GroupedListV2 which is a drop-in replacement for GropedList (V1) as it adheres to the same API. Internally GroupedListV2 flattens virtualization into a single List eliminating the virtualization bug described above and making the list easier to reason about and debug. * List: add conditional rendering option Adds support to conditionally render cells in List which helps when rendering flattend GroupedLists as we don't really know if we need to render certain parts of the list (e.g., footers) until we call the render function. Ensure GroupedList <--> GroupedListV2 compatibility. * DetailsList: allow for custom GroupedList renderer This change allows users to provide a custom GroupedList renderer like GroupedListV2. * Update @fluentui/react API snapshot Add GroupedListV2 tests Add DetailsList tests A dd support for ungrouped lists * add perf tests for groupedlist/groupedlistv2 * change files * better types and refactor render functions. * refactor grouped items * typescript * WIP debugging * fix issues from tests - Add proper `getKey()` handling. - Remove selection dependency for "show all" and footer rendering. * Mark GroupedListV2 as unstable * groupedlistv2: update naming - Rename to GroupedListV2_unstable - Update tests to use this name * update api snapshot * update groupedlistv2 import for perf-test * update snapshots * pr feedback * update test snapshot * fix issues in DetailsList * fix type and build errors * fix types * fix build errors * Change files * update import package name for perf tests * Fix: row indices for Grouped DetailsList and GroupedList to improve a11y experience (#17591) * GroupedDetailsList will now have correct row-index for GroupHeader and correct offset is passed to DetailsRow which accounts for GroupHeader * DetailsRow now takes groups as a prop which is used to determine aria-posinset for GroupedList * updated GroupedList examples to pass groups prop to DetailsRow * updated react api file * updated detailsrow documentationa and minor fix * updated react snapshots * updated react examples snapshots * Change files * updated change file * minor * add aria-rowindex attribute to GroupedHeader.base * updated react snapshots * comment cleanup * fix: build errors * fix: build error * added missing dependency * fix CI error * getItemGroup now returns the row item's exact group * remove first for-of loop in getItemGroup * fix CI error * update getItemGroup to remove ! operator * update getTotalRowCount to remove ! operator * update DetailsRow props * remove aria-posinset calculation from DetailsRow * ariaPosinSet and ariaSetSize are now passed in return function of onRenderGroupCell for consumers to use * update GroupedList examples topass ariaPosInSet & ariaSetSize to DetailsRow * update api * ci fix * add useGroupedDetailsListIndexMap for O(1) lookup of helper variables to calculate correct index for GroupedDetailsList * GroupedListSection now passes group to onRenderCell, removes ariaPosInSet and ariaSetSize * DetailsRow now accepts group prop and calculates ariaPosInSet and ariaSetSize based on group * update GroupedList examples to pass group prop to DetailsRow * update API * update DetailsRow type documentation and ci fix * ci fix * fix tests * remove change file * update perf-test dependencies * fix build errors in react-examples * fix imports in doc site * fix path in react-examples * fix imports in example * update list implementation GroupedListV2 depends on some updates to List that were not present in the v7 branch. These List changes broke some DetailsList tests so those have been updated as well. * update API snapshot * change perf-test parameters for groupedlist * clean up perf-test settings * Change files * remove unneeded change files Co-authored-by: Tristan <tristan.watanabe@gmail.com>
Is there a plan to remove this from unstable(preview) category |
Current Behavior
GroupedList
s orDetailsList
s (internallyDetailsList
renders groups withGroupedList
) with two or more levels of nesting may have rendering issues when scrolling toward the end of the list. This manifests in different ways but generally appears in two forms:These issues are related to
List
, the component that handles list virtualization in Fluent UI v8 and v7. As a performance optimization list does some work asynchronously.GroupedList
represents each group with a differentList
meaning thatGroupedList
is a virtualizedList
s of nested virtualizedList
s. This makes reasoning about the state ofGroupedList
and debugging the code very difficult.New Behavior
NOTE: This PR adds
GroupedListV2
as an "unstable" component so users can test it out and provide feedback.Because
GroupedList
is a complicated control this PR adds a new control,GroupedListV2
that implements the same interface and is a drop-in replacement forGroupedList
. Additionally,DetailsList
's API has been expanded to allow users to swap out the previousGroupedList
forGroupedListV2
. This approach was taken so users may opt in to the fix as changing a fundamental design decision in the existing control is likely to be a breaking change for some users who rely on its current observable behavior.While
GroupedListV2
is API compatible withGroupedList
it's behavior may not be 100% consistent. For example, it renders different HTML.Additionally,
List
's API has been expanded so that it can conditionally render cells. This is necessary for group footers as it's not known until render time whether the footer should be rendered or not.Performance
All tests performed with a group depth of 5 and a group count of 5 with a total item count of 15625.
Flamegrill
GroupedList
GroupedListV2
DevTools Profiler in Edge
NB: these results are taken when running the dev version of React. Working on getting production React results.
GroupedList
GroupedListV2
The profile included scrolling the list until a new virtual page was rendered, then collapsing the same groups and expanding one of the collapsed groups.
Summary as a percentage as the tests did not take the exact same amount of time
GroupedList
GroupedListV2
GroupedListV2
reduced memoizationMissing Features
Related Issue(s)
Fixes #20825
Fixes #22215
Fixes #21367