From 3ab1de1102611c5c9d904fd651eb89ed3d29c175 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Tue, 7 Dec 2021 19:14:37 -0800 Subject: [PATCH 1/2] 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 25e02db40d31ef..f4887ed5617fbc 100644 --- a/Libraries/Lists/VirtualizedList.js +++ b/Libraries/Lists/VirtualizedList.js @@ -755,7 +755,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); @@ -768,17 +768,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 @@ -1016,7 +1011,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 => { @@ -1364,7 +1359,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; @@ -1376,7 +1371,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, { @@ -1485,12 +1480,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)) { @@ -1926,7 +1921,7 @@ class VirtualizedList extends React.PureComponent { const renderMask = VirtualizedList._createRenderMask( props, cellsAroundViewport, - this._lastFocusedItem, + this._getNonViewportRenderRegions(), ); if ( @@ -1998,6 +1993,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 6ec0a178c3b0ba..eeeded8db344a5 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`] = ` + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +`; From b8a64f0074aa4b1d858baa5f0694ec7dfeab9fdc Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Thu, 16 Dec 2021 19:09:53 -0800 Subject: [PATCH 2/2] office-shared-comments-ui aeb76bb4 --- Libraries/Lists/VirtualizedList.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Libraries/Lists/VirtualizedList.js b/Libraries/Lists/VirtualizedList.js index 3ed0f0b55a1d38..520d1075af1523 100644 --- a/Libraries/Lists/VirtualizedList.js +++ b/Libraries/Lists/VirtualizedList.js @@ -1842,7 +1842,7 @@ class VirtualizedList extends React.PureComponent { } // Mark as high priority if we're close to the end of the last item // But only if there are items after the last rendered item - if (last < itemCount - 1) { + if (last > 0 && last < itemCount - 1) { const distBottom = this._getFrameMetricsApprox(last).offset - (offset + visibleLength); hiPri = @@ -1954,7 +1954,11 @@ class VirtualizedList extends React.PureComponent { // check for invalid frames due to row re-ordering return frame; } else { - const {getItemLayout} = this.props; + const {data, getItemCount, getItemLayout} = this.props; + invariant( + index >= 0 && index < getItemCount(data), + 'Tried to get frame for out of range index ' + index, + ); invariant( !getItemLayout, 'Should not have to estimate frames when a measurement metrics function is provided', @@ -1977,7 +1981,7 @@ class VirtualizedList extends React.PureComponent { } => { const {data, getItem, getItemCount, getItemLayout} = this.props; invariant( - getItemCount(data) > index, + index >= 0 && index < getItemCount(data), 'Tried to get frame for out of range index ' + index, ); const item = getItem(data, index);