Update VirtualizedList's content layout in onScroll

[j-piasecki]

Summary:

#38529 introduced some changes to the internal state of VirtualizedList, replacing _scrollMetrics.contentLength with _listMetrics.getContentLength() and _listMetrics.notifyListContentLayout(...).

I'm not sure whether it was done on purpose or by accident, but setting the contentLength in the onScroll handler here was removed instead of being updated to call the new method.

I'm not sure whether it has any impact on apps, but testing relying on testing-library and its fireEvent.scroll do not work anymore due to this change.

cc. @NickGerleman as they are the author of the aforementioned PR

Changelog:

[GENERAL] [FIXED] - Fix VirualizedList not updating its content length on scroll events

Test Plan:

I found the problem during upgrade of the Expensify app to React Native 0.73 when some of the tests started failing. I guess it should be easy to tell whether the original change was deliberate or accidental, but if it's necessary I can prepare a reproduction with a simple failing test.

[NickGerleman]

The reason length is taken from onContentSizeChange (scrollview layout event) is around more guaranteed event ordering relative to child layout updates, which ends up mattering when trying to support horizontal RTL lists. I’m not certain the size from scroll events, reading the same layout, would be right.

A lot of VirtualizedList functionality relies on timers, layout events on the list, and layout events on the children. It can all be simulated in a Jest environment, but I have wondered before if it might be better to automatically mock it to a ScrollView or something (though bits like viewability callbacks cannot work without layout events).

I think otherwise, they way to more reliably test this would look like:

1. After the list is mounted, it should receive layout events (top down) for the scrollview and list items
2. Timers need to be run after scrolling
3. Layout events need to be fired for everything the list decides to render at that moment

That’s admittedly not super trivial to do.

Otherwise you might get a half functioning list, like rendering the first ten items (or initialNumToRender), which I suspect is what a lot of snapshotting relies on.

[NickGerleman]

Let me ask around a bit about this. Interacting with VirtualizedList in Jest environment doesn’t seem like too crazy a scenario.

[NickGerleman]

One more nuance, that I wonder if you are hitting, is that list doesn’t need to observe child layout events if getItemLayout is provided. I think in that case, you’d get closer to a working list without mocking the child layout events before.

[NickGerleman]

I thought about this a little bit more, and think it would be a bad idea to mock with a lighter version not requiring events, since the event values do really matter, and we can virtualize a non-finite data source.

@j-piasecki is this a place where the tests could either mock more of the environment, or rely on a different mechanism for testing?

[j-piasecki]

@j-piasecki is this a place where the tests could either mock more of the environment, or rely on a different mechanism for testing?

To be honest, I'm not sure as I'm not very familiar with the test setup nor with the used library.

That said I just tried and it seems like invoking contentSizeChange with mocked values instead of just scroll also fixes the tests. I guess this is a better approach than the one in the PR?

[NickGerleman]

That said I just tried and it seems like invoking contentSizeChange with mocked values instead of just scroll also fixes the tests. I guess this is a better approach than the one in the PR?

That seems like a good solution to me

[j-piasecki]

Thanks! I'll close this issue and open a new one in react-native-testing-library to ask if it's possible to incorporate it there somehow.