-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Update List to render children on first render() call #25331
Conversation
packages/react-examples/src/react/DetailsList/DetailsList.Basic.Example.tsx
Outdated
Show resolved
Hide resolved
📊 Bundle size report🤖 This report was generated against e799d65bcc8a71db789a41bda7240103f3bf5de7 |
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 f826e61:
|
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
GroupedList | mount | 53512 | 2171 | 2 | Possible regression |
GroupedList | virtual-rerender | 25388 | 1160 | 2 | Possible regression |
GroupedList | virtual-rerender-with-unmount | 94960 | 1666 | 2 | Possible regression |
All results
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
BaseButton | mount | 1161 | 1188 | 5000 | |
Breadcrumb | mount | 2932 | 2925 | 1000 | |
Checkbox | mount | 2650 | 2632 | 5000 | |
CheckboxBase | mount | 2379 | 2390 | 5000 | |
ChoiceGroup | mount | 4391 | 4377 | 5000 | |
ComboBox | mount | 1278 | 1251 | 1000 | |
CommandBar | mount | 9628 | 9629 | 1000 | |
ContextualMenu | mount | 11078 | 11211 | 1000 | |
DefaultButton | mount | 1385 | 1397 | 5000 | |
DetailsRow | mount | 3630 | 3613 | 5000 | |
DetailsRowFast | mount | 3612 | 3616 | 5000 | |
DetailsRowNoStyles | mount | 3439 | 3447 | 5000 | |
Dialog | mount | 3054 | 3113 | 1000 | |
DocumentCardTitle | mount | 570 | 581 | 1000 | |
Dropdown | mount | 3193 | 3227 | 5000 | |
FocusTrapZone | mount | 2032 | 2027 | 5000 | |
FocusZone | mount | 1972 | 1923 | 5000 | |
GroupedList | mount | 53512 | 2171 | 2 | Possible regression |
GroupedList | virtual-rerender | 25388 | 1160 | 2 | Possible regression |
GroupedList | virtual-rerender-with-unmount | 94960 | 1666 | 2 | Possible regression |
GroupedListV2 | mount | 558 | 570 | 2 | |
GroupedListV2 | virtual-rerender | 552 | 557 | 2 | |
GroupedListV2 | virtual-rerender-with-unmount | 572 | 576 | 2 | |
IconButton | mount | 1895 | 1912 | 5000 | |
Label | mount | 732 | 726 | 5000 | |
Layer | mount | 4275 | 4239 | 5000 | |
Link | mount | 831 | 829 | 5000 | |
MenuButton | mount | 1695 | 1677 | 5000 | |
MessageBar | mount | 2244 | 2309 | 5000 | |
Nav | mount | 3322 | 3307 | 1000 | |
OverflowSet | mount | 1378 | 1376 | 5000 | |
Panel | mount | 2548 | 2556 | 1000 | |
Persona | mount | 1268 | 1277 | 1000 | |
Pivot | mount | 1658 | 1654 | 1000 | |
PrimaryButton | mount | 1525 | 1529 | 5000 | |
Rating | mount | 7015 | 7048 | 5000 | |
SearchBox | mount | 1505 | 1518 | 5000 | |
Shimmer | mount | 2843 | 2888 | 5000 | |
Slider | mount | 2126 | 2097 | 5000 | |
SpinButton | mount | 4685 | 4903 | 5000 | |
Spinner | mount | 813 | 821 | 5000 | |
SplitButton | mount | 3095 | 3124 | 5000 | |
Stack | mount | 826 | 833 | 5000 | |
StackWithIntrinsicChildren | mount | 2432 | 2448 | 5000 | |
StackWithTextChildren | mount | 4872 | 4907 | 5000 | |
SwatchColorPicker | mount | 10555 | 10558 | 5000 | |
TagPicker | mount | 2633 | 2646 | 5000 | |
TeachingBubble | mount | 89478 | 90510 | 5000 | |
Text | mount | 786 | 797 | 5000 | |
TextField | mount | 1590 | 1609 | 5000 | |
ThemeProvider | mount | 1524 | 1528 | 5000 | |
ThemeProvider | virtual-rerender | 1087 | 1104 | 5000 | |
ThemeProvider | virtual-rerender-with-unmount | 2153 | 2157 | 5000 | |
Toggle | mount | 1100 | 1101 | 5000 | |
buttonNative | mount | 547 | 537 | 5000 |
What's the reason for not rendering a list earlier in all cases? |
22084c7
to
e478107
Compare
e478107
to
4c997fb
Compare
Backward compatibility. List is foundational to complex controls like DetailsList and GroupedList and I was concerned about potentially breaking them. That said, after thinking about it a bit and talking with some folks internally I've decided it's best to go ahead and make List always render early. It resolves this issue such that it will "just work" after the change is merged and makes List behave in a more typical "React-y" way (i.e., |
4c997fb
to
2346ab4
Compare
Asset size changes
Over Tolerance (1024 B) Over Baseline Below Baseline New Removed 1 kB = 1000 B Baseline commit: e799d65bcc8a71db789a41bda7240103f3bf5de7 (build) |
This ensures list children are always rendered on the first React render pass which allows thems to work reliably with FocusZone.
2346ab4
to
f8752da
Compare
Previously DOM measurements were not invoked from _getDerivedStateFromProps during SSR but the changes to make List render earlier lead to attempted DOM access during SSR. This adds another check to restore the previous behavior.
Previously the error object was used to generate a new error object which had the effect of removing the stack trace from the actual error. This made debugging SSR test failures very difficult. Now we just propagate the original error.
* master: (23 commits) fix(docsite-v9): move theme picker under component title so it can be visible on embedded pages (microsoft#25385) applying package updates feat: Implement child render function for DataGrid rows (microsoft#25476) fix(useTable): sort should adapt to enhanced row types (microsoft#25487) applying package updates feat: positioning should happen out of React lifecycle (microsoft#25456) applying package updates chore(react-link): migrate to new package structure (microsoft#25471) chore(react-input): migrate to new package structure (microsoft#25469) fix glob pattern for syncpack (microsoft#25465) chore(react-label): migrate to new package structure (microsoft#25470) chore: bump Griffel to latest (microsoft#25412) chore: add few small toolbar improvements (microsoft#25468) docs: fix small typos (microsoft#25464) chore: fix dependencies in @fluentui/react-toolbar (microsoft#25466) feat: v0 menu style migration from v9 (microsoft#25012) applying package updates Update List to render children on first render() call (microsoft#25331) Scaffold react-skeleton package (microsoft#25435) fix(public-docsite): Changing crossplatform urls to use 'cross' instead of 'crossplatform' (microsoft#25437) ...
* proof of concept for list + focuszone * update List to always render 'early' This ensures list children are always rendered on the first React render pass which allows thems to work reliably with FocusZone. * remove experimental prop * update hasMounted state * update detailslist snapshots * revert temporary changes to basic detailslist example. * change files * update api snapshot * update list changes to support ssr Previously DOM measurements were not invoked from _getDerivedStateFromProps during SSR but the changes to make List render earlier lead to attempted DOM access during SSR. This adds another check to restore the previous behavior. * ssr-tests: add better error messsages Previously the error object was used to generate a new error object which had the effect of removing the stack trace from the actual error. This made debugging SSR test failures very difficult. Now we just propagate the original error.
* proof of concept for list + focuszone * update List to always render 'early' This ensures list children are always rendered on the first React render pass which allows thems to work reliably with FocusZone. * remove experimental prop * update hasMounted state * update detailslist snapshots * revert temporary changes to basic detailslist example. * change files * update api snapshot * update list changes to support ssr Previously DOM measurements were not invoked from _getDerivedStateFromProps during SSR but the changes to make List render earlier lead to attempted DOM access during SSR. This adds another check to restore the previous behavior. * ssr-tests: add better error messsages Previously the error object was used to generate a new error object which had the effect of removing the stack trace from the actual error. This made debugging SSR test failures very difficult. Now we just propagate the original error.
…oft#25331)" This rendering change caused layout issues in TilesList and a performance regression for a partner. This reverts commit a180b67.
* Revert "Update List to render children on first render() call (#25331)" This rendering change caused layout issues in TilesList and a performance regression for a partner. This reverts commit a180b67. * change files * re-add "hasMounted" to List state This state was added in the change being reverted. Since this interface is exported it's part of the public API it must be maintained to avoid a breaking change. This state is currently not used but will be used when the feature being reverted is re-implemented.
Current Behavior
When
List
is inside aFocusZone
orFocusTrapZone
(for example, when in aDialog
) theList
will not receive focus despite having focusable elements. See #23925 for an example.This is because
List
does not render its list children on the first call torender()
, rather they are built in thecomponentDidMount()
lifecycle method which then triggers a secondrender()
that commits the list children to the DOM. WhenFocusZone
first renders it examines all its children for focusable elements and manages their tabindexes. BecauseList
's re-render does not trigger aFocusZone
re-render it remains unaware of the focusable childrenList
has now rendered. Even ifList
were to trigger a re-render focus would potentially already have been placed elsewhere (e.g., on the buttons in aDialog
).Which leads us to:
New Behavior
Update
List
to render on the first Reactrender()
call so it can be discovered byFocusZone
. Read on for a more detailed description!What
List
does today:List
does not render any elements, only its surfaces. So if you have items 1, 2, 3, they are not renderedcomponentDidMount()
will trigger a re-render that will render elements. Now items 1, 2, 3 are rendered.componentDidUpdate()
will, on "first" render (i.e., the actual second render(), this is a class property to track this) will re-measure elements for virtualization based on what was actually rendered. This is important because the virtualization algorithm relies on accurate measurements of what is visible on screen.What the change does:
getDerivedStateFromProps
do the work that was previously done incomponentDidMount()
. This function already did this but our new state allows us to trigger based on mounting. This is important becausegetDerviedStateFromProps
runs before the first call torender()
.componentDidUpdate()
tocomponentDidMount()
So really we're just "moving things up" to eliminate the first, empty render. The end result is that list items are now rendered on the first call to
render()
which, critically, allowsFocusZone
to "see" them and properly manage tabbing through ListsRelated Issue(s)
Fixes #23925
Fixes #25443