From 19cb111f1262452fec74a51fe5ce2377277af49d Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Sun, 21 Nov 2021 14:48:01 -0800 Subject: [PATCH 1/6] Enable Generic Discontiguous Regions within VirtualizedList Builds upon the `CellRenderMask` data structure added with https://github.com/facebook/react-native/pull/31420, and VirtualizedList coverage added with https://github.com/facebook/react-native/pull/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/react-native#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). --- Libraries/Lists/CellRenderMask.js | 4 + Libraries/Lists/VirtualizeUtils.js | 2 - Libraries/Lists/VirtualizedList.js | 528 +++++++++++------- .../Lists/__tests__/VirtualizedList-test.js | 11 +- .../VirtualizedList-test.js.snap | 385 ++++--------- 5 files changed, 428 insertions(+), 502 deletions(-) diff --git a/Libraries/Lists/CellRenderMask.js b/Libraries/Lists/CellRenderMask.js index ddcb96d21c46ac..66c667f4e31a76 100644 --- a/Libraries/Lists/CellRenderMask.js +++ b/Libraries/Lists/CellRenderMask.js @@ -110,6 +110,10 @@ export class CellRenderMask { ); } + numCells(): number { + return this._numCells; + } + equals(other: CellRenderMask): boolean { return ( this._numCells === other._numCells && diff --git a/Libraries/Lists/VirtualizeUtils.js b/Libraries/Lists/VirtualizeUtils.js index 6e50e62b9cabc9..ff14f93d37e814 100644 --- a/Libraries/Lists/VirtualizeUtils.js +++ b/Libraries/Lists/VirtualizeUtils.js @@ -92,7 +92,6 @@ export function computeWindowedRenderLimits( prev: { first: number, last: number, - ... }, getFrameMetricsApprox: (index: number) => { length: number, @@ -109,7 +108,6 @@ export function computeWindowedRenderLimits( ): { first: number, last: number, - ... } { const itemCount = getItemCount(data); if (itemCount === 0) { diff --git a/Libraries/Lists/VirtualizedList.js b/Libraries/Lists/VirtualizedList.js index 6de43b77c4687d..9b9c12178e04fd 100644 --- a/Libraries/Lists/VirtualizedList.js +++ b/Libraries/Lists/VirtualizedList.js @@ -40,7 +40,10 @@ import { VirtualizedListContextProvider, type ChildListState, type ListDebugInfo, -} from './VirtualizedListContext.js'; +} from './VirtualizedListContext'; + +import {CellRenderMask} from './CellRenderMask'; +import clamp from '../Utilities/clamp'; type Item = any; @@ -309,8 +312,8 @@ let _usedIndexForKey = false; let _keylessItemComponentName: string = ''; type State = { - first: number, - last: number, + renderMask: CellRenderMask, + cellsAroundViewport: {first: number, last: number}, }; /** @@ -348,6 +351,19 @@ function windowSizeOrDefault(windowSize: ?number) { return windowSize ?? 21; } +function findLastWhere( + arr: $ReadOnlyArray, + predicate: (element: T) => boolean, +): T | null { + for (let i = arr.length - 1; i >= 0; i--) { + if (predicate(arr[i])) { + return arr[i]; + } + } + + return null; +} + /** * Base implementation for the more convenient [``](https://reactnative.dev/docs/flatlist) * and [``](https://reactnative.dev/docs/sectionlist) components, which are also better @@ -690,6 +706,11 @@ class VirtualizedList extends React.PureComponent { 'VirtualizedList: The windowSize prop must be present and set to a value greater than 0.', ); + invariant( + props.getItemCount, + 'VirtualizedList: The "getItemCount" prop must be provided', + ); + this._fillRateHelper = new FillRateHelper(this._getFrameMetrics); this._updateCellsToRenderBatcher = new Batchinator( this._updateCellsToRender, @@ -713,14 +734,11 @@ class VirtualizedList extends React.PureComponent { } } - let initialState = { - first: this.props.initialScrollIndex || 0, - last: - Math.min( - this.props.getItemCount(this.props.data), - (this.props.initialScrollIndex || 0) + - initialNumToRenderOrDefault(this.props.initialNumToRender), - ) - 1, + const initialRenderRegion = VirtualizedList._initialRenderRegion(props); + + let initialState: State = { + cellsAroundViewport: initialRenderRegion, + renderMask: VirtualizedList._createRenderMask(props, initialRenderRegion), }; if (this._isNestedWithSameOrientation()) { @@ -735,6 +753,166 @@ class VirtualizedList extends React.PureComponent { this.state = initialState; } + static _createRenderMask( + props: Props, + cellsAroundViewport: {first: number, last: number}, + ): CellRenderMask { + const itemCount = props.getItemCount(props.data); + + invariant( + cellsAroundViewport.first >= 0 && + cellsAroundViewport.last >= cellsAroundViewport.first - 1 && + cellsAroundViewport.last < itemCount, + `Invalid cells around viewport "[${cellsAroundViewport.first}, ${cellsAroundViewport.last}]" was passed to VirtualizedList._createRenderMask`, + ); + + const renderMask = new CellRenderMask(itemCount); + + if (itemCount > 0) { + renderMask.addCells(cellsAroundViewport); + + // The initially rendered cells are retained as part of the + // "scroll-to-top" optimization + if (props.initialScrollIndex == null || props.initialScrollIndex === 0) { + const initialRegion = VirtualizedList._initialRenderRegion(props); + renderMask.addCells(initialRegion); + } + + // The layout coordinates of sticker headers may be off-screen while the + // actual header is on-screen. Keep the most recent before the viewport + // rendered, even if its layout coordinates are not in viewport. + const stickyIndicesSet = new Set(props.stickyHeaderIndices); + VirtualizedList._ensureClosestStickyHeader( + props, + stickyIndicesSet, + renderMask, + cellsAroundViewport.first, + ); + } + + return renderMask; + } + + static _initialRenderRegion(props: Props): {first: number, last: number} { + const itemCount = props.getItemCount(props.data); + const scrollIndex = props.initialScrollIndex || 0; + + return { + first: scrollIndex, + last: + Math.min( + itemCount, + scrollIndex + initialNumToRenderOrDefault(props.initialNumToRender), + ) - 1, + }; + } + + static _ensureClosestStickyHeader( + props: Props, + stickyIndicesSet: Set, + renderMask: CellRenderMask, + cellIdx: number, + ) { + const stickyOffset = props.ListHeaderComponent ? 1 : 0; + + for (let itemIdx = cellIdx - 1; itemIdx >= 0; itemIdx--) { + if (stickyIndicesSet.has(itemIdx + stickyOffset)) { + renderMask.addCells({first: itemIdx, last: itemIdx}); + break; + } + } + } + + _adjustCellsAroundViewport( + props: Props, + cellsAroundViewport: {first: number, last: number}, + ): {first: number, last: number} { + const {data, getItemCount} = props; + const onEndReachedThreshold = onEndReachedThresholdOrDefault( + props.onEndReachedThreshold, + ); + this._updateViewableItems(data); + + const {contentLength, offset, visibleLength} = this._scrollMetrics; + + // Wait until the scroll view metrics have been set up. And until then, + // we will trust the initialNumToRender suggestion + if (visibleLength <= 0 || contentLength <= 0) { + return cellsAroundViewport; + } + + let newCellsAroundViewport: {first: number, last: number}; + if (this._isVirtualizationDisabled()) { + const distanceFromEnd = contentLength - visibleLength - offset; + const renderAhead = + distanceFromEnd < onEndReachedThreshold * visibleLength + ? maxToRenderPerBatchOrDefault(props.maxToRenderPerBatch) + : 0; + + newCellsAroundViewport = { + first: 0, + last: Math.min( + this.state.cellsAroundViewport.last + renderAhead, + getItemCount(data) - 1, + ), + }; + } else { + // If we have a non-zero initialScrollIndex and run this before we've scrolled, + // wait until we've scrolled the view to the right place. And until then, + // we will trust the initialScrollIndex suggestion. + if (this.props.initialScrollIndex && !this._scrollMetrics.offset) { + return cellsAroundViewport; + } + + newCellsAroundViewport = computeWindowedRenderLimits( + props.data, + props.getItemCount, + maxToRenderPerBatchOrDefault(props.maxToRenderPerBatch), + windowSizeOrDefault(props.windowSize), + cellsAroundViewport, + this._getFrameMetricsApprox, + this._scrollMetrics, + ); + } + + if (this._nestedChildLists.size > 0) { + // If some cell in the new state has a child list in it, we should only render + // up through that item, so that we give that list a chance to render. + // Otherwise there's churn from multiple child lists mounting and un-mounting + // their items. + + // Will this prevent rendering if the nested list doesn't realize the end? + const childIdx = this._findFirstChildWithMore( + newCellsAroundViewport.first, + newCellsAroundViewport.last, + ); + + newCellsAroundViewport.last = childIdx ?? newCellsAroundViewport.last; + } + + return newCellsAroundViewport; + } + + _findFirstChildWithMore(first: number, last: number): number | null { + for (let ii = first; ii <= last; ii++) { + const cellKeyForIndex = this._indicesToKeys.get(ii); + const childListKeys = + cellKeyForIndex && this._cellKeysToChildListKeys.get(cellKeyForIndex); + if (!childListKeys) { + continue; + } + // For each cell, need to check whether any child list in it has more elements to render + for (let childKey of childListKeys) { + const childList = this._nestedChildLists.get(childKey); + if (childList && childList.ref && childList.ref.hasMore()) { + return ii; + } + } + } + + return null; + } + componentDidMount() { if (this._isNestedWithSameOrientation()) { this.context.registerAsNestedChild({ @@ -755,8 +933,7 @@ class VirtualizedList extends React.PureComponent { this.context.unregisterAsNestedChild({ key: this._getListKey(), state: { - first: this.state.first, - last: this.state.last, + ...this.state, frames: this._frames, }, }); @@ -770,18 +947,21 @@ class VirtualizedList extends React.PureComponent { } static getDerivedStateFromProps(newProps: Props, prevState: State): State { - const {data, getItemCount} = newProps; - const maxToRenderPerBatch = maxToRenderPerBatchOrDefault( - newProps.maxToRenderPerBatch, - ); // first and last could be stale (e.g. if a new, shorter items props is passed in), so we make // sure we're rendering a reasonable range here. + const itemCount = newProps.getItemCount(newProps.data); + if (itemCount === prevState.renderMask.numCells()) { + return prevState; + } + + const constrainedCells = VirtualizedList._constrainToItemCount( + prevState.cellsAroundViewport, + newProps, + ); + return { - first: Math.max( - 0, - Math.min(prevState.first, getItemCount(data) - 1 - maxToRenderPerBatch), - ), - last: Math.max(0, Math.min(prevState.last, getItemCount(data) - 1)), + cellsAroundViewport: prevState.cellsAroundViewport, + renderMask: VirtualizedList._createRenderMask(newProps, constrainedCells), }; } @@ -837,6 +1017,23 @@ class VirtualizedList extends React.PureComponent { } } + static _constrainToItemCount( + cells: {first: number, last: number}, + props: Props, + ): {first: number, last: number} { + const itemCount = props.getItemCount(props.data); + const last = Math.min(itemCount - 1, cells.last); + + const maxToRenderPerBatch = maxToRenderPerBatchOrDefault( + props.maxToRenderPerBatch, + ); + + return { + first: clamp(0, itemCount - 1 - maxToRenderPerBatch, cells.first), + last, + }; + } + _onUpdateSeparators = (keys: Array, newProps: Object) => { keys.forEach(key => { const ref = key != null && this._cellRefs[key]; @@ -887,7 +1084,6 @@ class VirtualizedList extends React.PureComponent { const {ListEmptyComponent, ListFooterComponent, ListHeaderComponent} = this.props; const {data, horizontal} = this.props; - const isVirtualizationDisabled = this._isVirtualizationDisabled(); const inversionStyle = this.props.inverted ? horizontalOrDefault(this.props.horizontal) ? styles.horizontallyInverted @@ -896,6 +1092,8 @@ class VirtualizedList extends React.PureComponent { const cells = []; const stickyIndicesFromProps = new Set(this.props.stickyHeaderIndices); const stickyHeaderIndices = []; + + // 1. Add cell for ListHeaderComponent if (ListHeaderComponent) { if (stickyIndicesFromProps.has(0)) { stickyHeaderIndices.push(0); @@ -925,103 +1123,10 @@ class VirtualizedList extends React.PureComponent { , ); } + + // 2a. Add a cell for ListEmptyComponent if applicable const itemCount = this.props.getItemCount(data); - if (itemCount > 0) { - _usedIndexForKey = false; - _keylessItemComponentName = ''; - const spacerKey = this._getSpacerKey(!horizontal); - const lastInitialIndex = this.props.initialScrollIndex - ? -1 - : initialNumToRenderOrDefault(this.props.initialNumToRender) - 1; - const {first, last} = this.state; - this._pushCells( - cells, - stickyHeaderIndices, - stickyIndicesFromProps, - 0, - lastInitialIndex, - inversionStyle, - ); - const firstAfterInitial = Math.max(lastInitialIndex + 1, first); - if (!isVirtualizationDisabled && first > lastInitialIndex + 1) { - let insertedStickySpacer = false; - if (stickyIndicesFromProps.size > 0) { - const stickyOffset = ListHeaderComponent ? 1 : 0; - // See if there are any sticky headers in the virtualized space that we need to render. - for (let ii = firstAfterInitial - 1; ii > lastInitialIndex; ii--) { - if (stickyIndicesFromProps.has(ii + stickyOffset)) { - const initBlock = this._getFrameMetricsApprox(lastInitialIndex); - const stickyBlock = this._getFrameMetricsApprox(ii); - const leadSpace = - stickyBlock.offset - - initBlock.offset - - (this.props.initialScrollIndex ? 0 : initBlock.length); - cells.push( - , - ); - this._pushCells( - cells, - stickyHeaderIndices, - stickyIndicesFromProps, - ii, - ii, - inversionStyle, - ); - const trailSpace = - this._getFrameMetricsApprox(first).offset - - (stickyBlock.offset + stickyBlock.length); - cells.push( - , - ); - insertedStickySpacer = true; - break; - } - } - } - if (!insertedStickySpacer) { - const initBlock = this._getFrameMetricsApprox(lastInitialIndex); - const firstSpace = - this._getFrameMetricsApprox(first).offset - - (initBlock.offset + initBlock.length); - cells.push( - , - ); - } - } - this._pushCells( - cells, - stickyHeaderIndices, - stickyIndicesFromProps, - firstAfterInitial, - last, - inversionStyle, - ); - if (!this._hasWarned.keys && _usedIndexForKey) { - console.warn( - 'VirtualizedList: missing keys for items, make sure to specify a key or id property on each ' + - 'item or provide a custom keyExtractor.', - _keylessItemComponentName, - ); - this._hasWarned.keys = true; - } - if (!isVirtualizationDisabled && last < itemCount - 1) { - const lastFrame = this._getFrameMetricsApprox(last); - // 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. - const end = this.props.getItemLayout - ? itemCount - 1 - : Math.min(itemCount - 1, this._highestMeasuredFrameIndex); - const endFrame = this._getFrameMetricsApprox(end); - const tailSpacerLength = - endFrame.offset + - endFrame.length - - (lastFrame.offset + lastFrame.length); - cells.push( - , - ); - } - } else if (ListEmptyComponent) { + if (itemCount === 0 && ListEmptyComponent) { const element: React.Element = ((React.isValidElement( ListEmptyComponent, ) ? ( @@ -1044,6 +1149,70 @@ class VirtualizedList extends React.PureComponent { }), ); } + + // 2b. Add cells and spacers for each item + if (itemCount > 0) { + _usedIndexForKey = false; + _keylessItemComponentName = ''; + const spacerKey = this._getSpacerKey(!horizontal); + + const renderRegions = this.state.renderMask.enumerateRegions(); + const lastSpacer = findLastWhere(renderRegions, r => r.isSpacer); + + for (const section of renderRegions) { + if (section.isSpacer) { + // Legacy behavior is to avoid spacers when virtualization is + // disabled (including head spacers on initial render). + if (this._isVirtualizationDisabled()) { + continue; + } + + // 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. + const isLastSpacer = section === lastSpacer; + const constrainToMeasured = isLastSpacer && !this.props.getItemLayout; + const last = constrainToMeasured + ? clamp( + section.first - 1, + section.last, + this._highestMeasuredFrameIndex, + ) + : section.last; + + const firstMetrics = this._getFrameMetricsApprox(section.first); + const lastMetrics = this._getFrameMetricsApprox(last); + const spacerSize = + lastMetrics.offset + lastMetrics.length - firstMetrics.offset; + cells.push( + , + ); + } else { + this._pushCells( + cells, + stickyHeaderIndices, + stickyIndicesFromProps, + section.first, + section.last, + inversionStyle, + ); + } + } + + if (!this._hasWarned.keys && _usedIndexForKey) { + console.warn( + 'VirtualizedList: missing keys for items, make sure to specify a key or id property on each ' + + 'item or provide a custom keyExtractor.', + _keylessItemComponentName, + ); + this._hasWarned.keys = true; + } + } + + // 3. Add cell for ListFooterComponent if (ListFooterComponent) { const element = React.isValidElement(ListFooterComponent) ? ( ListFooterComponent @@ -1070,6 +1239,8 @@ class VirtualizedList extends React.PureComponent { , ); } + + // 4. Render the ScrollView const scrollProps = { ...this.props, onContentSizeChange: this._onContentSizeChange, @@ -1092,8 +1263,7 @@ class VirtualizedList extends React.PureComponent { : this.props.style, }; - this._hasMore = - this.state.last < this.props.getItemCount(this.props.data) - 1; + this._hasMore = this.state.cellsAroundViewport.last < itemCount - 1; const innerRet = ( { _computeBlankness() { this._fillRateHelper.computeBlankness( this.props, - this.state, + this.state.cellsAroundViewport, this._scrollMetrics, ); } @@ -1427,8 +1597,12 @@ class VirtualizedList extends React.PureComponent { framesInLayout.push(frame); } } - const windowTop = this._getFrameMetricsApprox(this.state.first).offset; - const frameLast = this._getFrameMetricsApprox(this.state.last); + const windowTop = this._getFrameMetricsApprox( + this.state.cellsAroundViewport.first, + ).offset; + const frameLast = this._getFrameMetricsApprox( + this.state.cellsAroundViewport.last, + ); const windowLen = frameLast.offset + frameLast.length - windowTop; const visTop = this._scrollMetrics.offset; const visLen = this._scrollMetrics.visibleLength; @@ -1503,7 +1677,7 @@ class VirtualizedList extends React.PureComponent { onEndReachedThreshold != null ? onEndReachedThreshold * visibleLength : 2; if ( onEndReached && - this.state.last === getItemCount(data) - 1 && + this.state.cellsAroundViewport.last === getItemCount(data) - 1 && distanceFromEnd < threshold && this._scrollMetrics.contentLength !== this._sentEndForContentLength ) { @@ -1631,7 +1805,7 @@ class VirtualizedList extends React.PureComponent { }; _scheduleCellsToRenderUpdate() { - const {first, last} = this.state; + const {first, last} = this.state.cellsAroundViewport; const {offset, visibleLength, velocity} = this._scrollMetrics; const itemCount = this.props.getItemCount(this.props.data); let hiPri = false; @@ -1719,93 +1893,25 @@ class VirtualizedList extends React.PureComponent { }; _updateCellsToRender = () => { - const { - data, - getItemCount, - onEndReachedThreshold: _onEndReachedThreshold, - } = this.props; - const onEndReachedThreshold = onEndReachedThresholdOrDefault( - _onEndReachedThreshold, - ); - const isVirtualizationDisabled = this._isVirtualizationDisabled(); - this._updateViewableItems(data); - if (!data) { - return; - } - this.setState(state => { - let newState; - const {contentLength, offset, visibleLength} = this._scrollMetrics; - if (!isVirtualizationDisabled) { - // If we run this with bogus data, we'll force-render window {first: 0, last: 0}, - // and wipe out the initialNumToRender rendered elements. - // So let's wait until the scroll view metrics have been set up. And until then, - // we will trust the initialNumToRender suggestion - if (visibleLength > 0 && contentLength > 0) { - // If we have a non-zero initialScrollIndex and run this before we've scrolled, - // we'll wipe out the initialNumToRender rendered elements starting at initialScrollIndex. - // So let's wait until we've scrolled the view to the right place. And until then, - // we will trust the initialScrollIndex suggestion. - if (!this.props.initialScrollIndex || this._scrollMetrics.offset) { - newState = computeWindowedRenderLimits( - this.props.data, - this.props.getItemCount, - maxToRenderPerBatchOrDefault(this.props.maxToRenderPerBatch), - windowSizeOrDefault(this.props.windowSize), - state, - this._getFrameMetricsApprox, - this._scrollMetrics, - ); - } - } - } else { - const distanceFromEnd = contentLength - visibleLength - offset; - const renderAhead = - distanceFromEnd < onEndReachedThreshold * visibleLength - ? maxToRenderPerBatchOrDefault(this.props.maxToRenderPerBatch) - : 0; - newState = { - first: 0, - last: Math.min(state.last + renderAhead, getItemCount(data) - 1), - }; - } - if (newState && this._nestedChildLists.size > 0) { - const newFirst = newState.first; - const newLast = newState.last; - // If some cell in the new state has a child list in it, we should only render - // up through that item, so that we give that list a chance to render. - // Otherwise there's churn from multiple child lists mounting and un-mounting - // their items. - for (let ii = newFirst; ii <= newLast; ii++) { - const cellKeyForIndex = this._indicesToKeys.get(ii); - const childListKeys = - cellKeyForIndex && - this._cellKeysToChildListKeys.get(cellKeyForIndex); - if (!childListKeys) { - continue; - } - let someChildHasMore = false; - // For each cell, need to check whether any child list in it has more elements to render - for (let childKey of childListKeys) { - const childList = this._nestedChildLists.get(childKey); - if (childList && childList.ref && childList.ref.hasMore()) { - someChildHasMore = true; - break; - } - } - if (someChildHasMore) { - newState.last = ii; - break; - } - } - } + this.setState((state, props) => { + const cellsAroundViewport = this._adjustCellsAroundViewport( + props, + state.cellsAroundViewport, + ); + const renderMask = VirtualizedList._createRenderMask( + props, + cellsAroundViewport, + ); + if ( - newState != null && - newState.first === state.first && - newState.last === state.last + cellsAroundViewport.first === state.cellsAroundViewport.first && + cellsAroundViewport.last === state.cellsAroundViewport.last && + renderMask.equals(state.renderMask) ) { - newState = null; + return null; } - return newState; + + return {cellsAroundViewport, renderMask}; }); }; @@ -1877,7 +1983,7 @@ class VirtualizedList extends React.PureComponent { this._getFrameMetrics, this._createViewToken, tuple.onViewableItemsChanged, - this.state, + this.state.cellsAroundViewport, ); }); } diff --git a/Libraries/Lists/__tests__/VirtualizedList-test.js b/Libraries/Lists/__tests__/VirtualizedList-test.js index 1efb5e53fb38e8..9a8b2b2d1523c3 100644 --- a/Libraries/Lists/__tests__/VirtualizedList-test.js +++ b/Libraries/Lists/__tests__/VirtualizedList-test.js @@ -965,13 +965,12 @@ it('renders no spacers up to initialScrollIndex on first render when virtualizat ); }); - // There should be no spacers present in an offset initial render with - // virtualiztion disabled. Only initialNumToRender items starting at - // initialScrollIndex. + // We should render initialNumToRender items with no spacers on initial render + // when virtualization is disabled expect(component).toMatchSnapshot(); }); -it('expands first in viewport to render up to maxToRenderPerBatch on initial render', () => { +it('renders initialNumToRender when initialScrollIndex is offset', () => { const items = generateItems(10); const ITEM_HEIGHT = 10; @@ -988,9 +987,7 @@ it('expands first in viewport to render up to maxToRenderPerBatch on initial ren ); }); - // When virtualization is disabled we may render items before initialItemIndex - // if initialItemIndex + initialNumToRender < maToRenderPerBatch. Expect cells - // 0-3 to be rendered in this example, even though initialScrollIndex is 4. + // We should render initialNumToRender items starting at initialScrollIndex. expect(component).toMatchSnapshot(); }); diff --git a/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap b/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap index ad93703101b2ad..0784444ad79f4d 100644 --- a/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap +++ b/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap @@ -850,10 +850,8 @@ exports[`VirtualizedList keeps sticky headers above viewport visualized 1`] = ` Array [ 0, 2, - 4, - 7, - 10, - 13, + 5, + 8, ] } windowSize={1} @@ -870,7 +868,7 @@ exports[`VirtualizedList keeps sticky headers above viewport visualized 1`] = ` @@ -879,53 +877,16 @@ exports[`VirtualizedList keeps sticky headers above viewport visualized 1`] = ` > - - - - - - - - - - - - - - - @@ -1993,45 +1954,10 @@ exports[`discards intitial render if initialScrollIndex != 0 1`] = ` - - - - - - - - - - - - - - - @@ -2399,33 +2325,12 @@ exports[`does not over-render when there is less than initialNumToRender cells 1 > - - - - - - - - - - - + style={ + Object { + "height": 40, + } + } + /> @@ -2602,113 +2507,6 @@ exports[`eventually renders all items when virtualization disabled 1`] = ` `; -exports[`expands first in viewport to render up to maxToRenderPerBatch on initial render 1`] = ` - - - - - - - - - - - - - - - - - - - - - - - -`; - exports[`expands render area by maxToRenderPerBatch on tick 1`] = ` + + + + + +`; + +exports[`renders initialNumToRender when initialScrollIndex is offset 1`] = ` + + + @@ -3138,6 +3008,13 @@ exports[`renders initialNumToRender cells when virtualization disabled 1`] = ` value={5} /> + `; @@ -3377,33 +3254,12 @@ exports[`renders offset cells in initial render when initialScrollIndex set 1`] > - - - - - - - - - - - + style={ + Object { + "height": 40, + } + } + /> @@ -3635,7 +3491,7 @@ exports[`renders tail spacer up to last measured with irregular layout when getI @@ -4405,45 +4261,10 @@ exports[`retains intitial render if initialScrollIndex == 0 1`] = ` - - - - - - - - - - - - - - - From 8a9c4e2a4949f87d2e9c470982c01864c09fd542 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Tue, 23 Nov 2021 01:51:17 -0800 Subject: [PATCH 2/6] Do not virtualize items adjacent to the last focused item This change also includes the contents of #32638 This change makes VirtualizedList track the last focused cell, through the capture phase of `onFocus`. It will keep the last focus cell, and its neighbors rendered. This allows for some basic keyboard interactions, like tab/up/down when on an item out of viewport. We keep the last focused rendered even if blurred for the scenario of tabbing in and and out of the VirtualizedList. Validated via UT. --- Libraries/Components/View/ViewPropTypes.js | 6 +- Libraries/Lists/VirtualizedList.js | 37 +- .../Lists/__tests__/VirtualizedList-test.js | 57 ++ .../__snapshots__/FlatList-test.js.snap | 13 + .../__snapshots__/SectionList-test.js.snap | 20 + .../VirtualizedList-test.js.snap | 714 ++++++++++++++++++ .../VirtualizedSectionList-test.js.snap | 47 ++ 7 files changed, 889 insertions(+), 5 deletions(-) diff --git a/Libraries/Components/View/ViewPropTypes.js b/Libraries/Components/View/ViewPropTypes.js index 0e6f739470b3f9..32078a2afc1c05 100644 --- a/Libraries/Components/View/ViewPropTypes.js +++ b/Libraries/Components/View/ViewPropTypes.js @@ -32,9 +32,11 @@ import type { export type ViewLayout = Layout; export type ViewLayoutEvent = LayoutEvent; -type BubblingEventProps = $ReadOnly<{| +type FocusEventProps = $ReadOnly<{| onBlur?: ?(event: BlurEvent) => mixed, + onBlurCapture?: ?(event: BlurEvent) => mixed, onFocus?: ?(event: FocusEvent) => mixed, + onFocusCapture?: ?(event: FocusEvent) => mixed, |}>; type DirectEventProps = $ReadOnly<{| @@ -377,7 +379,7 @@ type IOSViewProps = $ReadOnly<{| |}>; export type ViewProps = $ReadOnly<{| - ...BubblingEventProps, + ...FocusEventProps, ...DirectEventProps, ...GestureResponderEventProps, ...MouseEventProps, diff --git a/Libraries/Lists/VirtualizedList.js b/Libraries/Lists/VirtualizedList.js index 9b9c12178e04fd..ca5ebfbce91cba 100644 --- a/Libraries/Lists/VirtualizedList.js +++ b/Libraries/Lists/VirtualizedList.js @@ -41,6 +41,7 @@ import { type ChildListState, type ListDebugInfo, } from './VirtualizedListContext'; +import type {FocusEvent} from '../Types/CoreEventTypes'; import {CellRenderMask} from './CellRenderMask'; import clamp from '../Utilities/clamp'; @@ -314,6 +315,7 @@ let _keylessItemComponentName: string = ''; type State = { renderMask: CellRenderMask, cellsAroundViewport: {first: number, last: number}, + lastFocusedItem: ?number, }; /** @@ -739,6 +741,7 @@ class VirtualizedList extends React.PureComponent { let initialState: State = { cellsAroundViewport: initialRenderRegion, renderMask: VirtualizedList._createRenderMask(props, initialRenderRegion), + lastFocusedItem: null, }; if (this._isNestedWithSameOrientation()) { @@ -756,6 +759,7 @@ class VirtualizedList extends React.PureComponent { static _createRenderMask( props: Props, cellsAroundViewport: {first: number, last: number}, + lastFocusedItem: ?number, ): CellRenderMask { const itemCount = props.getItemCount(props.data); @@ -768,6 +772,14 @@ class VirtualizedList extends React.PureComponent { const renderMask = new CellRenderMask(itemCount); + // Keep the items around the last focused rendered, to allow for keyboard + // navigation + if (lastFocusedItem) { + const first = Math.max(0, lastFocusedItem - 1); + const last = Math.min(itemCount - 1, lastFocusedItem + 1); + renderMask.addCells({first, last}); + } + if (itemCount > 0) { renderMask.addCells(cellsAroundViewport); @@ -962,6 +974,7 @@ class VirtualizedList extends React.PureComponent { return { cellsAroundViewport: prevState.cellsAroundViewport, renderMask: VirtualizedList._createRenderMask(newProps, constrainedCells), + lastFocusedItem: prevState.lastFocusedItem, }; } @@ -1006,6 +1019,7 @@ class VirtualizedList extends React.PureComponent { prevCellKey={prevCellKey} onUpdateSeparators={this._onUpdateSeparators} onLayout={e => this._onCellLayout(e, key, ii)} + onFocusCapture={e => this._onCellFocusCapture(ii)} onUnmount={this._onCellUnmount} parentProps={this.props} ref={ref => { @@ -1473,6 +1487,15 @@ class VirtualizedList extends React.PureComponent { this._updateViewableItems(this.props.data); } + _onCellFocusCapture(itemIndex: number) { + const renderMask = VirtualizedList._createRenderMask( + this.props, + this.state.cellsAroundViewport, + itemIndex, + ); + this.setState({...this.state, renderMask, lastFocusedItem: itemIndex}); + } + _onCellUnmount = (cellKey: string) => { const curr = this._frames[cellKey]; if (curr) { @@ -1901,6 +1924,7 @@ class VirtualizedList extends React.PureComponent { const renderMask = VirtualizedList._createRenderMask( props, cellsAroundViewport, + state.lastFocusedItem, ); if ( @@ -2020,6 +2044,7 @@ type CellRendererProps = { ... }, prevCellKey: ?string, + onFocusCapture: (event: FocusEvent) => mixed, ... }; @@ -2134,6 +2159,7 @@ class CellRenderer extends React.Component< index, inversionStyle, parentProps, + onFocusCapture, } = this.props; const {renderItem, getItemLayout, ListItemComponent} = parentProps; const element = this._renderElement( @@ -2163,10 +2189,14 @@ class CellRenderer extends React.Component< ? [styles.row, inversionStyle] : inversionStyle; const result = !CellRendererComponent ? ( - /* $FlowFixMe[incompatible-type-arg] (>=0.89.0 site=react_native_fb) * + =0.89.0 site=react_native_fb) * This comment suppresses an error found when Flow v0.89 was deployed. * To see the error, delete this comment and run Flow. */ - + > {element} {itemSeparator} @@ -2174,7 +2204,8 @@ class CellRenderer extends React.Component< + onLayout={onLayout} + onFocusCapture={onFocusCapture}> {element} {itemSeparator} diff --git a/Libraries/Lists/__tests__/VirtualizedList-test.js b/Libraries/Lists/__tests__/VirtualizedList-test.js index 9a8b2b2d1523c3..0a7217694ef9fd 100644 --- a/Libraries/Lists/__tests__/VirtualizedList-test.js +++ b/Libraries/Lists/__tests__/VirtualizedList-test.js @@ -1445,6 +1445,63 @@ it('renders windowSize derived region at bottom', () => { expect(component).toMatchSnapshot(); }); +it('keeps last focused item rendered', () => { + const items = generateItems(20); + const ITEM_HEIGHT = 10; + + let component; + ReactTestRenderer.act(() => { + component = ReactTestRenderer.create( + , + ); + }); + + ReactTestRenderer.act(() => { + simulateLayout(component, { + viewport: {width: 10, height: 50}, + content: {width: 10, height: 200}, + }); + + performAllBatches(); + }); + + ReactTestRenderer.act(() => { + const cell3 = component.root.findByProps({value: 3}); + cell3.parent.props.onFocusCapture(null); + }); + + ReactTestRenderer.act(() => { + simulateScroll(component, {x: 0, y: 150}); + performAllBatches(); + }); + + // Cells 1-4 should remain rendered after scrolling to the bottom of the list + expect(component).toMatchSnapshot(); + + ReactTestRenderer.act(() => { + const cell17 = component.root.findByProps({value: 17}); + cell17.parent.props.onFocusCapture(null); + }); + + // Cells 2-4 should no longer be rendered after focus is moved to the end of + // the list + expect(component).toMatchSnapshot(); + + ReactTestRenderer.act(() => { + simulateScroll(component, {x: 0, y: 0}); + performAllBatches(); + }); + + // Cells 16-18 should remain rendered after scrolling back to the top of the + // list + expect(component).toMatchSnapshot(); +}); + function generateItems(count) { return Array(count) .fill() diff --git a/Libraries/Lists/__tests__/__snapshots__/FlatList-test.js.snap b/Libraries/Lists/__tests__/__snapshots__/FlatList-test.js.snap index c8b33bb98df2de..03e7c9533385bb 100644 --- a/Libraries/Lists/__tests__/__snapshots__/FlatList-test.js.snap +++ b/Libraries/Lists/__tests__/__snapshots__/FlatList-test.js.snap @@ -58,6 +58,7 @@ exports[`FlatList renders all the bells and whistles 1`] = `
@@ -216,6 +220,7 @@ exports[`FlatList renders simple list (multiple columns) 1`] = ` @@ -268,6 +273,7 @@ exports[`FlatList renders simple list 1`] = ` > @@ -276,6 +282,7 @@ exports[`FlatList renders simple list 1`] = ` /> @@ -284,6 +291,7 @@ exports[`FlatList renders simple list 1`] = ` /> @@ -328,6 +336,7 @@ exports[`FlatList renders simple list using ListItemComponent (multiple columns) > @@ -347,6 +356,7 @@ exports[`FlatList renders simple list using ListItemComponent (multiple columns) @@ -399,6 +409,7 @@ exports[`FlatList renders simple list using ListItemComponent 1`] = ` > @@ -407,6 +418,7 @@ exports[`FlatList renders simple list using ListItemComponent 1`] = ` /> @@ -415,6 +427,7 @@ exports[`FlatList renders simple list using ListItemComponent 1`] = ` /> diff --git a/Libraries/Lists/__tests__/__snapshots__/SectionList-test.js.snap b/Libraries/Lists/__tests__/__snapshots__/SectionList-test.js.snap index 8e63455db938e0..ea1021883c1d92 100644 --- a/Libraries/Lists/__tests__/__snapshots__/SectionList-test.js.snap +++ b/Libraries/Lists/__tests__/__snapshots__/SectionList-test.js.snap @@ -37,10 +37,12 @@ exports[`SectionList rendering empty section headers is fine 1`] = ` > @@ -49,6 +51,7 @@ exports[`SectionList rendering empty section headers is fine 1`] = ` /> @@ -57,6 +60,7 @@ exports[`SectionList rendering empty section headers is fine 1`] = ` /> @@ -94,6 +98,7 @@ exports[`SectionList renders a footer when there is no data 1`] = ` > @@ -102,6 +107,7 @@ exports[`SectionList renders a footer when there is no data 1`] = ` /> @@ -143,10 +149,12 @@ exports[`SectionList renders a footer when there is no data and no header 1`] = > @@ -242,6 +250,7 @@ exports[`SectionList renders all the bells and whistles 1`] = ` /> @@ -250,6 +259,7 @@ exports[`SectionList renders all the bells and whistles 1`] = ` /> @@ -266,6 +276,7 @@ exports[`SectionList renders all the bells and whistles 1`] = ` @@ -279,6 +290,7 @@ exports[`SectionList renders all the bells and whistles 1`] = ` @@ -287,6 +299,7 @@ exports[`SectionList renders all the bells and whistles 1`] = ` /> @@ -295,6 +308,7 @@ exports[`SectionList renders all the bells and whistles 1`] = ` /> @@ -311,6 +325,7 @@ exports[`SectionList renders all the bells and whistles 1`] = ` @@ -324,6 +339,7 @@ exports[`SectionList renders all the bells and whistles 1`] = ` @@ -332,6 +348,7 @@ exports[`SectionList renders all the bells and whistles 1`] = ` /> @@ -340,6 +357,7 @@ exports[`SectionList renders all the bells and whistles 1`] = ` /> @@ -356,6 +374,7 @@ exports[`SectionList renders all the bells and whistles 1`] = ` @@ -369,6 +388,7 @@ exports[`SectionList renders all the bells and whistles 1`] = ` diff --git a/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap b/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap index 0784444ad79f4d..6f4721ff239a16 100644 --- a/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap +++ b/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap @@ -70,6 +70,7 @@ exports[`VirtualizedList forwards correct stickyHeaderIndices when ListHeaderCom
@@ -453,6 +479,7 @@ exports[`VirtualizedList handles nested lists 1`] = ` stickyHeaderIndices={Array []} > @@ -461,6 +488,7 @@ exports[`VirtualizedList handles nested lists 1`] = ` /> @@ -471,6 +499,7 @@ exports[`VirtualizedList handles nested lists 1`] = ` @@ -501,6 +530,7 @@ exports[`VirtualizedList handles nested lists 1`] = ` > @@ -584,6 +616,7 @@ exports[`VirtualizedList handles separators correctly 1`] = ` /> @@ -600,6 +633,7 @@ exports[`VirtualizedList handles separators correctly 1`] = ` /> @@ -642,6 +676,7 @@ exports[`VirtualizedList handles separators correctly 2`] = ` > @@ -658,6 +693,7 @@ exports[`VirtualizedList handles separators correctly 2`] = ` /> @@ -674,6 +710,7 @@ exports[`VirtualizedList handles separators correctly 2`] = ` /> @@ -716,6 +753,7 @@ exports[`VirtualizedList handles separators correctly 3`] = ` > @@ -732,6 +770,7 @@ exports[`VirtualizedList handles separators correctly 3`] = ` /> @@ -749,6 +788,7 @@ exports[`VirtualizedList handles separators correctly 3`] = ` /> @@ -858,6 +898,7 @@ exports[`VirtualizedList keeps sticky headers above viewport visualized 1`] = ` > @@ -1295,6 +1349,7 @@ exports[`VirtualizedList renders simple list 1`] = ` > @@ -1303,6 +1358,7 @@ exports[`VirtualizedList renders simple list 1`] = ` /> @@ -1311,6 +1367,7 @@ exports[`VirtualizedList renders simple list 1`] = ` /> @@ -1352,6 +1409,7 @@ exports[`VirtualizedList renders simple list using ListItemComponent 1`] = ` > @@ -1360,6 +1418,7 @@ exports[`VirtualizedList renders simple list using ListItemComponent 1`] = ` /> @@ -1368,6 +1427,7 @@ exports[`VirtualizedList renders simple list using ListItemComponent 1`] = ` /> @@ -1442,6 +1502,7 @@ exports[`VirtualizedList renders sticky headers in viewport on batched render 1` > @@ -1549,6 +1615,7 @@ exports[`VirtualizedList warns if both renderItem or ListItemComponent are speci > @@ -1647,6 +1714,7 @@ exports[`adjusts render area with non-zero initialScrollIndex after scrolled 1`] > `; +exports[`keeps last focused item rendered 1`] = ` + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +`; + +exports[`keeps last focused item rendered 2`] = ` + + + + + + + + + + + + + + + + + + + + + + + + + + +`; + +exports[`keeps last focused item rendered 3`] = ` + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +`; + exports[`renders a zero-height tail spacer on initial render if getItemLayout not defined 1`] = ` @@ -2710,6 +3338,7 @@ exports[`renders a zero-height tail spacer on initial render if getItemLayout no /> @@ -2718,6 +3347,7 @@ exports[`renders a zero-height tail spacer on initial render if getItemLayout no /> @@ -2790,6 +3420,7 @@ exports[`renders full tail spacer if all cells measured 1`] = ` > @@ -2798,6 +3429,7 @@ exports[`renders full tail spacer if all cells measured 1`] = ` /> @@ -2806,6 +3438,7 @@ exports[`renders full tail spacer if all cells measured 1`] = ` /> @@ -2814,6 +3447,7 @@ exports[`renders full tail spacer if all cells measured 1`] = ` /> @@ -2822,6 +3456,7 @@ exports[`renders full tail spacer if all cells measured 1`] = ` /> @@ -2895,6 +3530,7 @@ exports[`renders initialNumToRender cells when virtualization disabled 1`] = ` > @@ -3361,6 +4017,7 @@ exports[`renders tail spacer up to last measured index if getItemLayout not defi /> @@ -3369,6 +4026,7 @@ exports[`renders tail spacer up to last measured index if getItemLayout not defi /> @@ -3377,6 +4035,7 @@ exports[`renders tail spacer up to last measured index if getItemLayout not defi /> @@ -3385,6 +4044,7 @@ exports[`renders tail spacer up to last measured index if getItemLayout not defi /> @@ -3457,6 +4117,7 @@ exports[`renders tail spacer up to last measured with irregular layout when getI > @@ -3465,6 +4126,7 @@ exports[`renders tail spacer up to last measured with irregular layout when getI /> @@ -3473,6 +4135,7 @@ exports[`renders tail spacer up to last measured with irregular layout when getI /> @@ -3481,6 +4144,7 @@ exports[`renders tail spacer up to last measured with irregular layout when getI /> @@ -3554,6 +4218,7 @@ exports[`renders windowSize derived region at bottom 1`] = ` > @@ -3889,6 +4572,7 @@ exports[`renders zero-height tail spacer on batch render if cells not yet measur /> @@ -3897,6 +4581,7 @@ exports[`renders zero-height tail spacer on batch render if cells not yet measur /> @@ -3972,6 +4657,7 @@ exports[`retains batch render region when an item is appended 1`] = ` > @@ -72,10 +74,12 @@ exports[`VirtualizedSectionList handles nested lists 1`] = ` stickyHeaderIndices={Array []} > @@ -84,6 +88,7 @@ exports[`VirtualizedSectionList handles nested lists 1`] = ` /> @@ -92,12 +97,14 @@ exports[`VirtualizedSectionList handles nested lists 1`] = ` /> @@ -134,6 +141,7 @@ exports[`VirtualizedSectionList handles nested lists 1`] = ` > @@ -232,10 +244,12 @@ exports[`VirtualizedSectionList handles separators correctly 1`] = ` > @@ -275,6 +289,7 @@ exports[`VirtualizedSectionList handles separators correctly 1`] = ` @@ -314,6 +329,7 @@ exports[`VirtualizedSectionList handles separators correctly 1`] = ` @@ -322,6 +338,7 @@ exports[`VirtualizedSectionList handles separators correctly 1`] = ` /> @@ -365,10 +382,12 @@ exports[`VirtualizedSectionList handles separators correctly 2`] = ` > @@ -408,6 +427,7 @@ exports[`VirtualizedSectionList handles separators correctly 2`] = ` @@ -447,6 +467,7 @@ exports[`VirtualizedSectionList handles separators correctly 2`] = ` @@ -455,6 +476,7 @@ exports[`VirtualizedSectionList handles separators correctly 2`] = ` /> @@ -498,10 +520,12 @@ exports[`VirtualizedSectionList handles separators correctly 3`] = ` > @@ -541,6 +565,7 @@ exports[`VirtualizedSectionList handles separators correctly 3`] = ` @@ -581,6 +606,7 @@ exports[`VirtualizedSectionList handles separators correctly 3`] = ` @@ -589,6 +615,7 @@ exports[`VirtualizedSectionList handles separators correctly 3`] = ` /> @@ -632,10 +659,12 @@ exports[`VirtualizedSectionList handles separators correctly 4`] = ` > @@ -675,6 +704,7 @@ exports[`VirtualizedSectionList handles separators correctly 4`] = ` @@ -715,6 +745,7 @@ exports[`VirtualizedSectionList handles separators correctly 4`] = ` @@ -723,6 +754,7 @@ exports[`VirtualizedSectionList handles separators correctly 4`] = ` /> @@ -813,6 +845,7 @@ exports[`VirtualizedSectionList renders all the bells and whistles 1`] = `
@@ -1074,6 +1115,7 @@ exports[`VirtualizedSectionList renders list with empty component 1`] = ` /> @@ -1117,10 +1159,12 @@ exports[`VirtualizedSectionList renders simple list 1`] = ` > @@ -1129,6 +1173,7 @@ exports[`VirtualizedSectionList renders simple list 1`] = ` /> @@ -1137,6 +1182,7 @@ exports[`VirtualizedSectionList renders simple list 1`] = ` /> @@ -1145,6 +1191,7 @@ exports[`VirtualizedSectionList renders simple list 1`] = ` /> From 32e73ee6f309fda45ce0c2babd0d0fa69d9de197 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Tue, 23 Nov 2021 02:40:57 -0800 Subject: [PATCH 3/6] Do not rerender on focus change if mask doesn't change --- Libraries/Lists/VirtualizedList.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Libraries/Lists/VirtualizedList.js b/Libraries/Lists/VirtualizedList.js index ca5ebfbce91cba..15c26e008506a9 100644 --- a/Libraries/Lists/VirtualizedList.js +++ b/Libraries/Lists/VirtualizedList.js @@ -315,7 +315,6 @@ let _keylessItemComponentName: string = ''; type State = { renderMask: CellRenderMask, cellsAroundViewport: {first: number, last: number}, - lastFocusedItem: ?number, }; /** @@ -741,7 +740,6 @@ class VirtualizedList extends React.PureComponent { let initialState: State = { cellsAroundViewport: initialRenderRegion, renderMask: VirtualizedList._createRenderMask(props, initialRenderRegion), - lastFocusedItem: null, }; if (this._isNestedWithSameOrientation()) { @@ -974,7 +972,6 @@ class VirtualizedList extends React.PureComponent { return { cellsAroundViewport: prevState.cellsAroundViewport, renderMask: VirtualizedList._createRenderMask(newProps, constrainedCells), - lastFocusedItem: prevState.lastFocusedItem, }; } @@ -1379,6 +1376,7 @@ class VirtualizedList extends React.PureComponent { _hiPriInProgress: boolean = false; // flag to prevent infinite hiPri cell limit update _highestMeasuredFrameIndex = 0; _indicesToKeys: Map = new Map(); + _lastFocusedItem: ?number = null; _nestedChildLists: Map< string, { @@ -1488,12 +1486,16 @@ class VirtualizedList extends React.PureComponent { } _onCellFocusCapture(itemIndex: number) { + this._lastFocusedItem = itemIndex; const renderMask = VirtualizedList._createRenderMask( this.props, this.state.cellsAroundViewport, - itemIndex, + this._lastFocusedItem, ); - this.setState({...this.state, renderMask, lastFocusedItem: itemIndex}); + + if (!renderMask.equals(this.state.renderMask)) { + this.setState({...this.state, renderMask}); + } } _onCellUnmount = (cellKey: string) => { @@ -1924,7 +1926,7 @@ class VirtualizedList extends React.PureComponent { const renderMask = VirtualizedList._createRenderMask( props, cellsAroundViewport, - state.lastFocusedItem, + this._lastFocusedItem, ); if ( From 2a5503ec0dba6eb42c7752a9ffe883c1375958cd Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Tue, 23 Nov 2021 02:45:55 -0800 Subject: [PATCH 4/6] Do not make assumptions on cell structure --- Libraries/Lists/__tests__/VirtualizedList-test.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Libraries/Lists/__tests__/VirtualizedList-test.js b/Libraries/Lists/__tests__/VirtualizedList-test.js index 0a7217694ef9fd..cd2f5902f15895 100644 --- a/Libraries/Lists/__tests__/VirtualizedList-test.js +++ b/Libraries/Lists/__tests__/VirtualizedList-test.js @@ -1471,8 +1471,7 @@ it('keeps last focused item rendered', () => { }); ReactTestRenderer.act(() => { - const cell3 = component.root.findByProps({value: 3}); - cell3.parent.props.onFocusCapture(null); + component.getInstance()._onCellFocusCapture(3); }); ReactTestRenderer.act(() => { @@ -1484,8 +1483,7 @@ it('keeps last focused item rendered', () => { expect(component).toMatchSnapshot(); ReactTestRenderer.act(() => { - const cell17 = component.root.findByProps({value: 17}); - cell17.parent.props.onFocusCapture(null); + component.getInstance()._onCellFocusCapture(17); }); // Cells 2-4 should no longer be rendered after focus is moved to the end of From f0de65b84eac9121b720b1b19170080584d8f262 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Tue, 7 Dec 2021 15:00:23 -0800 Subject: [PATCH 5/6] office-shared-comments-ui ee041633 --- Libraries/Lists/VirtualizedList.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Libraries/Lists/VirtualizedList.js b/Libraries/Lists/VirtualizedList.js index 15c26e008506a9..d46e441a4df58f 100644 --- a/Libraries/Lists/VirtualizedList.js +++ b/Libraries/Lists/VirtualizedList.js @@ -779,7 +779,9 @@ class VirtualizedList extends React.PureComponent { } if (itemCount > 0) { - renderMask.addCells(cellsAroundViewport); + if (cellsAroundViewport.last >= cellsAroundViewport.first) { + renderMask.addCells(cellsAroundViewport); + } // The initially rendered cells are retained as part of the // "scroll-to-top" optimization @@ -970,7 +972,7 @@ class VirtualizedList extends React.PureComponent { ); return { - cellsAroundViewport: prevState.cellsAroundViewport, + cellsAroundViewport: constrainedCells, renderMask: VirtualizedList._createRenderMask(newProps, constrainedCells), }; } From 03f180fb87c816b91dc501a4800438c1f61214d4 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Tue, 7 Dec 2021 19:14:37 -0800 Subject: [PATCH 6/6] Add more sophisticated logic for "last focused" render region --- Libraries/Lists/VirtualizedList.js | 82 ++- .../Lists/__tests__/VirtualizedList-test.js | 133 ++++- .../VirtualizedList-test.js.snap | 531 +++++++++++++----- 3 files changed, 592 insertions(+), 154 deletions(-) diff --git a/Libraries/Lists/VirtualizedList.js b/Libraries/Lists/VirtualizedList.js index d46e441a4df58f..1495ef1e9875bd 100644 --- a/Libraries/Lists/VirtualizedList.js +++ b/Libraries/Lists/VirtualizedList.js @@ -757,7 +757,7 @@ class VirtualizedList extends React.PureComponent { static _createRenderMask( props: Props, cellsAroundViewport: {first: number, last: number}, - lastFocusedItem: ?number, + additionalRegions?: ?$ReadOnlyArray<{first: number, last: number}>, ): CellRenderMask { const itemCount = props.getItemCount(props.data); @@ -770,17 +770,12 @@ class VirtualizedList extends React.PureComponent { const renderMask = new CellRenderMask(itemCount); - // Keep the items around the last focused rendered, to allow for keyboard - // navigation - if (lastFocusedItem) { - const first = Math.max(0, lastFocusedItem - 1); - const last = Math.min(itemCount - 1, lastFocusedItem + 1); - renderMask.addCells({first, last}); - } - if (itemCount > 0) { - if (cellsAroundViewport.last >= cellsAroundViewport.first) { - renderMask.addCells(cellsAroundViewport); + const allRegions = [cellsAroundViewport, ...(additionalRegions ?? [])]; + for (const region of allRegions) { + if (region.last >= region.first) { + renderMask.addCells(region); + } } // The initially rendered cells are retained as part of the @@ -1018,7 +1013,7 @@ class VirtualizedList extends React.PureComponent { prevCellKey={prevCellKey} onUpdateSeparators={this._onUpdateSeparators} onLayout={e => this._onCellLayout(e, key, ii)} - onFocusCapture={e => this._onCellFocusCapture(ii)} + onFocusCapture={e => this._onCellFocusCapture(key)} onUnmount={this._onCellUnmount} parentProps={this.props} ref={ref => { @@ -1366,7 +1361,7 @@ class VirtualizedList extends React.PureComponent { _averageCellLength = 0; // Maps a cell key to the set of keys for all outermost child lists within that cell _cellKeysToChildListKeys: Map> = new Map(); - _cellRefs = {}; + _cellRefs: {[string]: ?CellRenderer} = {}; _fillRateHelper: FillRateHelper; _frames = {}; _footerLength = 0; @@ -1378,7 +1373,7 @@ class VirtualizedList extends React.PureComponent { _hiPriInProgress: boolean = false; // flag to prevent infinite hiPri cell limit update _highestMeasuredFrameIndex = 0; _indicesToKeys: Map = new Map(); - _lastFocusedItem: ?number = null; + _lastFocusedCellKey: ?string = null; _nestedChildLists: Map< string, { @@ -1487,12 +1482,12 @@ class VirtualizedList extends React.PureComponent { this._updateViewableItems(this.props.data); } - _onCellFocusCapture(itemIndex: number) { - this._lastFocusedItem = itemIndex; + _onCellFocusCapture(cellKey: string) { + this._lastFocusedCellKey = cellKey; const renderMask = VirtualizedList._createRenderMask( this.props, this.state.cellsAroundViewport, - this._lastFocusedItem, + this._getNonViewportRenderRegions(), ); if (!renderMask.equals(this.state.renderMask)) { @@ -1928,7 +1923,7 @@ class VirtualizedList extends React.PureComponent { const renderMask = VirtualizedList._createRenderMask( props, cellsAroundViewport, - this._lastFocusedItem, + this._getNonViewportRenderRegions(), ); if ( @@ -2000,6 +1995,57 @@ class VirtualizedList extends React.PureComponent { return frame; }; + _getNonViewportRenderRegions = (): $ReadOnlyArray<{ + first: number, + last: number, + }> => { + // Keep a viewport's worth of content around the last focused cell to allow + // random navigation around it without any blanking. E.g. tabbing from one + // focused item out of viewport to another. + if ( + !(this._lastFocusedCellKey && this._cellRefs[this._lastFocusedCellKey]) + ) { + return []; + } + + const lastFocusedCellRenderer = this._cellRefs[this._lastFocusedCellKey]; + const focusedCellIndex = lastFocusedCellRenderer.props.index; + const itemCount = this.props.getItemCount(this.props.data); + + // The cell may have been unmounted and have a stale index + if ( + focusedCellIndex >= itemCount || + this._indicesToKeys.get(focusedCellIndex) !== this._lastFocusedCellKey + ) { + return []; + } + + let first = focusedCellIndex; + let heightOfCellsBeforeFocused = 0; + for ( + let i = first - 1; + i >= 0 && heightOfCellsBeforeFocused < this._scrollMetrics.visibleLength; + i-- + ) { + first--; + heightOfCellsBeforeFocused += this._getFrameMetricsApprox(i).length; + } + + let last = focusedCellIndex; + let heightOfCellsAfterFocused = 0; + for ( + let i = last + 1; + i < itemCount && + heightOfCellsAfterFocused < this._scrollMetrics.visibleLength; + i++ + ) { + last++; + heightOfCellsAfterFocused += this._getFrameMetricsApprox(i).length; + } + + return [{first, last}]; + }; + _updateViewableItems(data: any) { const {getItemCount} = this.props; diff --git a/Libraries/Lists/__tests__/VirtualizedList-test.js b/Libraries/Lists/__tests__/VirtualizedList-test.js index cd2f5902f15895..710b9a701570c7 100644 --- a/Libraries/Lists/__tests__/VirtualizedList-test.js +++ b/Libraries/Lists/__tests__/VirtualizedList-test.js @@ -1445,7 +1445,7 @@ it('renders windowSize derived region at bottom', () => { expect(component).toMatchSnapshot(); }); -it('keeps last focused item rendered', () => { +it('keeps viewport below last focused rendered', () => { const items = generateItems(20); const ITEM_HEIGHT = 10; @@ -1479,24 +1479,147 @@ it('keeps last focused item rendered', () => { performAllBatches(); }); - // Cells 1-4 should remain rendered after scrolling to the bottom of the list + // Cells 1-8 should remain rendered after scrolling to the bottom of the list expect(component).toMatchSnapshot(); +}); + +it('virtualizes away last focused item if focus changes to a new cell', () => { + const items = generateItems(20); + const ITEM_HEIGHT = 10; + + let component; + ReactTestRenderer.act(() => { + component = ReactTestRenderer.create( + , + ); + }); + + ReactTestRenderer.act(() => { + simulateLayout(component, { + viewport: {width: 10, height: 50}, + content: {width: 10, height: 200}, + }); + + performAllBatches(); + }); + + ReactTestRenderer.act(() => { + component.getInstance()._onCellFocusCapture(3); + }); + + ReactTestRenderer.act(() => { + simulateScroll(component, {x: 0, y: 150}); + performAllBatches(); + }); ReactTestRenderer.act(() => { component.getInstance()._onCellFocusCapture(17); }); - // Cells 2-4 should no longer be rendered after focus is moved to the end of + // Cells 1-8 should no longer be rendered after focus is moved to the end of // the list expect(component).toMatchSnapshot(); +}); + +it('keeps viewport above last focused rendered', () => { + const items = generateItems(20); + const ITEM_HEIGHT = 10; + + let component; + ReactTestRenderer.act(() => { + component = ReactTestRenderer.create( + , + ); + }); + + ReactTestRenderer.act(() => { + simulateLayout(component, { + viewport: {width: 10, height: 50}, + content: {width: 10, height: 200}, + }); + + performAllBatches(); + }); + + ReactTestRenderer.act(() => { + component.getInstance()._onCellFocusCapture(3); + }); + + ReactTestRenderer.act(() => { + simulateScroll(component, {x: 0, y: 150}); + performAllBatches(); + }); + + ReactTestRenderer.act(() => { + component.getInstance()._onCellFocusCapture(17); + }); ReactTestRenderer.act(() => { simulateScroll(component, {x: 0, y: 0}); performAllBatches(); }); - // Cells 16-18 should remain rendered after scrolling back to the top of the - // list + // Cells 12-19 should remain rendered after scrolling to the top of the list + expect(component).toMatchSnapshot(); +}); + +it('virtualizes away last focused index if item removed', () => { + const items = generateItems(20); + const ITEM_HEIGHT = 10; + + let component; + ReactTestRenderer.act(() => { + component = ReactTestRenderer.create( + , + ); + }); + + ReactTestRenderer.act(() => { + simulateLayout(component, { + viewport: {width: 10, height: 50}, + content: {width: 10, height: 200}, + }); + + performAllBatches(); + }); + + ReactTestRenderer.act(() => { + component.getInstance()._onCellFocusCapture(3); + }); + + ReactTestRenderer.act(() => { + simulateScroll(component, {x: 0, y: 150}); + performAllBatches(); + }); + + const itemsWithoutFocused = [...items.slice(0, 3), ...items.slice(4)]; + ReactTestRenderer.act(() => { + component.update( + , + ); + }); + + // Cells 1-8 should no longer be rendered expect(component).toMatchSnapshot(); }); diff --git a/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap b/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap index 6f4721ff239a16..4a06e273d5e14f 100644 --- a/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap +++ b/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap @@ -2781,7 +2781,7 @@ exports[`expands render area by maxToRenderPerBatch on tick 1`] = ` `; -exports[`keeps last focused item rendered 1`] = ` +exports[`keeps viewport above last focused rendered 1`] = ` + onFocusCapture={[Function]} + style={null} + > + + + + + + + + `; -exports[`keeps last focused item rendered 2`] = ` +exports[`keeps viewport below last focused rendered 1`] = ` - - - -`; - -exports[`keeps last focused item rendered 3`] = ` - - + - - `; @@ -5167,3 +5083,356 @@ exports[`unmounts sticky headers moved below viewport 1`] = ` `; + +exports[`virtualizes away last focused index if item removed 1`] = ` + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +`; + +exports[`virtualizes away last focused item if focus changes to a new cell 1`] = ` + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +`;