From 20b0c41b61b7a1aed7dd739b414903bd1466ea9d Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Tue, 10 Oct 2023 08:56:50 -0700 Subject: [PATCH] Remove code to support bottom-up layout events in horizontal RTL (#39646) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/39646 We can dramatically simplify this code and remove quirks/hacks, now that we can assume layout events are always fired top down. Changelog: [Internal] Reviewed By: yungsters Differential Revision: D49628669 fbshipit-source-id: 3332770f8cff3c3c423d777a25085a2c58fc9d4c --- .../Lists/ListMetricsAggregator.js | 100 ++---------- .../Lists/VirtualizedList.js | 35 +--- .../__tests__/ListMetricsAggregator-test.js | 149 ++++-------------- 3 files changed, 41 insertions(+), 243 deletions(-) diff --git a/packages/virtualized-lists/Lists/ListMetricsAggregator.js b/packages/virtualized-lists/Lists/ListMetricsAggregator.js index 23f7f8d327ca16..01d6e5aff29c5f 100644 --- a/packages/virtualized-lists/Lists/ListMetricsAggregator.js +++ b/packages/virtualized-lists/Lists/ListMetricsAggregator.js @@ -14,8 +14,6 @@ import {keyExtractor as defaultKeyExtractor} from './VirtualizeUtils'; import invariant from 'invariant'; -type LayoutEventDirection = 'top-down' | 'bottom-up'; - export type CellMetrics = { /** * Index of the item in the list @@ -55,25 +53,12 @@ export type CellMetricProps = { ... }; -type UnresolvedCellMetrics = { - index: number, - layout: Layout, - isMounted: boolean, - - // The length of list content at the time of layout is needed to correctly - // resolve flow relative offset in RTL. We are lazily notified of this after - // the layout of the cell, unless the cell relayout does not cause a length - // change. To keep stability, we use content length at time of query, or - // unmount if never queried. - listContentLength?: ?number, -}; - /** * Provides an interface to query information about the metrics of a list and its cells. */ export default class ListMetricsAggregator { _averageCellLength = 0; - _cellMetrics: Map = new Map(); + _cellMetrics: Map = new Map(); _contentLength: ?number; _highestMeasuredCellIndex = 0; _measuredCellsLength = 0; @@ -83,10 +68,6 @@ export default class ListMetricsAggregator { rtl: false, }; - // Fabric and Paper may call onLayout in different orders. We can tell which - // direction layout events happen on the first layout. - _onLayoutDirection: LayoutEventDirection = 'top-down'; - /** * Notify the ListMetricsAggregator that a cell has been laid out. * @@ -103,39 +84,22 @@ export default class ListMetricsAggregator { orientation: ListOrientation, layout: Layout, }): boolean { - if (this._contentLength == null) { - this._onLayoutDirection = 'bottom-up'; - } - this._invalidateIfOrientationChanged(orientation); - // If layout is top-down, our most recently cached content length - // corresponds to this cell. Otherwise, we need to resolve when events fire - // up the tree to the new length. - const listContentLength = - this._onLayoutDirection === 'top-down' ? this._contentLength : null; - - const next: UnresolvedCellMetrics = { + const next: CellMetrics = { index: cellIndex, - layout: layout, + length: this._selectLength(layout), isMounted: true, - listContentLength, + offset: this.flowRelativeOffset(layout), }; const curr = this._cellMetrics.get(cellKey); - if ( - !curr || - this._selectOffset(next.layout) !== this._selectOffset(curr.layout) || - this._selectLength(next.layout) !== this._selectLength(curr.layout) || - (curr.listContentLength != null && - curr.listContentLength !== this._contentLength) - ) { + if (!curr || next.offset !== curr.offset || next.length !== curr.length) { if (curr) { - const dLength = - this._selectLength(next.layout) - this._selectLength(curr.layout); + const dLength = next.length - curr.length; this._measuredCellsLength += dLength; } else { - this._measuredCellsLength += this._selectLength(next.layout); + this._measuredCellsLength += next.length; this._measuredCellsCount += 1; } @@ -174,21 +138,7 @@ export default class ListMetricsAggregator { layout: $ReadOnly<{width: number, height: number}>, }): void { this._invalidateIfOrientationChanged(orientation); - const newLength = this._selectLength(layout); - - // Fill in any just-measured cells which did not have this length available. - // This logic assumes that cell relayout will always change list content - // size, which isn't strictly correct, but issues should be rare and only - // on Paper. - if (this._onLayoutDirection === 'bottom-up') { - for (const cellMetric of this._cellMetrics.values()) { - if (cellMetric.listContentLength == null) { - cellMetric.listContentLength = newLength; - } - } - } - - this._contentLength = newLength; + this._contentLength = this._selectLength(layout); } /** @@ -245,7 +195,7 @@ export default class ListMetricsAggregator { keyExtractor(getItem(data, index), index), ); if (frame && frame.index === index) { - return this._resolveCellMetrics(frame); + return frame; } if (getItemLayout) { @@ -286,26 +236,6 @@ export default class ListMetricsAggregator { return this._contentLength != null; } - /** - * Whether the ListMetricsAggregator is notified of cell metrics before - * ScrollView metrics (bottom-up) or ScrollView metrics before cell metrics - * (top-down). - * - * Must be queried after cell layout - */ - getLayoutEventDirection(): LayoutEventDirection { - return this._onLayoutDirection; - } - - /** - * Whether the ListMetricsAggregator must be aware of the current length of - * ScrollView content to be able to correctly resolve the (flow-relative) - * metrics of a cell. - */ - needsContentLengthForCellMetrics(): boolean { - return this._orientation.horizontal && this._orientation.rtl; - } - /** * Finds the flow-relative offset (e.g. starting from the left in LTR, but * right in RTL) from a layout box. @@ -352,7 +282,6 @@ export default class ListMetricsAggregator { if (orientation.horizontal !== this._orientation.horizontal) { this._averageCellLength = 0; - this._contentLength = null; this._highestMeasuredCellIndex = 0; this._measuredCellsLength = 0; this._measuredCellsCount = 0; @@ -371,15 +300,4 @@ export default class ListMetricsAggregator { _selectOffset({x, y}: $ReadOnly<{x: number, y: number, ...}>): number { return this._orientation.horizontal ? x : y; } - - _resolveCellMetrics(metrics: UnresolvedCellMetrics): CellMetrics { - const {index, layout, isMounted, listContentLength} = metrics; - - return { - index, - length: this._selectLength(layout), - isMounted, - offset: this.flowRelativeOffset(layout, listContentLength), - }; - } } diff --git a/packages/virtualized-lists/Lists/VirtualizedList.js b/packages/virtualized-lists/Lists/VirtualizedList.js index fed3f3f140b9a4..dcd9cbaaeb103d 100644 --- a/packages/virtualized-lists/Lists/VirtualizedList.js +++ b/packages/virtualized-lists/Lists/VirtualizedList.js @@ -1302,39 +1302,13 @@ class VirtualizedList extends StateSafePureComponent { orientation: this._orientation(), }); - // In RTL layout we need parent content length to calculate the offset of a - // cell from the start of the list. In Paper, layout events are bottom up, - // so we do not know this yet, and must defer calculation until after - // `onContentSizeChange` is called. - const deferCellMetricCalculation = - this._listMetrics.getLayoutEventDirection() === 'bottom-up' && - this._listMetrics.needsContentLengthForCellMetrics(); - - // Note: In Paper RTL logical position may have changed when - // `layoutHasChanged` is false if a cell maintains same X/Y coordinates, - // but contentLength shifts. This will be corrected by - // `onContentSizeChange` triggering a cell update. if (layoutHasChanged) { - this._scheduleCellsToRenderUpdate({ - allowImmediateExecution: !deferCellMetricCalculation, - }); + this._scheduleCellsToRenderUpdate(); } this._triggerRemeasureForChildListsInCell(cellKey); - this._computeBlankness(); - - if (deferCellMetricCalculation) { - if (!this._pendingViewabilityUpdate) { - this._pendingViewabilityUpdate = true; - setTimeout(() => { - this._updateViewableItems(this.props, this.state.cellsAroundViewport); - this._pendingViewabilityUpdate = false; - }, 0); - } - } else { - this._updateViewableItems(this.props, this.state.cellsAroundViewport); - } + this._updateViewableItems(this.props, this.state.cellsAroundViewport); }; _onCellFocusCapture(cellKey: string) { @@ -1765,9 +1739,7 @@ class VirtualizedList extends StateSafePureComponent { } } - _scheduleCellsToRenderUpdate(opts?: {allowImmediateExecution?: boolean}) { - const allowImmediateExecution = opts?.allowImmediateExecution ?? true; - + _scheduleCellsToRenderUpdate() { // Only trigger high-priority updates if we've actually rendered cells, // and with that size estimate, accurately compute how many cells we should render. // Otherwise, it would just render as many cells as it can (of zero dimension), @@ -1776,7 +1748,6 @@ class VirtualizedList extends StateSafePureComponent { // If this is triggered in an `componentDidUpdate` followed by a hiPri cellToRenderUpdate // We shouldn't do another hipri cellToRenderUpdate if ( - allowImmediateExecution && this._shouldRenderWithPriority() && (this._listMetrics.getAverageCellLength() || this.props.getItemLayout) && !this._hiPriInProgress diff --git a/packages/virtualized-lists/Lists/__tests__/ListMetricsAggregator-test.js b/packages/virtualized-lists/Lists/__tests__/ListMetricsAggregator-test.js index 7426b4c1617b8a..da2bb5df90f6fc 100644 --- a/packages/virtualized-lists/Lists/__tests__/ListMetricsAggregator-test.js +++ b/packages/virtualized-lists/Lists/__tests__/ListMetricsAggregator-test.js @@ -437,6 +437,11 @@ describe('ListMetricsAggregator', () => { getItem: (i: number) => nullthrows(props.data)[i], }; + listMetrics.notifyListContentLayout({ + layout: {width: 100, height: 5}, + orientation, + }); + listMetrics.notifyCellLayout({ cellIndex: 0, cellKey: '0', @@ -461,11 +466,6 @@ describe('ListMetricsAggregator', () => { }, }); - listMetrics.notifyListContentLayout({ - layout: {width: 100, height: 5}, - orientation, - }); - expect(listMetrics.getCellMetrics(1, props)).toEqual({ index: 1, length: 20, @@ -489,6 +489,11 @@ describe('ListMetricsAggregator', () => { getItem: (i: number) => nullthrows(props.data)[i], }; + listMetrics.notifyListContentLayout({ + layout: {width: 100, height: 5}, + orientation, + }); + listMetrics.notifyCellLayout({ cellIndex: 0, cellKey: '0', @@ -513,11 +518,6 @@ describe('ListMetricsAggregator', () => { }, }); - listMetrics.notifyListContentLayout({ - layout: {width: 100, height: 5}, - orientation, - }); - expect(listMetrics.getCellMetrics(2, props)).toBeNull(); expect(listMetrics.getCellMetricsApprox(2, props)).toEqual({ index: 2, @@ -537,6 +537,11 @@ describe('ListMetricsAggregator', () => { getItemLayout: () => ({index: 2, length: 40, offset: 30}), }; + listMetrics.notifyListContentLayout({ + layout: {width: 100, height: 5}, + orientation, + }); + listMetrics.notifyCellLayout({ cellIndex: 0, cellKey: '0', @@ -561,11 +566,6 @@ describe('ListMetricsAggregator', () => { }, }); - listMetrics.notifyListContentLayout({ - layout: {width: 100, height: 5}, - orientation, - }); - expect(listMetrics.getCellMetrics(2, props)).toMatchObject({ index: 2, length: 40, @@ -713,98 +713,7 @@ describe('ListMetricsAggregator', () => { }); }); - it('resolves metrics of unmounted cell after list shift when using bottom-up layout propagation', () => { - const listMetrics = new ListMetricsAggregator(); - const orientation = {horizontal: true, rtl: true}; - const props: CellMetricProps = { - data: [1, 2, 3, 4, 5], - getItemCount: () => nullthrows(props.data).length, - getItem: (i: number) => nullthrows(props.data)[i], - }; - - listMetrics.notifyCellLayout({ - cellIndex: 0, - cellKey: '0', - orientation, - layout: { - height: 5, - width: 10, - x: 90, - y: 0, - }, - }); - - listMetrics.notifyCellLayout({ - cellIndex: 1, - cellKey: '1', - orientation, - layout: { - height: 5, - width: 20, - x: 70, - y: 0, - }, - }); - - listMetrics.notifyListContentLayout({ - layout: {width: 100, height: 5}, - orientation, - }); - - expect(listMetrics.getCellMetrics(1, props)).toEqual({ - index: 1, - length: 20, - offset: 10, - isMounted: true, - }); - - listMetrics.notifyCellLayout({ - cellIndex: 2, - cellKey: '2', - orientation, - layout: { - height: 5, - width: 20, - x: 50, - y: 0, - }, - }); - - listMetrics.notifyListContentLayout({ - layout: {width: 120, height: 5}, - orientation, - }); - - expect(listMetrics.getCellMetrics(1, props)).toEqual({ - index: 1, - length: 20, - offset: 10, - isMounted: true, - }); - - listMetrics.notifyCellUnmounted('1'); - - expect(listMetrics.getCellMetrics(1, props)).toEqual({ - index: 1, - length: 20, - offset: 10, - isMounted: false, - }); - - listMetrics.notifyListContentLayout({ - layout: {width: 100, height: 5}, - orientation, - }); - - expect(listMetrics.getCellMetrics(1, props)).toEqual({ - index: 1, - length: 20, - offset: 10, - isMounted: false, - }); - }); - - it('resolves metrics of unmounted cell after list shift when using top-down layout propagation', () => { + it('resolves metrics of unmounted cell after list shift', () => { const listMetrics = new ListMetricsAggregator(); const orientation = {horizontal: true, rtl: true}; const props: CellMetricProps = { @@ -1089,18 +998,18 @@ describe('ListMetricsAggregator', () => { getItem: (i: number) => nullthrows(props.data)[i], }; - listMetrics.notifyCellLayout({ - cellIndex: 0, - cellKey: '0', - orientation, - layout: { - height: 10, - width: 5, - x: 0, - y: 0, - }, - }); - - expect(() => listMetrics.getCellMetrics(0, props)).toThrow(); + expect(() => + listMetrics.notifyCellLayout({ + cellIndex: 0, + cellKey: '0', + orientation, + layout: { + height: 10, + width: 5, + x: 0, + y: 0, + }, + }), + ).toThrow(); }); });