Fix VirtualizedList with initialScrollIndex not rendering all element…

…s when data is updated (#33558) Summary: Fixes #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: #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
facebook · Apr 20, 2022 · c5c1798 · c5c1798
1 parent 2c52131
commit c5c1798
Show file tree

Hide file tree

Showing 3 changed files with 96 additions and 150 deletions.
diff --git a/Libraries/Lists/VirtualizedList.js b/Libraries/Lists/VirtualizedList.js
@@ -1754,7 +1754,7 @@ 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._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
@@ -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(
+      <VirtualizedList
+        initialNumToRender={5}
+        initialScrollIndex={1}
+        windowSize={10}
+        maxToRenderPerBatch={10}
+        {...baseItemProps(newItems)}
+        {...fixedHeightItemLayoutProps(ITEM_HEIGHT)}
+      />,
+    );
+  });
+
+  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
@@ -1600,7 +1600,7 @@ exports[`VirtualizedList warns if both renderItem or ListItemComponent are speci
 </RCTScrollView>
 `;
 
-exports[`adjusts render area with non-zero initialScrollIndex after scrolled 1`] = `
+exports[`adjusts render area with non-zero initialScrollIndex 1`] = `
 <RCTScrollView
   data={
     Array [
@@ -2207,144 +2207,6 @@ exports[`does not adjust render area until content area layed out 1`] = `
 </RCTScrollView>
 `;
 
-exports[`does not adjust render area with non-zero initialScrollIndex until scrolled 1`] = `
-<RCTScrollView
-  data={
-    Array [
-      Object {
-        "key": 0,
-      },
-      Object {
-        "key": 1,
-      },
-      Object {
-        "key": 2,
-      },
-      Object {
-        "key": 3,
-      },
-      Object {
-        "key": 4,
-      },
-      Object {
-        "key": 5,
-      },
-      Object {
-        "key": 6,
-      },
-      Object {
-        "key": 7,
-      },
-      Object {
-        "key": 8,
-      },
-      Object {
-        "key": 9,
-      },
-      Object {
-        "key": 10,
-      },
-      Object {
-        "key": 11,
-      },
-      Object {
-        "key": 12,
-      },
-      Object {
-        "key": 13,
-      },
-      Object {
-        "key": 14,
-      },
-      Object {
-        "key": 15,
-      },
-      Object {
-        "key": 16,
-      },
-      Object {
-        "key": 17,
-      },
-      Object {
-        "key": 18,
-      },
-      Object {
-        "key": 19,
-      },
-    ]
-  }
-  getItem={[Function]}
-  getItemCount={[Function]}
-  getItemLayout={[Function]}
-  initialNumToRender={5}
-  initialScrollIndex={1}
-  maxToRenderPerBatch={10}
-  onContentSizeChange={[Function]}
-  onLayout={[Function]}
-  onMomentumScrollBegin={[Function]}
-  onMomentumScrollEnd={[Function]}
-  onScroll={[Function]}
-  onScrollBeginDrag={[Function]}
-  onScrollEndDrag={[Function]}
-  renderItem={[Function]}
-  scrollEventThrottle={50}
-  stickyHeaderIndices={Array []}
-  windowSize={10}
->
-  <View>
-    <View
-      style={
-        Object {
-          "height": 10,
-        }
-      }
-    />
-    <View
-      style={null}
-    >
-      <MockCellItem
-        value={1}
-      />
-    </View>
-    <View
-      style={null}
-    >
-      <MockCellItem
-        value={2}
-      />
-    </View>
-    <View
-      style={null}
-    >
-      <MockCellItem
-        value={3}
-      />
-    </View>
-    <View
-      style={null}
-    >
-      <MockCellItem
-        value={4}
-      />
-    </View>
-    <View
-      style={null}
-    >
-      <MockCellItem
-        value={5}
-      />
-    </View>
-    <View
-      style={
-        Object {
-          "height": 140,
-        }
-      }
-    />
-  </View>
-</RCTScrollView>
-`;
-
 exports[`does not over-render when there is less than initialNumToRender cells 1`] = `
 <RCTScrollView
   data={
@@ -3250,6 +3112,75 @@ exports[`renders items before initialScrollIndex on first batch tick when virtua
 </RCTScrollView>
 `;
 
+exports[`renders new items when data is updated with non-zero initialScrollIndex 1`] = `
+<RCTScrollView
+  data={
+    Array [
+      Object {
+        "key": 0,
+      },
+      Object {
+        "key": 1,
+      },
+      Object {
+        "key": 2,
+      },
+      Object {
+        "key": 3,
+      },
+    ]
+  }
+  getItem={[Function]}
+  getItemCount={[Function]}
+  getItemLayout={[Function]}
+  initialNumToRender={5}
+  initialScrollIndex={1}
+  maxToRenderPerBatch={10}
+  onContentSizeChange={[Function]}
+  onLayout={[Function]}
+  onMomentumScrollBegin={[Function]}
+  onMomentumScrollEnd={[Function]}
+  onScroll={[Function]}
+  onScrollBeginDrag={[Function]}
+  onScrollEndDrag={[Function]}
+  renderItem={[Function]}
+  scrollEventThrottle={50}
+  stickyHeaderIndices={Array []}
+  windowSize={10}
+>
+  <View>
+    <View
+      style={null}
+    >
+      <MockCellItem
+        value={0}
+      />
+    </View>
+    <View
+      style={null}
+    >
+      <MockCellItem
+        value={1}
+      />
+    </View>
+    <View
+      style={null}
+    >
+      <MockCellItem
+        value={2}
+      />
+    </View>
+    <View
+      style={null}
+    >
+      <MockCellItem
+        value={3}
+      />
+    </View>
+  </View>
+</RCTScrollView>
+`;
+
 exports[`renders no spacers up to initialScrollIndex on first render when virtualization disabled 1`] = `
 <RCTScrollView
   data={