Skip to content

Commit

Permalink
Fix invariant violation when using viewability callbacks with horizon…
Browse files Browse the repository at this point in the history
…tal RTL FlatList on Paper (#39335)

Summary:
Pull Request resolved: #39335

In RTL we must have scrollview content length in order to resolve cell metrics. This means that on Paper, where layout events are bottom up, we cannot immediately calculate viewability in response to cell metric changes, as we may not yet have an accurate length of laid out list content.

This change makes us defer calculation of viewability changes in this case via `setTimeout()`, to run in a single batch after the next layout events are fired.

 ---

We need container dimensions to resolve the right-edge relative child directions. It is tricky to do this at a guaranteed right time with onLayout model on Paper.
When we are laying out children, the first layout on Paper looks like:

1. Child is laid out

2. Container is laid out

However, we will never see container onLayout if a child layout does not change the dimensions of the parent container. This will be the common case of subsequent child layouts, where the spacer size was accurate.

I.e. we may or may not ever see content laid out, but can only rely on having both offsets be up to date if we trigger calculation after the container layout would have happened. This is not an issue on Fabric where layout events are fired top-down, or for the most common cases of VirtualizedList, where we run calculations in idle batches that will happen after layout is set, but ends up causing problems for two scenarios I didn't originally account for:

We may recalculate cells to render immediately instead scheduling a later update if the list thinks blanking is immediate (high priority render). This means we cannot do an immediate update in response to cell layout, but we can in response to events batched after layout events are all dispatched, or in worst case delay in Paper RTL.
We do not batch/schedule viewability calculations in response to cell layout in the same wasy as we do for calculating cells to render, but do need them to trigger recalculation.

The way less hacky, but much more invasive solution, that would simplify a lot of this, would be to include parentWidth and parentHeight in onLayout events for Paper (and Fabric for consistency), so that we don't need to rely on event ordering, or sometimes not firing. I thought this would be too much at first, if we didn't have other use-cases, but am more and more tempted to tear down a lot of what we have here to do that instead, since this is not going to be able to rely on useLayoutEffect or IntersectionObserver in today's VirtualizedList because it will need to support Paper for the forseeable future..

Changelog:
[General][Fixed] - Fix invariant violation when using viewability callbacks with horizontal RTL FlatList on Paper

Reviewed By: yungsters

Differential Revision: D49072963

fbshipit-source-id: accd33e2c50935bb67700d94820f6418f130fe08
  • Loading branch information
NickGerleman authored and facebook-github-bot committed Sep 12, 2023
1 parent 2a48c95 commit a2fb46e
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 8 deletions.
24 changes: 23 additions & 1 deletion packages/virtualized-lists/Lists/ListMetricsAggregator.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ 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
Expand Down Expand Up @@ -83,7 +85,7 @@ export default class ListMetricsAggregator {

// Fabric and Paper may call onLayout in different orders. We can tell which
// direction layout events happen on the first layout.
_onLayoutDirection: 'top-down' | 'bottom-up' = 'top-down';
_onLayoutDirection: LayoutEventDirection = 'top-down';

/**
* Notify the ListMetricsAggregator that a cell has been laid out.
Expand Down Expand Up @@ -284,6 +286,26 @@ 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.
Expand Down
33 changes: 26 additions & 7 deletions packages/virtualized-lists/Lists/VirtualizedList.js
Original file line number Diff line number Diff line change
Expand Up @@ -1211,6 +1211,7 @@ class VirtualizedList extends StateSafePureComponent<Props, State> {
_nestedChildLists: ChildListCollection<VirtualizedList> =
new ChildListCollection();
_offsetFromParentVirtualizedList: number = 0;
_pendingViewabilityUpdate: boolean = false;
_prevParentOffset: number = 0;
_scrollMetrics: {
dOffset: number,
Expand Down Expand Up @@ -1301,21 +1302,39 @@ class VirtualizedList extends StateSafePureComponent<Props, State> {
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) {
// 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),
allowImmediateExecution: !deferCellMetricCalculation,
});
}

this._triggerRemeasureForChildListsInCell(cellKey);

this._computeBlankness();
this._updateViewableItems(this.props, this.state.cellsAroundViewport);

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);
}
};

_onCellFocusCapture(cellKey: string) {
Expand Down

0 comments on commit a2fb46e

Please sign in to comment.