From c5c17985dae402725abb8a3a94ccedc515428711 Mon Sep 17 00:00:00 2001 From: Antoine Doubovetzky Date: Wed, 20 Apr 2022 10:26:50 -0700 Subject: [PATCH] Fix VirtualizedList with initialScrollIndex not rendering all elements when data is updated (#33558) Summary: Fixes https://github.com/facebook/react-native/issues/33529 (note that I reproduced the bug on iOS too). The bug comes from the fact that we were using `this._scrollMetrics.offset` to determine if the initial scroll was done. But sometimes it equals 0 even after the initial scroll is done, for example when the content does not fill the list. So I replaced it with `this._hasDoneInitialScroll`. I believe that `this._hasDoneInitialScroll` was not used in the first place because it was introduced later (3 years ago vs 5 years ago for the original code). The replacement correctly fixes the broken test case and the example given in the issue. Then I had to update two test cases (rename the first and remove the second), that shows explicitly the broken behavior: we have to simulate the initial scroll for the content to be adjusted, so when the content does not fill the view and the scroll cannot be executed, the content is not adjusted. ## Changelog [General] [Fix] - Fix VirtualizedList with initialScrollIndex not rendering all elements when data is updated Pull Request resolved: https://github.com/facebook/react-native/pull/33558 Test Plan: - I added a broken test case based on the issue - I tested with the RNTesterApp using the code example given in the issue Reviewed By: ryancat Differential Revision: D35503114 Pulled By: yungsters fbshipit-source-id: 67bb75d7cf1ebac0d59127d0d45afbaa3167dcf3 --- Libraries/Lists/VirtualizedList.js | 2 +- .../Lists/__tests__/VirtualizedList-test.js | 35 ++- .../VirtualizedList-test.js.snap | 209 ++++++------------ 3 files changed, 96 insertions(+), 150 deletions(-) diff --git a/Libraries/Lists/VirtualizedList.js b/Libraries/Lists/VirtualizedList.js index 0a02fb1ef1567f..77b69cd72ac976 100644 --- a/Libraries/Lists/VirtualizedList.js +++ b/Libraries/Lists/VirtualizedList.js @@ -1754,7 +1754,7 @@ class VirtualizedList extends React.PureComponent { // 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._scrollMetrics.offset) { + if (!this.props.initialScrollIndex || this._hasDoneInitialScroll) { newState = computeWindowedRenderLimits( this.props.data, this.props.getItemCount, diff --git a/Libraries/Lists/__tests__/VirtualizedList-test.js b/Libraries/Lists/__tests__/VirtualizedList-test.js index a7589992564214..9f902f180c7fbc 100644 --- a/Libraries/Lists/__tests__/VirtualizedList-test.js +++ b/Libraries/Lists/__tests__/VirtualizedList-test.js @@ -862,7 +862,7 @@ it('does not adjust render area until content area layed out', () => { expect(component).toMatchSnapshot(); }); -it('does not adjust render area with non-zero initialScrollIndex until scrolled', () => { +it('adjusts render area with non-zero initialScrollIndex', () => { const items = generateItems(20); const ITEM_HEIGHT = 10; @@ -885,18 +885,17 @@ it('does not adjust render area with non-zero initialScrollIndex until scrolled' viewport: {width: 10, height: 50}, content: {width: 10, height: 200}, }); + performAllBatches(); }); - // Layout information from before the time we scroll to initial index may not - // correspond to the area "initialScrollIndex" points to. Expect only the 5 - // initial items (starting at initialScrollIndex) to be rendered after - // processing all batch work, even though the windowSize allows for more. + // We should expand the render area after receiving a message indcating we + // arrived at initialScrollIndex. expect(component).toMatchSnapshot(); }); -it('adjusts render area with non-zero initialScrollIndex after scrolled', () => { - const items = generateItems(20); +it('renders new items when data is updated with non-zero initialScrollIndex', () => { + const items = generateItems(2); const ITEM_HEIGHT = 10; let component; @@ -918,13 +917,29 @@ it('adjusts render area with non-zero initialScrollIndex after scrolled', () => viewport: {width: 10, height: 50}, content: {width: 10, height: 200}, }); + performAllBatches(); + }); + + const newItems = generateItems(4); - simulateScroll(component, {x: 0, y: 10}); + ReactTestRenderer.act(() => { + component.update( + , + ); + }); + + ReactTestRenderer.act(() => { performAllBatches(); }); - // We should expand the render area after receiving a message indcating we - // arrived at initialScrollIndex. + // We expect all the items to be rendered expect(component).toMatchSnapshot(); }); diff --git a/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap b/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap index ad93703101b2ad..7c8cbb8edd6f5f 100644 --- a/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap +++ b/Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap @@ -1600,7 +1600,7 @@ exports[`VirtualizedList warns if both renderItem or ListItemComponent are speci `; -exports[`adjusts render area with non-zero initialScrollIndex after scrolled 1`] = ` +exports[`adjusts render area with non-zero initialScrollIndex 1`] = ` `; -exports[`does not adjust render area with non-zero initialScrollIndex until scrolled 1`] = ` - - - - - - - - - - - - - - - - - - - - - -`; - exports[`does not over-render when there is less than initialNumToRender cells 1`] = ` `; +exports[`renders new items when data is updated with non-zero initialScrollIndex 1`] = ` + + + + + + + + + + + + + + + + +`; + exports[`renders no spacers up to initialScrollIndex on first render when virtualization disabled 1`] = `