Add "CellRenderMask" Region Tracking Structure #31420
Conversation
A CellRenderMask helps track regions of cells/spacers to render. It's API allows adding ranges of cells, where its otput be an ordered list of contiguous spacer/non-spacer ranges. The implementation keeps this region list internally, splitting or merging regions when cells are added. This output will be used by the render function of a refactored VirtualizedList. Validated via UTs.
facebook-github-bot
added
p: Microsoft
Base commit: de477a0 |
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
|
@rozele has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| return this._regions; | ||
| } | ||
|
|
||
| addCells(cells: {first: number, last: number}) { |
There was a problem hiding this comment.
I'm curious why we don't also need removeCells. I presume you may want to remove entire ranges as well as things virtualize away.
There was a problem hiding this comment.
The current VirtualizedList implementation I have in #31422 will compute a whole new mask if inputs change, instead of trying to reason about the existing one.
A cell can have multiple conditions that would cause it to be rendered, so we do not know at the absense of just one condition that the cell should not be rendered. So it becomes tricky to mutate the existing mask.
There was a problem hiding this comment.
Is there a use case of adding cells that are spacers?
There was a problem hiding this comment.
The render function in the linked PR above gives a good example of usage. When we hit spacers we still need to add views to the scroll view. So this structure allows us to iterate through regions and either add cells, or if a spacer, add the spacer view. If we omitted spacers we would need to derive where they were in VirtualizedList.
|
|
||
| // We need to replace the existing covered regions with 1-3 new regions | ||
| // depending whether we need to split spacers out of overlapping regions. | ||
| const newLeadRegion: Array<CellRegion> = []; |
There was a problem hiding this comment.
Why are these arrays? Is it just for ease of spreading? I guess in that case could we create the replacementRegions array at this point and push the new regions into it?
There was a problem hiding this comment.
Yep, that is why they are arrays. Meant to represent that there may be zero or one region.
We could create replacementRegions a bit earlier and push. Might be marginally more efficient, but also maybe less clear to reason about.
| ...newTailRegion, | ||
| ]; | ||
| const numRegionsToDelete = lastIntersectIdx - firstIntersectIdx + 1; | ||
| this._regions.splice( |
There was a problem hiding this comment.
Would it be simpler to create a new regions array?
There was a problem hiding this comment.
I don't necessarily think so. We would still need to figure out how to split existing intersecting cells when iterating through them. It would also be less efficient if we ever had large masks since we would need to rebuild the whole buffer.
| ); | ||
| } | ||
|
|
||
| equals(other: CellRenderMask): boolean { |
There was a problem hiding this comment.
Should we add test cases for this? What's the use case for checking equality for render masks?
There was a problem hiding this comment.
Can add tests for this. The use-case is comparing newly derived state to the existing, for whether to return null in setState to avoid extra renders. Right now this comparison is based on the first/last indices.
| return this._regions; | ||
| } | ||
|
|
||
| addCells(cells: {first: number, last: number}) { |
There was a problem hiding this comment.
Is there a use case of adding cells that are spacers?
Builds upon the `CellRenderMask` data structure added with facebook#31420, and VirtualizedList coverage added with facebook#31401. 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. MS/FB folks have a video discussion about VirtualizedList here: https://msit.microsoftstream.com/video/fe01a1ff-0400-94b1-d4f1-f1eb924b1809 facebook#31401 added quite a few snapshot tests, centering around the logic this change is touching. I manually validated RNTester FlatList examples (and their should be some upstream UI testing for them).
Builds upon the `CellRenderMask` data structure added with facebook#31420, and VirtualizedList coverage added with facebook#31401. 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. MS/FB folks have a video discussion about VirtualizedList here: https://msit.microsoftstream.com/video/fe01a1ff-0400-94b1-d4f1-f1eb924b1809 facebook#31401 added quite a few snapshot tests, centering around the logic this change is touching. I manually validated RNTester FlatList examples (and their should be some upstream UI testing for them).
Summary
A CellRenderMask helps track regions of cells/spacers to render. It's API allows adding ranges of cells, where its otput be an ordered list of contiguous spacer/non-spacer ranges.
The implementation keeps this region list internally, splitting or merging regions when cells are added. This output will be used by the render function of a refactored VirtualizedList.
Changelog
[Internal] [Added] - Add "CellRenderMask" Region Tracking Structure
Test Plan
Validated via UTs.