Shift VirtualizedList State Representation to CellRenderMask #31422
Conversation
Depends on: facebook#31401 facebook#31420 VirtualizedList currently keeps a [first, last] range as state, tracking the region of cells to render. The render functions uses this as an input, along with a few special cases to render more (sticky headers, initial render region.) This change moves to instead keep state which describes discontiguous render regions. This mask is continually updated as the viewport changes, batch renders expand the region, etc. Special cases are baked into the render mask, with a relatively simple tranformation from the mask to render function. This representation makes it much easier to support keyboarding scenarios, which require keeping distinct regions (e.g. for last focused) realized while out of viewport. MSFT employees can see more here: https://msit.microsoftstream.com/video/fe01a1ff-0400-94b1-d4f1-f1eb924b1809
facebook-github-bot
added
p: Microsoft
|
Keeping this as a draft, since I still have a couple of failing tests to look at, and need to do some final look through/polish. This should be the rough change though. |
| // Without getItemLayout, we limit our tail spacer to the _highestMeasuredFrameIndex to | ||
| // prevent the user for hyperscrolling into un-measured area because otherwise content will | ||
| // likely jump around as it renders in above the viewport. |
There was a problem hiding this comment.
Need to think about how to fold this into masking, since the same issue would happen if we were trying to render the end region without having rendered it before (with bonus wrong offset from incorrect spacer).
There was a problem hiding this comment.
Maybe we should just avoid all spacers in not yet measured areas.
There was a problem hiding this comment.
Everything at or after the first spacer in unmeasured area seems right. Should avoid shifting at the cost of not being able to pre-realize.
Libraries/Lists/VirtualizedList.js
Outdated
| function findLastWhere<T>( | ||
| arr: Array<T>, | ||
| pred: (element: T) => boolean, | ||
| ): T | null { | ||
| for (let i = arr.length - 1; i >= 0; i--) { | ||
| if (pred(arr[i])) { | ||
| return arr[i]; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
Will move this somewhere more shared if it stays after fixes to "no getLayoutView clamping"
| ? clamp( | ||
| section.first - 1, | ||
| section.last, | ||
| this._highestMeasuredFrameIndex, |
There was a problem hiding this comment.
The existing behavior here I think assumes that there will not be many unmeasured frames before this. This logic would effectively be broken by large gaps if we did something like end region realization.
This is I think effectively solved if we constrain realized items in the render mask as well, since we will never render and lay them out. Otherwise we might need to only set _highestMeasuredFrameIndex when viewport window intersecting frames are measured.
Libraries/Lists/VirtualizedList.js
Outdated
| @@ -1094,8 +1254,7 @@ class VirtualizedList extends React.PureComponent<Props, State> { | |||
| : this.props.style, | |||
| }; | |||
|
|
|||
| this._hasMore = | |||
| this.state.last < this.props.getItemCount(this.props.data) - 1; | |||
| this._hasMore = this.state.viewportWindow.last < itemCount - 1; | |||
There was a problem hiding this comment.
This seems a bit wonky, since it is possible we will never realize up to the end. This ends up blocking parent rendering though. Seems like an existing bug here?
| Math.min( | ||
| itemCount, | ||
| scrollIndex + initialNumToRenderOrDefault(props.initialNumToRender), | ||
| ) - 1, |
There was a problem hiding this comment.
This allows something odd like [0, -1], but that is previous behavior for how we represent a zero-item range.
Base commit: 26e30a5 |
Base commit: 26e30a5 |
VirtualizedList currently keeps a [first, last] range as state, tracking the region of cells to render. The render functions uses this as an input, along with a few special cases to render more (sticky headers, initial render region.)
This change moves to instead keep state which describes discontiguous render regions. This mask is continually updated as the viewport changes, batch renders expand the region, etc. Special cases are baked into the render mask, with a relatively simple tranformation from the mask to render function.
This representation makes it much easier to support keyboarding scenarios, which require keeping distinct regions (e.g. for last focused) realized while out of viewport.
MSFT employees can see more here: https://msit.microsoftstream.com/video/fe01a1ff-0400-94b1-d4f1-f1eb924b1809
Summary
Changelog
[Internal] [Changed] - Shift VirtualizedList State Representation to CellRenderMask
Test Plan
This change is a bit hairy, given the number of special cases and wide usage. #31401 added quite a few snapshot tests, centering around the logic this change is touching. Some quick manual sanity testing was done apart from this.