Skip to content

Commit

Permalink
Fix bounds calculation with initialScrollIndex
Browse files Browse the repository at this point in the history
Summary:
Sometimes this._scrollMetrics.offset is 0 even after initial scroll is triggered, because the offset is updated only upon _onScroll, which may not have been called in time for the next computation of the render limits. Thus, there is a window of time where computeWindowedRenderLimits calculates undesired render limits as it uses the offset. This results in 2 extra rerenders of the VirtualizedList if an initial scroll offset is applied, as the render limits shifts from the expected bounds (calculated using initialScrollIndex), to the 0 offset bounds (calculated using computeWindowedRenderLimits due to offset = 0), back to the expected bounds (onScroll triggers recalculation of render limits via _updateCellsToRender).

This issue was introduced in https://www.internalfb.com/diff/D35503114 (c5c1798)

We cannot rely on this._hasDoneInitialScroll to indicate that scrolling *actually* finished (specifically, that _onScroll was called). Instead, we want to recalculate the windowed render limits if any of the following hold:
- initialScrollIndex is undefined or is 0
- initialScrollIndex > 0 AND scrolling is complete
- initialScrollIndex > 0 AND the end of the list is visible (this handles the case where the list is shorter than the visible area) <- this is the case that https://www.internalfb.com/diff/D35503114 (c5c1798) attempted to address

Changelog:
[Internal][Fixed] - Fix issue where VirtualizedList rerenders multiple times and flickers when initialScrollIndex is set

Reviewed By: JoshuaGross

Differential Revision: D36328891

fbshipit-source-id: aba26aa06b24f6976657dd1e9f95bb666f60d9a6
  • Loading branch information
genkikondo authored and facebook-github-bot committed May 12, 2022
1 parent 2e49332 commit 73ad651
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 7 deletions.
21 changes: 16 additions & 5 deletions Libraries/Lists/VirtualizedList.js
Original file line number Diff line number Diff line change
Expand Up @@ -1202,7 +1202,8 @@ class VirtualizedList extends React.PureComponent<Props, State> {
},
} = {};
_footerLength = 0;
_hasDoneInitialScroll = false;
// Used for preventing scrollToIndex from being called multiple times for initialScrollIndex
_hasTriggeredInitialScrollToIndex = false;
_hasInteracted = false;
_hasMore = false;
_hasWarned: {[string]: boolean} = {};
Expand Down Expand Up @@ -1539,15 +1540,15 @@ class VirtualizedList extends React.PureComponent<Props, State> {
height > 0 &&
this.props.initialScrollIndex != null &&
this.props.initialScrollIndex > 0 &&
!this._hasDoneInitialScroll
!this._hasTriggeredInitialScrollToIndex
) {
if (this.props.contentOffset == null) {
this.scrollToIndex({
animated: false,
index: this.props.initialScrollIndex,
});
}
this._hasDoneInitialScroll = true;
this._hasTriggeredInitialScrollToIndex = true;
}
if (this.props.onContentSizeChange) {
this.props.onContentSizeChange(width, height);
Expand Down Expand Up @@ -1758,6 +1759,7 @@ class VirtualizedList extends React.PureComponent<Props, State> {
| $TEMPORARY$object<{first: number, last: number}>
);
const {contentLength, offset, visibleLength} = this._scrollMetrics;
const distanceFromEnd = contentLength - visibleLength - offset;
if (!isVirtualizationDisabled) {
// If we run this with bogus data, we'll force-render window {first: 0, last: 0},
// and wipe out the initialNumToRender rendered elements.
Expand All @@ -1768,7 +1770,17 @@ class VirtualizedList extends React.PureComponent<Props, State> {
// we'll wipe out the initialNumToRender rendered elements starting at initialScrollIndex.
// So let's wait until we've scrolled the view to the right place. And until then,
// we will trust the initialScrollIndex suggestion.
if (!this.props.initialScrollIndex || this._hasDoneInitialScroll) {

// Thus, we want to recalculate the windowed render limits if any of the following hold:
// - initialScrollIndex is undefined or is 0
// - initialScrollIndex > 0 AND scrolling is complete
// - initialScrollIndex > 0 AND the end of the list is visible (this handles the case
// where the list is shorter than the visible area)
if (
!this.props.initialScrollIndex ||
this._scrollMetrics.offset ||
Math.abs(distanceFromEnd) < Number.EPSILON
) {
newState = computeWindowedRenderLimits(
this.props.data,
this.props.getItemCount,
Expand All @@ -1781,7 +1793,6 @@ class VirtualizedList extends React.PureComponent<Props, State> {
}
}
} else {
const distanceFromEnd = contentLength - visibleLength - offset;
const renderAhead =
distanceFromEnd < onEndReachedThreshold * visibleLength
? maxToRenderPerBatchOrDefault(this.props.maxToRenderPerBatch)
Expand Down
5 changes: 3 additions & 2 deletions Libraries/Lists/__tests__/VirtualizedList-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -885,6 +885,7 @@ it('adjusts render area with non-zero initialScrollIndex', () => {
viewport: {width: 10, height: 50},
content: {width: 10, height: 200},
});
simulateScroll(component, {x: 0, y: 10}); // simulate scroll offset for initialScrollIndex

performAllBatches();
});
Expand Down Expand Up @@ -914,8 +915,8 @@ it('renders new items when data is updated with non-zero initialScrollIndex', ()

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

0 comments on commit 73ad651

Please sign in to comment.