Fix VirtualizedList with initialScrollIndex not rendering all elements when data is updated

[AntoineDoubovetzky]

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

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

[pull-bot]

Messages

📖

📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message. CATEGORY may be: General iOS Android JavaScript Internal (for changes that do not need to be called out in the release notes) TYPE may be: Added, for new features. Changed, for changes in existing functionality. Deprecated, for soon-to-be removed features. Removed, for now removed features. Fixed, for any bug fixes. Security, in case of vulnerabilities. MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 5eb2a0a

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: a129595
Branch: main

[facebook-github-bot]

@yungsters has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[ryancat]

Thanks for making this change. I think it makes sense to me.

One thing to mention here -- I tried to reproduce this with your example, but got error at VirtualizedList.js where the this._highestMeasuredFrameIndex value is still zero. I believe that only got updated when onCellLayout is triggered, which is not happening when I load the playground code.

Could you check if that's the case for you? I wonder why that is happening on my side. That might indicate an issue by itself (separate from this one).

[AntoineDoubovetzky]

@ryancat Sure, I'll check if that's also the case for me and I'll try to give you an update soon.

[AntoineDoubovetzky]

@ryancat You were right, I get the same error using the example in the issue. There is no error when adding the getItemLayout or the onScrollToIndexFailed prop as mentionned in the warning.

The correct code example is the following:

/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @format
 * @flow
 */

import {Text, FlatList, SafeAreaView, View} from 'react-native';
import * as React from 'react';

const getData = length => {
  return new Array(length).fill(0).map((_, index) => ({
    title: `item ${index}`,
  }));
};

const ITEM_HEIGHT = 70;

const RNTesterApp = (): React.Node => {
  const renderItem = React.useCallback(
    (item: {
      item: {
        title: string,
      },
    }) => {
      return (
        <View style={{height: ITEM_HEIGHT, backgroundColor: '#00ff00'}}>
          <Text>{item.item.title}</Text>
        </View>
      );
    },
    [],
  );

  const [data, setData] = React.useState(getData(2));

  React.useEffect(() => {
    setTimeout(() => {
      setData(getData(4));
    }, 2000);
  }, []);

  return (
    <SafeAreaView>
      <FlatList
        data={data}
        renderItem={renderItem}
        initialScrollIndex={1}
        getItemLayout={(items, index) => ({
          length: ITEM_HEIGHT,
          offset: ITEM_HEIGHT * index,
          index,
        })}
      />
    </SafeAreaView>
  );
};

export default RNTesterApp;

I think there is no issue with this warning:
When index > this._highestMeasuredFrameIndex, we won't be able to get the frame from this._frames so we'll need to get it from getItemLayout:

react-native/Libraries/Lists/VirtualizedList.js

Lines 1864 to 1869 in a223874

	let frame = item && this._frames[this._keyExtractor(item, index)];
	if (!frame || frame.index !== index) {
	if (getItemLayout) {
	frame = getItemLayout(data, index);
	}
	}

[facebook-github-bot]

@yungsters has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[AntoineDoubovetzky]

I rebased onto master to resolve conflicts.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,774,154	-41
android	hermes	armeabi-v7a	7,180,039	-44
android	hermes	x86	8,083,250	-38
android	hermes	x86_64	8,063,191	-33
android	jsc	arm64-v8a	9,647,295	-7
android	jsc	armeabi-v7a	8,421,568	-11
android	jsc	x86	9,596,584	-8
android	jsc	x86_64	10,193,934	-6

Base commit: a129595
Branch: main

[AntoineDoubovetzky]

(Rebased again after commit was reverted)

[facebook-github-bot]

@yungsters has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @AntoineDoubovetzky in c5c1798.

^{When will my fix make it into a release? | Upcoming Releases}

[yepMad]

Is this still a problem or something? I have a horizontal FlatList, each item has the width of the screen (carousel style) and with active pagination. When I declare a value for initialScrollIndex all next items are only rendered when scrolling/paging is done