Skip to content

Commit d84d153

Browse files
Abbondanzofacebook-github-bot
authored andcommitted
Fix viewability for zero-sized lists
Summary: VirtualizedList viewability only considered the scroll-axis viewport length. A horizontal list with zero height could therefore report items as viewable. Record the cross-axis length in the same scroll metrics snapshot and require both viewport dimensions to be non-zero for viewability. Changelog: [General][Fixed] - Prevent zero-sized lists from reporting viewable items Differential Revision: D116356139
1 parent 4338af0 commit d84d153

2 files changed

Lines changed: 81 additions & 1 deletion

File tree

packages/virtualized-lists/Lists/VirtualizedList.js

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1250,6 +1250,7 @@ class VirtualizedList extends StateSafePureComponent<
12501250
_pendingViewabilityUpdate: boolean = false;
12511251
_prevParentOffset: number = 0;
12521252
_scrollMetrics: {
1253+
crossAxisLength: number,
12531254
dOffset: number,
12541255
dt: number,
12551256
offset: number,
@@ -1258,6 +1259,7 @@ class VirtualizedList extends StateSafePureComponent<
12581259
visibleLength: number,
12591260
zoomScale: number,
12601261
} = {
1262+
crossAxisLength: 0,
12611263
dOffset: 0,
12621264
dt: 10,
12631265
offset: 0,
@@ -1381,6 +1383,7 @@ class VirtualizedList extends StateSafePureComponent<
13811383
this.context.getOutermostParentListRef().getScrollRef(),
13821384
(x, y, width, height) => {
13831385
this._offsetFromParentVirtualizedList = this._selectOffset({x, y});
1386+
const crossAxisLength = this._selectCrossAxisLength({width, height});
13841387
this._listMetrics.notifyListContentLayout({
13851388
layout: {width, height},
13861389
orientation: this._orientation(),
@@ -1390,10 +1393,12 @@ class VirtualizedList extends StateSafePureComponent<
13901393
);
13911394

13921395
const metricsChanged =
1396+
this._scrollMetrics.crossAxisLength !== crossAxisLength ||
13931397
this._scrollMetrics.visibleLength !== scrollMetrics.visibleLength ||
13941398
this._scrollMetrics.offset !== scrollMetrics.offset;
13951399

13961400
if (metricsChanged) {
1401+
this._scrollMetrics.crossAxisLength = crossAxisLength;
13971402
this._scrollMetrics.visibleLength = scrollMetrics.visibleLength;
13981403
this._scrollMetrics.offset = scrollMetrics.offset;
13991404

@@ -1420,6 +1425,9 @@ class VirtualizedList extends StateSafePureComponent<
14201425
}
14211426

14221427
_onLayout = (e: LayoutChangeEvent) => {
1428+
this._scrollMetrics.crossAxisLength = this._selectCrossAxisLength(
1429+
e.nativeEvent.layout,
1430+
);
14231431
if (this._isNestedWithSameOrientation()) {
14241432
// Need to adjust our scroll metrics to be relative to our containing
14251433
// VirtualizedList before we can make claims about list item viewability
@@ -1527,6 +1535,18 @@ class VirtualizedList extends StateSafePureComponent<
15271535
: metrics.width;
15281536
}
15291537

1538+
_selectCrossAxisLength(
1539+
metrics: Readonly<{
1540+
height: number,
1541+
width: number,
1542+
...
1543+
}>,
1544+
): number {
1545+
return !horizontalOrDefault(this.props.horizontal)
1546+
? metrics.width
1547+
: metrics.height;
1548+
}
1549+
15301550
_selectOffset({x, y}: Readonly<{x: number, y: number, ...}>): number {
15311551
return this._orientation().horizontal ? x : y;
15321552
}
@@ -1710,6 +1730,9 @@ class VirtualizedList extends StateSafePureComponent<
17101730
this.props.onScroll(e);
17111731
}
17121732
const timestamp = e.timeStamp;
1733+
let crossAxisLength = this._selectCrossAxisLength(
1734+
e.nativeEvent.layoutMeasurement,
1735+
);
17131736
let visibleLength = this._selectLength(e.nativeEvent.layoutMeasurement);
17141737
let contentLength = this._selectLength(e.nativeEvent.contentSize);
17151738
let offset = this._offsetFromScrollEvent(e);
@@ -1726,6 +1749,7 @@ class VirtualizedList extends StateSafePureComponent<
17261749
visibleLength,
17271750
offset,
17281751
}));
1752+
crossAxisLength = this._scrollMetrics.crossAxisLength;
17291753
}
17301754

17311755
const dt = this._scrollMetrics.timestamp
@@ -1751,6 +1775,7 @@ class VirtualizedList extends StateSafePureComponent<
17511775
// For invalid negative values (w/ RTL), set this to 1.
17521776
const zoomScale = e.nativeEvent.zoomScale < 0 ? 1 : e.nativeEvent.zoomScale;
17531777
this._scrollMetrics = {
1778+
crossAxisLength,
17541779
dt,
17551780
dOffset,
17561781
offset,
@@ -2032,11 +2057,15 @@ class VirtualizedList extends StateSafePureComponent<
20322057
if (this.state.pendingScrollUpdateCount > 0) {
20332058
return;
20342059
}
2060+
const visibleLength =
2061+
this._scrollMetrics.crossAxisLength > 0
2062+
? this._scrollMetrics.visibleLength
2063+
: 0;
20352064
this._viewabilityTuples.forEach(tuple => {
20362065
tuple.viewabilityHelper.onUpdate(
20372066
props,
20382067
this._scrollMetrics.offset,
2039-
this._scrollMetrics.visibleLength,
2068+
visibleLength,
20402069
this._listMetrics,
20412070
this._createViewToken,
20422071
tuple.onViewableItemsChanged,

packages/virtualized-lists/Lists/__tests__/VirtualizedList-test.js

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -468,6 +468,7 @@ describe('VirtualizedList', () => {
468468

469469
const instance = component.getInstance();
470470

471+
simulateViewportLayout(component, {width: 300, height: 600});
471472
instance._onScrollBeginDrag({nativeEvent});
472473
instance._onScroll({
473474
timeStamp: 1000,
@@ -504,6 +505,56 @@ describe('VirtualizedList', () => {
504505
);
505506
});
506507

508+
it('does not report viewable items when scroll metrics have an empty cross-axis viewport', async () => {
509+
const data = [{key: 'i1'}, {key: 'i2'}, {key: 'i3'}];
510+
const onViewableItemsChanged = jest.fn();
511+
let component;
512+
513+
await act(() => {
514+
component = create(
515+
<VirtualizedList
516+
data={data}
517+
getItem={(items, index) => items[index]}
518+
getItemCount={items => items.length}
519+
getItemLayout={(items, index) => ({
520+
index,
521+
length: 100,
522+
offset: index * 100,
523+
})}
524+
horizontal={true}
525+
onViewableItemsChanged={onViewableItemsChanged}
526+
renderItem={({item}) => <item value={item.key} />}
527+
/>,
528+
);
529+
});
530+
531+
component.getInstance()._onScroll({
532+
timeStamp: 1000,
533+
nativeEvent: {
534+
contentInset: {bottom: 0, left: 0, right: 0, top: 0},
535+
contentOffset: {x: 0, y: 0},
536+
contentSize: {width: 300, height: 0},
537+
layoutMeasurement: {width: 300, height: 0},
538+
zoomScale: 1,
539+
},
540+
});
541+
542+
expect(onViewableItemsChanged).not.toHaveBeenCalled();
543+
544+
component.getInstance()._onScroll({
545+
timeStamp: 2000,
546+
nativeEvent: {
547+
contentInset: {bottom: 0, left: 0, right: 0, top: 0},
548+
contentOffset: {x: 0, y: 0},
549+
contentSize: {width: 300, height: 100},
550+
layoutMeasurement: {width: 300, height: 100},
551+
zoomScale: 1,
552+
},
553+
});
554+
555+
expect(onViewableItemsChanged).toHaveBeenCalledTimes(1);
556+
});
557+
507558
it('getScrollRef for case where it returns a ScrollView', async () => {
508559
const listRef = createRef(null);
509560

0 commit comments

Comments
 (0)