From 1ff698df536810e32e0058553871d8b47d6da13c Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Thu, 20 Jul 2023 05:05:44 -0700 Subject: [PATCH] Fix virtualization logic with horizontal RTL lists Summary: VirtualizedList internally represents metrics along the scrolling axis using `offset` (x, y), and `length` (width, height). This one dimensional representation allows the list to be generic to being horizontal or vertical. Right now offset conversion directly takes the `x` or `y` axis value, but this is not correct when we are using an inverted FlatList, or a horizontal FlatList in RTL. This change converts most VirtualizedList code handling `offset,length` coordinates consistently flow from start to end, including in horizontal RTL and in inverted FlatList. This is paired with a fix to Android native code in D47627115. With these together, basic Horizontal FlatList scenarios should work correctly when laid out in RTL. Differential Revision: D46586420 fbshipit-source-id: 0e6a89271d942d81102716a4ec63870ac1d52ed3 --- .../Lists/ListMetricsAggregator.js | 222 ++++++++++++++--- .../Lists/VirtualizedList.js | 223 ++++++++++-------- 2 files changed, 315 insertions(+), 130 deletions(-) diff --git a/packages/virtualized-lists/Lists/ListMetricsAggregator.js b/packages/virtualized-lists/Lists/ListMetricsAggregator.js index 3fcb051525114b..36460759b9c604 100644 --- a/packages/virtualized-lists/Lists/ListMetricsAggregator.js +++ b/packages/virtualized-lists/Lists/ListMetricsAggregator.js @@ -8,6 +8,7 @@ * @format */ +import type {Layout} from 'react-native/Libraries/Types/CoreEventTypes'; import type {Props as VirtualizedListProps} from './VirtualizedListProps'; import {keyExtractor as defaultKeyExtractor} from './VirtualizeUtils'; @@ -23,7 +24,8 @@ export type CellMetrics = { */ length: number, /** - * Offset to the cell along the scrolling axis + * Distance between this cell and the start of the list along the scrolling + * axis */ offset: number, /** @@ -32,6 +34,13 @@ export type CellMetrics = { isMounted: boolean, }; +// TODO: `inverted` can be incorporated here if it is moved to an order +// based implementation instead of transform. +export type ListOrientation = { + horizontal: boolean, + rtl: boolean, +}; + /** * Subset of VirtualizedList props needed to calculate cell metrics */ @@ -44,52 +53,85 @@ 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; - _frames: {[string]: CellMetrics} = {}; + _cellMetrics: {[string]: UnresolvedCellMetrics} = {}; + _contentLength: ?number; _highestMeasuredCellIndex = 0; - _totalCellLength = 0; - _totalCellsMeasured = 0; + _measuredCellsLength = 0; + _measuredCellsCount = 0; + _orientation: ListOrientation = { + horizontal: false, + rtl: false, + }; /** * Notify the ListMetricsAggregator that a cell has been laid out. * * @returns whether the cell layout has changed since last notification */ - notifyCellLayout( + notifyCellLayout({ + cellIndex, + cellKey, + orientation, + layout, + }: { + cellIndex: number, cellKey: string, - index: number, - length: number, - offset: number, - ): boolean { - const next: CellMetrics = { - offset, - length, - index, + orientation: ListOrientation, + layout: Layout, + }): boolean { + this._invalidateIfOrientationChanged(orientation); + + const next: UnresolvedCellMetrics = { + index: cellIndex, + layout: layout, isMounted: true, }; - const curr = this._frames[cellKey]; + const curr = this._cellMetrics[cellKey]; + if ( !curr || - next.offset !== curr.offset || - next.length !== curr.length || - index !== curr.index + this._selectOffset(next.layout) !== this._selectOffset(curr.layout) || + this._selectLength(next.layout) !== this._selectLength(curr.layout) || + (curr.listContentLength != null && + curr.listContentLength !== this._contentLength) ) { - this._totalCellLength += next.length - (curr ? curr.length : 0); - this._totalCellsMeasured += curr ? 0 : 1; + if (curr) { + const dLength = + this._selectLength(next.layout) - this._selectLength(curr.layout); + this._measuredCellsLength += dLength; + } else { + this._measuredCellsLength += this._selectLength(next.layout); + this._measuredCellsCount += 1; + } + this._averageCellLength = - this._totalCellLength / this._totalCellsMeasured; - this._frames[cellKey] = next; + this._measuredCellsLength / this._measuredCellsCount; + this._cellMetrics[cellKey] = next; this._highestMeasuredCellIndex = Math.max( this._highestMeasuredCellIndex, - index, + cellIndex, ); return true; } else { - this._frames[cellKey].isMounted = true; + this._cellMetrics[cellKey].isMounted = true; return false; } } @@ -98,12 +140,31 @@ export default class ListMetricsAggregator { * Notify ListMetricsAggregator that a cell has been unmounted. */ notifyCellUnmounted(cellKey: string): void { - const curr = this._frames[cellKey]; + const curr = this._cellMetrics[cellKey]; if (curr) { - this._frames[cellKey] = {...curr, isMounted: false}; + this._cellMetrics[cellKey] = { + ...curr, + isMounted: false, + listContentLength: curr.listContentLength ?? this._contentLength, + }; } } + /** + * Notify ListMetricsAggregator that the lists content container has been laid out. + */ + notifyListContentLayout({ + orientation, + layout, + }: { + orientation: ListOrientation, + layout: $ReadOnly<{width: number, height: number}>, + }): void { + this._invalidateIfOrientationChanged(orientation); + const newLength = this._selectLength(layout); + this._contentLength = newLength; + } + /** * Return the average length of the cells which have been measured */ @@ -123,7 +184,10 @@ export default class ListMetricsAggregator { * otherwise an estimate based on the average length of previously measured * cells */ - getCellMetricsApprox(index: number, props: CellMetricProps): CellMetrics { + getCellMetricsApprox( + index: number, + props: CellMetricProps, + ): $ReadOnly { const frame = this.getCellMetrics(index, props); if (frame && frame.index === index) { // check for invalid frames due to row re-ordering @@ -146,21 +210,27 @@ export default class ListMetricsAggregator { /** * Returns the exact metrics of a cell if it has already been laid out */ - getCellMetrics(index: number, props: CellMetricProps): ?CellMetrics { + getCellMetrics( + index: number, + props: CellMetricProps, + ): ?$ReadOnly { const {data, getItem, getItemCount, getItemLayout} = props; invariant( index >= 0 && index < getItemCount(data), - 'Tried to get frame for out of range index ' + index, + 'Tried to get metrics for out of range cell index ' + index, ); const keyExtractor = props.keyExtractor ?? defaultKeyExtractor; - const frame = this._frames[keyExtractor(getItem(data, index), index)]; - if (!frame || frame.index !== index) { - if (getItemLayout) { - const {length, offset} = getItemLayout(data, index); - return {index, length, offset, isMounted: true}; - } + const frame = this._cellMetrics[keyExtractor(getItem(data, index), index)]; + if (frame && frame.index === index) { + return this._resolveCellMetrics(frame); } - return frame; + + if (getItemLayout) { + const {length, offset} = getItemLayout(data, index); + return {index, length, offset, isMounted: true}; + } + + return null; } /** @@ -176,4 +246,86 @@ export default class ListMetricsAggregator { return frameMetrics.offset + remainder * frameMetrics.length; } } + + /** + * Returns the length of all ScrollView content along the scrolling axis. + */ + getContentLength(): number { + return this._contentLength ?? 0; + } + + /** + * Converts a cartesian offset along the x or y axis to a flow-relative + * offset, (e.g. starting from the left in LTR, but right in RTL). + */ + flowRelativeOffset(layout: Layout, referenceContentLength?: ?number): number { + const {horizontal, rtl} = this._orientation; + + if (horizontal && rtl) { + const contentLength = referenceContentLength ?? this._contentLength; + invariant( + contentLength != null, + 'ListMetricsAggregator must be notified of list content layout before resolving offsets', + ); + return contentLength - this._selectOffset(layout); + } else { + return this._selectOffset(layout); + } + } + + /** + * Converts a flow-relative offset to a cartesian offset + */ + cartesianOffset(flowRelativeOffset: number): {x?: number, y?: number} { + invariant( + this._contentLength != null, + 'ListMetricsAggregator must be notified of list content layout before resolving offsets', + ); + + const {horizontal, rtl} = this._orientation; + const cartOffset = + horizontal && rtl + ? this._contentLength - flowRelativeOffset + : flowRelativeOffset; + + return horizontal ? {x: cartOffset} : {y: cartOffset}; + } + + _invalidateIfOrientationChanged(orientation: ListOrientation): void { + if (orientation.rtl !== this._orientation.rtl) { + this._orientation = orientation; + this._cellMetrics = {}; + } + + if (orientation.horizontal !== this._orientation.horizontal) { + this._averageCellLength = 0; + this._contentLength = 0; + this._highestMeasuredCellIndex = 0; + this._measuredCellsLength = 0; + this._measuredCellsCount = 0; + } + } + + _selectLength({ + width, + height, + }: $ReadOnly<{width: number, height: number, ...}>): number { + return this._orientation.horizontal ? width : height; + } + + _selectOffset({x, y}: $ReadOnly<{x: number, y: number, ...}>): number { + return this._orientation.horizontal ? x : y; + } + + _resolveCellMetrics(metrics: UnresolvedCellMetrics): CellMetrics { + metrics.listContentLength ??= this._contentLength; + 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 50dad2f2dab389..b8bd1038c64559 100644 --- a/packages/virtualized-lists/Lists/VirtualizedList.js +++ b/packages/virtualized-lists/Lists/VirtualizedList.js @@ -22,9 +22,11 @@ import type { RenderItemType, Separators, } from './VirtualizedListProps'; -import type {CellMetricProps} from './ListMetricsAggregator'; +import type {CellMetricProps, ListOrientation} from './ListMetricsAggregator'; import { + I18nManager, + Platform, RefreshControl, ScrollView, View, @@ -142,37 +144,12 @@ class VirtualizedList extends StateSafePureComponent { // scrollToEnd may be janky without getItemLayout prop scrollToEnd(params?: ?{animated?: ?boolean, ...}) { const animated = params ? params.animated : true; - const veryLast = this.props.getItemCount(this.props.data) - 1; - if (veryLast < 0) { - return; - } - const frame = this._listMetrics.getCellMetricsApprox(veryLast, this.props); - const offset = Math.max( - 0, - frame.offset + - frame.length + - this._footerLength - + this.scrollToOffset({ + animated, + offset: + this._listMetrics.getContentLength() - this._scrollMetrics.visibleLength, - ); - - if (this._scrollRef == null) { - return; - } - - if (this._scrollRef.scrollTo == null) { - console.warn( - 'No scrollTo method provided. This may be because you have two nested ' + - 'VirtualizedLists with the same orientation, or because you are ' + - 'using a custom component that does not implement scrollTo.', - ); - return; - } - - this._scrollRef.scrollTo( - horizontalOrDefault(this.props.horizontal) - ? {x: offset, animated} - : {y: offset, animated}, - ); + }); } // scrollToIndex may be janky without getItemLayout prop @@ -286,12 +263,13 @@ class VirtualizedList extends StateSafePureComponent { */ scrollToOffset(params: {animated?: ?boolean, offset: number, ...}) { const {animated, offset} = params; + const scrollRef = this._scrollRef; - if (this._scrollRef == null) { + if (scrollRef == null) { return; } - if (this._scrollRef.scrollTo == null) { + if (scrollRef.scrollTo == null) { console.warn( 'No scrollTo method provided. This may be because you have two nested ' + 'VirtualizedLists with the same orientation, or because you are ' + @@ -300,11 +278,10 @@ class VirtualizedList extends StateSafePureComponent { return; } - this._scrollRef.scrollTo( - horizontalOrDefault(this.props.horizontal) - ? {x: offset, animated} - : {y: offset, animated}, - ); + scrollRef.scrollTo({ + animated, + ...this._listMetrics.cartesianOffset(offset), + }); } recordInteraction() { @@ -621,7 +598,8 @@ class VirtualizedList extends StateSafePureComponent { const onEndReachedThreshold = onEndReachedThresholdOrDefault( props.onEndReachedThreshold, ); - const {contentLength, offset, visibleLength} = this._scrollMetrics; + const {offset, visibleLength} = this._scrollMetrics; + const contentLength = this._listMetrics.getContentLength(); const distanceFromEnd = contentLength - visibleLength - offset; // Wait until the scroll view metrics have been set up. And until then, @@ -1220,9 +1198,15 @@ class VirtualizedList extends StateSafePureComponent { new ChildListCollection(); _offsetFromParentVirtualizedList: number = 0; _prevParentOffset: number = 0; - // $FlowFixMe[missing-local-annot] - _scrollMetrics = { - contentLength: 0, + _scrollMetrics: { + dOffset: number, + dt: number, + offset: number, + timestamp: number, + velocity: number, + visibleLength: number, + zoomScale: number, + } = { dOffset: 0, dt: 10, offset: 0, @@ -1291,19 +1275,27 @@ class VirtualizedList extends StateSafePureComponent { } }; - _onCellLayout = (e: LayoutEvent, cellKey: string, index: number): void => { - const layout = e.nativeEvent.layout; - const offset = this._selectOffset(layout); - const length = this._selectLength(layout); - - const layoutHasChanged = this._listMetrics.notifyCellLayout( + _onCellLayout = ( + e: LayoutEvent, + cellKey: string, + cellIndex: number, + ): void => { + const layoutHasChanged = this._listMetrics.notifyCellLayout({ + cellIndex, cellKey, - index, - length, - offset, - ); + layout: e.nativeEvent.layout, + orientation: this._orientation(), + }); + if (layoutHasChanged) { - this._scheduleCellsToRenderUpdate(); + // TODO: We have not yet received parent content length, meaning we do not + // yet have up to date offsets in RTL. This means layout queries done + // when scheduling a new batch may not yet be correct. This is corrected + // when we schedule again in response to `onContentSizeChange`. + const {horizontal, rtl} = this._orientation(); + this._scheduleCellsToRenderUpdate({ + allowImmediateExecution: !(horizontal && rtl), + }); } this._triggerRemeasureForChildListsInCell(cellKey); @@ -1340,10 +1332,13 @@ class VirtualizedList extends StateSafePureComponent { this._scrollRef.measureLayout( this.context.getOutermostParentListRef().getScrollRef(), (x, y, width, height) => { - this._offsetFromParentVirtualizedList = this._selectOffset({x, y}); - this._scrollMetrics.contentLength = this._selectLength({ - width, - height, + this._offsetFromParentVirtualizedList = this._scrollOffset( + {x, y}, + {width, height}, + ); + this._listMetrics.notifyListContentLayout({ + layout: {width, height}, + orientation: this._orientation(), }); const scrollMetrics = this._convertParentScrollMetrics( this.context.getScrollMetrics(), @@ -1415,7 +1410,7 @@ class VirtualizedList extends StateSafePureComponent { _renderDebugOverlay() { const normalize = this._scrollMetrics.visibleLength / - (this._scrollMetrics.contentLength || 1); + (this._listMetrics.getContentLength() || 1); const framesInLayout = []; const itemCount = this.props.getItemCount(this.props.data); for (let ii = 0; ii < itemCount; ii++) { @@ -1487,14 +1482,38 @@ class VirtualizedList extends StateSafePureComponent { : metrics.width; } - _selectOffset( + _scrollOffset( metrics: $ReadOnly<{ x: number, y: number, ... }>, + contentSize: $ReadOnly<{ + width: number, + height: number, + ... + }>, ): number { - return !horizontalOrDefault(this.props.horizontal) ? metrics.y : metrics.x; + const {horizontal, rtl} = this._orientation(); + + if (horizontal) { + // Convert x coordinate to flow-relative offset. iOS scroll event + // coordinates are flipped already. + if (rtl && Platform.OS !== 'ios') { + return this._selectLength(contentSize) - metrics.x; + } else { + return metrics.x; + } + } else { + return metrics.y; + } + } + + _orientation(): ListOrientation { + return { + horizontal: horizontalOrDefault(this.props.horizontal), + rtl: I18nManager.isRTL, + }; } _maybeCallOnEdgeReached() { @@ -1512,9 +1531,10 @@ class VirtualizedList extends StateSafePureComponent { return; } - const {contentLength, visibleLength, offset} = this._scrollMetrics; + const {visibleLength, offset} = this._scrollMetrics; let distanceFromStart = offset; - let distanceFromEnd = contentLength - visibleLength - offset; + let distanceFromEnd = + this._listMetrics.getContentLength() - visibleLength - offset; // Especially when oERT is zero it's necessary to 'floor' very small distance values to be 0 // since debouncing causes us to not fire this event for every single "pixel" we scroll and can thus @@ -1548,9 +1568,9 @@ class VirtualizedList extends StateSafePureComponent { onEndReached && this.state.cellsAroundViewport.last === getItemCount(data) - 1 && isWithinEndThreshold && - this._scrollMetrics.contentLength !== this._sentEndForContentLength + this._listMetrics.getContentLength() !== this._sentEndForContentLength ) { - this._sentEndForContentLength = this._scrollMetrics.contentLength; + this._sentEndForContentLength = this._listMetrics.getContentLength(); onEndReached({distanceFromEnd}); } @@ -1561,9 +1581,9 @@ class VirtualizedList extends StateSafePureComponent { onStartReached != null && this.state.cellsAroundViewport.first === 0 && isWithinStartThreshold && - this._scrollMetrics.contentLength !== this._sentStartForContentLength + this._listMetrics.getContentLength() !== this._sentStartForContentLength ) { - this._sentStartForContentLength = this._scrollMetrics.contentLength; + this._sentStartForContentLength = this._listMetrics.getContentLength(); onStartReached({distanceFromStart}); } @@ -1605,7 +1625,10 @@ class VirtualizedList extends StateSafePureComponent { if (this.props.onContentSizeChange) { this.props.onContentSizeChange(width, height); } - this._scrollMetrics.contentLength = this._selectLength({height, width}); + this._listMetrics.notifyListContentLayout({ + layout: {width, height}, + orientation: this._orientation(), + }); this._scheduleCellsToRenderUpdate(); this._maybeCallOnEdgeReached(); }; @@ -1623,7 +1646,7 @@ class VirtualizedList extends StateSafePureComponent { // Child's visible length is the same as its parent's const visibleLength = metrics.visibleLength; const dOffset = offset - this._scrollMetrics.offset; - const contentLength = this._scrollMetrics.contentLength; + const contentLength = this._listMetrics.getContentLength(); return { visibleLength, @@ -1643,11 +1666,14 @@ class VirtualizedList extends StateSafePureComponent { const timestamp = e.timeStamp; let visibleLength = this._selectLength(e.nativeEvent.layoutMeasurement); let contentLength = this._selectLength(e.nativeEvent.contentSize); - let offset = this._selectOffset(e.nativeEvent.contentOffset); + let offset = this._scrollOffset( + e.nativeEvent.contentOffset, + e.nativeEvent.contentSize, + ); let dOffset = offset - this._scrollMetrics.offset; if (this._isNestedWithSameOrientation()) { - if (this._scrollMetrics.contentLength === 0) { + if (this._listMetrics.getContentLength() === 0) { // Ignore scroll events until onLayout has been called and we // know our offset from our offset from our parent return; @@ -1682,7 +1708,6 @@ class VirtualizedList extends StateSafePureComponent { // For invalid negative values (w/ RTL), set this to 1. const zoomScale = e.nativeEvent.zoomScale < 0 ? 1 : e.nativeEvent.zoomScale; this._scrollMetrics = { - contentLength, dt, dOffset, offset, @@ -1708,7 +1733,34 @@ class VirtualizedList extends StateSafePureComponent { this._scheduleCellsToRenderUpdate(); }; - _scheduleCellsToRenderUpdate() { + _scheduleCellsToRenderUpdate(opts?: {allowImmediateExecution?: boolean}) { + const allowImmediateExecution = opts?.allowImmediateExecution ?? true; + + // 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), + // each time through attempting to render more (limited by maxToRenderPerBatch), + // starving the renderer from actually laying out the objects and computing _averageCellLength. + // 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 + ) { + this._hiPriInProgress = true; + // Don't worry about interactions when scrolling quickly; focus on filling content as fast + // as possible. + this._updateCellsToRenderBatcher.dispose({abort: true}); + this._updateCellsToRender(); + return; + } else { + this._updateCellsToRenderBatcher.schedule(); + } + } + + _shouldRenderWithPriority(): boolean { const {first, last} = this.state.cellsAroundViewport; const {offset, visibleLength, velocity} = this._scrollMetrics; const itemCount = this.props.getItemCount(this.props.data); @@ -1743,27 +1795,8 @@ class VirtualizedList extends StateSafePureComponent { distBottom < getScrollingThreshold(onEndReachedThreshold, visibleLength)); } - // 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), - // each time through attempting to render more (limited by maxToRenderPerBatch), - // starving the renderer from actually laying out the objects and computing _averageCellLength. - // If this is triggered in an `componentDidUpdate` followed by a hiPri cellToRenderUpdate - // We shouldn't do another hipri cellToRenderUpdate - if ( - hiPri && - (this._listMetrics.getAverageCellLength() || this.props.getItemLayout) && - !this._hiPriInProgress - ) { - this._hiPriInProgress = true; - // Don't worry about interactions when scrolling quickly; focus on filling content as fast - // as possible. - this._updateCellsToRenderBatcher.dispose({abort: true}); - this._updateCellsToRender(); - return; - } else { - this._updateCellsToRenderBatcher.schedule(); - } + + return hiPri; } _onScrollBeginDrag = (e: ScrollEvent): void => { @@ -1781,9 +1814,9 @@ class VirtualizedList extends StateSafePureComponent { this._nestedChildLists.forEach(childList => { childList._onScrollEndDrag(e); }); - const {velocity} = e.nativeEvent; + const {velocity, contentSize} = e.nativeEvent; if (velocity) { - this._scrollMetrics.velocity = this._selectOffset(velocity); + this._scrollMetrics.velocity = this._scrollOffset(velocity, contentSize); } this._computeBlankness(); this.props.onScrollEndDrag && this.props.onScrollEndDrag(e);