Skip to content

Commit

Permalink
Gracefully handle out-of-bounds initialScrollIndex
Browse files Browse the repository at this point in the history 
Summary:
Changelog:
[General][Fixed] - Gracefully handle out-of-bounds initialScrollIndex

Reviewed By: rshest

Differential Revision: D43672964

fbshipit-source-id: dbd9007c538015fc586e573d268135b7557dc908
  • Loading branch information
@NickGerleman @facebook-github-bot
NickGerleman authored and facebook-github-bot committed Mar 3, 2023
1 parent 31b4550 commit aab9df3
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 46 deletions.
115 changes: 74 additions & 41 deletions packages/virtualized-lists/Lists/VirtualizedList.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import {
keyExtractor as defaultKeyExtractor,
} from './VirtualizeUtils';
import invariant from 'invariant';
import nullthrows from 'nullthrows';
import * as React from 'react';

export type {RenderItemProps, RenderItemType, Separators};
Expand Down Expand Up @@ -420,21 +421,7 @@ class VirtualizedList extends StateSafePureComponent<Props, State> {

constructor(props: Props) {
super(props);
invariant(
// $FlowFixMe[prop-missing]
!props.onScroll || !props.onScroll.__isNative,
'Components based on VirtualizedList must be wrapped with Animated.createAnimatedComponent ' +
'to support native onScroll events with useNativeDriver',
);
invariant(
windowSizeOrDefault(props.windowSize) > 0,
'VirtualizedList: The windowSize prop must be present and set to a value greater than 0.',
);

invariant(
props.getItemCount,
'VirtualizedList: The "getItemCount" prop must be provided',
);
this.checkProps(props);

this._fillRateHelper = new FillRateHelper(this._getFrameMetrics);
this._updateCellsToRenderBatcher = new Batchinator(
Expand All @@ -459,11 +446,6 @@ class VirtualizedList extends StateSafePureComponent<Props, State> {
}
}

invariant(
!this.context,
'Unexpectedly saw VirtualizedListContext available in ctor',
);

const initialRenderRegion = VirtualizedList._initialRenderRegion(props);

this.state = {
Expand All @@ -472,6 +454,53 @@ class VirtualizedList extends StateSafePureComponent<Props, State> {
};
}

checkProps(props: Props) {
const {onScroll, windowSize, getItemCount, data, initialScrollIndex} =
props;

invariant(
// $FlowFixMe[prop-missing]
!onScroll || !onScroll.__isNative,
'Components based on VirtualizedList must be wrapped with Animated.createAnimatedComponent ' +
'to support native onScroll events with useNativeDriver',
);
invariant(
windowSizeOrDefault(windowSize) > 0,
'VirtualizedList: The windowSize prop must be present and set to a value greater than 0.',
);

invariant(
getItemCount,
'VirtualizedList: The "getItemCount" prop must be provided',
);

const itemCount = getItemCount(data);

if (
initialScrollIndex != null &&
(initialScrollIndex < 0 ||
(itemCount > 0 && initialScrollIndex >= itemCount)) &&
!this._hasWarned.initialScrollIndex
) {
console.warn(
`initialScrollIndex "${initialScrollIndex}" is not valid (list has ${itemCount} items)`,
);
this._hasWarned.initialScrollIndex = true;
}

if (__DEV__ && !this._hasWarned.flexWrap) {
// $FlowFixMe[underconstrained-implicit-instantiation]
const flatStyles = StyleSheet.flatten(this.props.contentContainerStyle);
if (flatStyles != null && flatStyles.flexWrap === 'wrap') {
console.warn(
'`flexWrap: `wrap`` is not supported with the `VirtualizedList` components.' +
'Consider using `numColumns` with `FlatList` instead.',
);
this._hasWarned.flexWrap = true;
}
}
}

static _createRenderMask(
props: Props,
cellsAroundViewport: {first: number, last: number},
Expand Down Expand Up @@ -518,15 +547,21 @@ class VirtualizedList extends StateSafePureComponent<Props, State> {

static _initialRenderRegion(props: Props): {first: number, last: number} {
const itemCount = props.getItemCount(props.data);
const scrollIndex = Math.floor(Math.max(0, props.initialScrollIndex ?? 0));

const firstCellIndex = Math.max(
0,
Math.min(itemCount - 1, Math.floor(props.initialScrollIndex ?? 0)),
);

const lastCellIndex =
Math.min(
itemCount,
firstCellIndex + initialNumToRenderOrDefault(props.initialNumToRender),
) - 1;

return {
first: scrollIndex,
last:
Math.min(
itemCount,
scrollIndex + initialNumToRenderOrDefault(props.initialNumToRender),
) - 1,
first: firstCellIndex,
last: lastCellIndex,
};
}

Expand Down Expand Up @@ -807,16 +842,7 @@ class VirtualizedList extends StateSafePureComponent<Props, State> {
}

render(): React.Node {
if (__DEV__) {
// $FlowFixMe[underconstrained-implicit-instantiation]
const flatStyles = StyleSheet.flatten(this.props.contentContainerStyle);
if (flatStyles != null && flatStyles.flexWrap === 'wrap') {
console.warn(
'`flexWrap: `wrap`` is not supported with the `VirtualizedList` components.' +
'Consider using `numColumns` with `FlatList` instead.',
);
}
}
this.checkProps(this.props);
const {ListEmptyComponent, ListFooterComponent, ListHeaderComponent} =
this.props;
const {data, horizontal} = this.props;
Expand Down Expand Up @@ -1507,10 +1533,17 @@ class VirtualizedList extends StateSafePureComponent<Props, State> {
!this._hasTriggeredInitialScrollToIndex
) {
if (this.props.contentOffset == null) {
this.scrollToIndex({
animated: false,
index: this.props.initialScrollIndex,
});
if (
this.props.initialScrollIndex <
this.props.getItemCount(this.props.data)
) {
this.scrollToIndex({
animated: false,
index: nullthrows(this.props.initialScrollIndex),
});
} else {
this.scrollToEnd({animated: false});
}
}
this._hasTriggeredInitialScrollToIndex = true;
}
Expand Down
55 changes: 52 additions & 3 deletions packages/virtualized-lists/Lists/__tests__/VirtualizedList-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -825,10 +825,12 @@ it('unmounts sticky headers moved below viewport', () => {
expect(component).toMatchSnapshot();
});

it('gracefully handles negaitve initialScrollIndex', () => {
it('gracefully handles negative initialScrollIndex', () => {
const items = generateItems(10);
const ITEM_HEIGHT = 10;

const mockWarn = jest.spyOn(console, 'warn').mockImplementation(() => {});

const component = ReactTestRenderer.create(
<VirtualizedList
initialScrollIndex={-1}
Expand All @@ -838,9 +840,56 @@ it('gracefully handles negaitve initialScrollIndex', () => {
/>,
);

// Existing code assumes we handle this in some way. Do something reasonable
// here.
expect(mockWarn).toHaveBeenCalledTimes(1);

ReactTestRenderer.act(() => {
simulateLayout(component, {
viewport: {width: 10, height: 50},
content: {width: 10, height: 100},
});
performAllBatches();
});

expect(component).toMatchSnapshot();
expect(mockWarn).toHaveBeenCalledTimes(1);
mockWarn.mockRestore();
});

it('gracefully handles too large initialScrollIndex', () => {
const items = generateItems(10);
const ITEM_HEIGHT = 10;

const listRef = React.createRef();

const mockWarn = jest.spyOn(console, 'warn').mockImplementation(() => {});

const component = ReactTestRenderer.create(
<VirtualizedList
ref={listRef}
initialScrollIndex={15}
initialNumToRender={4}
{...baseItemProps(items)}
{...fixedHeightItemLayoutProps(ITEM_HEIGHT)}
/>,
);

expect(mockWarn).toHaveBeenCalledTimes(1);
listRef.current.scrollToEnd = jest.fn();

ReactTestRenderer.act(() => {
simulateLayout(component, {
viewport: {width: 10, height: 50},
content: {width: 10, height: 100},
});
performAllBatches();
});

expect(mockWarn).toHaveBeenCalledTimes(1);
mockWarn.mockRestore();

expect(listRef.current.scrollToEnd).toHaveBeenLastCalledWith({
animated: false,
});
});

it('renders offset cells in initial render when initialScrollIndex set', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/virtualized-lists/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3036,7 +3036,7 @@ exports[`expands render area by maxToRenderPerBatch on tick 1`] = `
</RCTScrollView>
`;

exports[`gracefully handles negaitve initialScrollIndex 1`] = `
exports[`gracefully handles negative initialScrollIndex 1`] = `
<RCTScrollView
data={
Array [
Expand Down
3 changes: 2 additions & 1 deletion packages/virtualized-lists/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
},
"license": "MIT",
"dependencies": {
"invariant": "^2.2.4"
"invariant": "^2.2.4",
"nullthrows": "^1.1.1"
},
"devDependencies": {
"react-test-renderer": "18.2.0"
Expand Down

0 comments on commit aab9df3

Please sign in to comment.