Fix VirtualizedList with maintainVisibleContentPosition

[janicduplessis]

Summary

maintainVisibleContentPosition is broken when using virtualization and the new content pushes visible content outside its "window". This can be reproduced in the example from this diff. When using a large page size it will always push visible content outside of the list "window" which will cause currently visible views to be unmounted so the implementation of maintainVisibleContentPosition can't adjust the content inset since the visible views no longer exist.

The first illustration shows the working case, when the new content doesn't push visible content outside the window. The red box represents the window, all views outside the box are not mounted, which means the native implementation of maintainVisibleContentPosition has no way to know it exists. In that case the first visible view is #2, after new content is added #2 is still inside the window so there's not problem adjusting content offset to maintain position. As you can see Step 1 and 3 result in the same position for all initial views.

The second illustation shows the broken case, when new content is added and pushes the first visible view outside the window. As you can see in step 2 the view #2 is no longer rendered so there's no way to maintain its position.

Illustration 1

Illustration 2

To fix maintainVisibleContentPosition when using VirtualizedList we need to make sure the visible items stay rendered when new items are added at the start of the list.

In order to do that we need to do the following:

• Detect new items that will cause content to be adjusted
• Add cells to render mask so that previously visible cells stay rendered
• Ignore certain updates while scroll metrics are invalid

Detect new items that will cause content to be adjusted

The goal here is to know that scroll position will be updated natively by the maintainVisibleContentPosition implementation. The problem is that the native code uses layout heuristics which are not easily available to JS to do so. In order to approximate the native heuristic we can assume that if new items are added at the start of the list, it will cause maintainVisibleContentPosition to be triggered. This simplifies JS logic a lot as we don't have to track visible items. In the worst case if for some reason our JS heuristic is wrong, it will cause extra cells to be rendered until the next scroll event, or content position will not be maintained (what happens all the time currently). I think this is a good compromise between complexity and accuracy.

We need to find how many items have been added before the first one. To do that we save the key of the first item in state firstItemKey. When data changes we can find the index of firstItemKey in the new data and that will be the amount we need to adjust the window state by.

Note that this means that keys need to be stable, and using index won't work.

Add cells to render mask so that previously visible cells stay rendered

Once we have the adjusted number we can save this in a new state value maintainVisibleContentPositionAdjustment and add the adjusted cells to the render mask.

This state is then cleared when we receive updated scroll metrics, once the native implementation is done adding the new items and adjusting the content offset.

This value is also only set when maintainVisibleContentPosition is set so this makes sure this maintains the currently behavior when that prop is not set.

Ignore certain updates while scroll metrics are invalid

While the maintainVisibleContentPositionAdjustment state is set we know that the current scroll metrics are invalid since they will be updated in the native ScrollView implementation. In that case we want to prevent certain code from running.

One example is onStartReached that will be called incorrectly while we are waiting for updated scroll metrics.

Changelog

[General] [Fixed] - Fix VirtualizedList with maintainVisibleContentPosition

Test Plan

Added bidirectional paging to RN tester FlatList example. Note that for this to work RN tester need to be run using old architecture on iOS, to use new architecture it requires #35319

Using debug mode we can see that virtualization is still working properly, and content position is being maintained.

Screen.Recording.2022-04-13.at.21.09.11.mov

[janicduplessis]

Working on adding some tests

[github-actions]

	Fails
🚫	📋 Missing Changelog - Please add a Changelog to your PR description. See Changelog format

	Warnings
⚠️	packages/rn-tester/js/examples/SectionList/SectionList-scrollable.js#L285 - packages/rn-tester/js/examples/SectionList/SectionList-scrollable.js line 285 – Do not define components during render. React will see a new component type on every render and destroy the entire subtree’s DOM nodes and state (https://reactjs.org/docs/reconciliation.html#elements-of-different-types). Instead, move this component definition out of the parent component “SectionList_scrollable” and pass data as props. If you want to allow component creation in props, set allowAsProps option to true. (react/no-unstable-nested-components)
⚠️	packages/rn-tester/js/examples/SectionList/SectionList-scrollable.js#L289 - packages/rn-tester/js/examples/SectionList/SectionList-scrollable.js line 289 – Do not define components during render. React will see a new component type on every render and destroy the entire subtree’s DOM nodes and state (https://reactjs.org/docs/reconciliation.html#elements-of-different-types). Instead, move this component definition out of the parent component “SectionList_scrollable” and pass data as props. If you want to allow component creation in props, set allowAsProps option to true. (react/no-unstable-nested-components)

Generated by 🚫 dangerJS against fb7b4da

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,623,754	+950
android	hermes	armeabi-v7a	7,937,110	+952
android	hermes	x86	9,110,397	+944
android	hermes	x86_64	8,965,426	+952
android	jsc	arm64-v8a	9,187,874	+820
android	jsc	armeabi-v7a	8,378,750	+820
android	jsc	x86	9,245,924	+813
android	jsc	x86_64	9,504,580	+806

Base commit: cff4bc8
Branch: main

[NickGerleman]

Very excited by this.

cc @rozele as well who was looking at scroll anchoring.

This should solve a huge class of issues with relayout pushing items around during realization. But it also actually would allow us to implement APIs like scrollToEnd() in a way which avoids flutter as well (since as of 0.71+ we can realize the end cell ahead of time and use as an anchor if we scroll down before other content is rendered.).

Going to try to dedicate some time to read through all of this. Have a feeling this will be one of those changes that requires a cup of coffee and some quiet time to grep through 😛.

[miafoo]

I've been testing this change for a while and for the most part it works great. However, sometimes when adding a lot of items at once it still seems to lose the anchor. I've recorded a short video to demonstrate the issue.

In the video it works great the first three times I reset the app and add items to the list, but then fails and loses anchor after more resets when adding items.

Each time I press the red square, 50 items are prepended to the list, each with a height of 200. The purple badge in the right corner shows the item count. After each reset it starts at 4 items. When adding fewer items at a time (eg. 5) it never seems to lose its anchor.

Simulator.Screen.Recording.-.iPhone.14.-.2023-02-23.at.12.22.44.mp4

[janicduplessis]

@mikfoo Could you provide me with the code you used to reproduce this problem?

[miafoo]

@mikfoo Could you provide me with the code you used to reproduce this problem?

@janicduplessis: Sure. If you create a new RN project and apply the patch manually to VirtualizedList.js in node_modules and prepend data to a simple FlatList the issue occurs, at least for me. Here's a quick example you can use: https://gist.github.com/mikfoo/289e8c54e04f0a34f090ce10e3603d90

With the code in the gist I can reliably reproduce the issue over and over. It usually happens on first "Add" and also when you have scrolled to the top of the list (when contentOffset.y <= 0).

Another small thing I noticed (not sure if it's directly related to this PR) is that when you add new items too fast to the list (eg. pressing the "Add" button a couple of times in fast succession) then it throws an error:

[rozele]

I think this will also work for the inverted case on mobile platforms, since that just uses transform tricks to make the start of the list render at the bottom, but it might be worth validating. For platforms like Web and Windows, we actually need to change the way inversion works because the transform tricks don't play nice with keyboard nav, mouse wheel events, etc. Anecdotally, on Windows, we have a prototype for this (microsoft/react-native-windows#8440).

It feels a bit like the hacks to get things to work in VirtualizedList are piling up 😅. I wonder how many of these could be resolved with a native virtualized list component (render mask for retaining focused islands, maintainVisibleContentPosition virtualization, native inversion for relevant platforms)...

[rozele]

I also wonder if you could build on @NickGerleman's render mask functionality to implement this. E.g., add a new event when a scroll anchor is selected and force an "island" of materialized cells around the selected anchor. Not sure it would be feasible, given the virtualization logic runs from onLayout and onScroll, and you don't select an anchor for maintainVisibleContentPosition until ScrollView layout (which would be too late to prevent the bug).

I've also long wondered if there's a better alternative to maintainVisibleContentPosition. Here are some ideas I've had in the past:

1. Switch the default behavior of React Native to anchor-by-default as on Web (this wouldn't fix this particular bug, but still perhaps a good idea).
2. Add an overflowAnchor prop to View to enable / disable anchoring for specific children of ScrollView.
3. (Fabric-only) Add a synchronous event to ScrollView, like onAnchorRequested to allow app code to specify the anchor just before layout is committed on the UI thread (there's a corollary to this on Windows with the ScrollViewer::AnchorRequested event).

If we switched to "anchor-by-default" behavior and assumed that an anchor was selected on every commit, we could be certain an event would fire with the selected anchor in VirtualizedList 🤷‍♂️.

This is all just food for thought. I have no opposition to fixing bugs for code that should already work 😅

[miafoo]

@janicduplessis I played with it again last night, and noticed that if you add items while it's batch rendering then it always loses focus if you're at the top of the list at least. If you wait adding items to the list until it's completely done rendering, then the focus is kept (except on the initial render, it always loses focus on that).

[janicduplessis]

@mikfoo Thanks for testing this! I was able to reproduce the issue with the code you provided and it should now be fixed with 3379aec. In the example I used to test I added items during scrolling with onStart/EndReached which would always trigger high priority updates which don't have that problem. To fix it I can cancel pending batches (which are now invalid because of the updated maintainVisibleContentPositionAdjustment) when the adjustment changes.

I can also reproduce the issue where the first time items are added the adjustment doesn't work. Will have a look at this next.

[janicduplessis]

@rozele Thanks for having a look. I agree with you that a better long term solution would be great to explore, but in the meantime this is an acceptable fix.

I am pretty sure inverted works, but will double check.

I also wonder if you could build on @NickGerleman's render mask functionality to implement this. E.g., add a new event when a scroll anchor is selected and force an "island" of materialized cells around the selected anchor.

Unless I'm wrong about what you mean here, this is already what I do here. Basically what I do is detect that the scrollview will be anchored and create an island around previously visible cells.

I think the idea of having anchor events would be great in this case, since currently I have to approximate that a scroll adjustment will happen, since that logic is implemented in native code using layout info not available to JS.

[miafoo]

@janicduplessis Awesome! I just tested the changes and they seem to work great! There's a new small issue where when I load a large list, it sometimes doesn't start at the top of the list on the initial load (it's a few items off), but I haven't been able to reliably reproduce it yet. Edit: this doesn't seem to happen anymore, at least I can't reproduce it so it might have been my app that was bugged.

[NickGerleman]

I was able to take a look through the code, and description. I have some implementation comments, but before I leave those, I wanted to check to see if I understand the overall problem, and the viability of an alternative method of doing this.

From the flow, here, it seems like:

1. User adds data before the viewport
2. VirtualizedList responds to the new data, by rendering an index-based window around the new data
3. maintainVisibleContentPosition doesn't save us and adjust scroll position because the old item isn't still rendered

What I am wondering, could we attack #2 by changing how VirtualizedList handles data changes. I.e. fix existing logic in getDerivedStateFromProps() to align by key instead of index when determining the next render range on prop change.

That would avoid needing to keep an arbitrarily large range of cells rendered, because the VirtualizedList decided to move its own point of reference in response to items shifting index.

[NickGerleman]

A comment pending on this implementation, that I think would effect that one as well, is that we usually want to avoid the full scan of data like this change is currently doing. The model is meant to allow data to be backed by something expensive/lazy, so doing a full search throughout all data defeats that.

Though I am not sure the current API provides a way to diff before/after efficiently, without searching for keys. But we could also maybe optimize that search (e.g. look around the last index first).

[janicduplessis]

@NickGerleman Thanks for having a look, if I understand your point correctly you suggest adjusting the cellsAroundViewport here instead of rendering an extra window based on maintainVisibleContentPositionAdjustment? I had something similar in my original implementation in the old VirtualizedList that didn't support multiple render windows, but I figured it would be better to leverage multiple windows when doing the adjustment so in case the adjustment does not happen in native (could be possible since we approximate that the adjustment will happen by checking item changes while native uses visible items layout). If the adjustment does not happen by having both old and new window rendered it would avoid rendering blank until the user scrolls and right items are rendered again. Since the larger range will only be rendered for a very short amount of time I figured that this should be ok.

Though I am not sure the current API provides a way to diff before/after efficiently, without searching for keys. But we could also maybe optimize that search (e.g. look around the last index first).

Yea the best I can think of is to use the change in data size and start the lookup from there. So if items are added at the start the old first item will be at index newItemCount - oldItemCount - 1 so we would get it at first iteration. However if items are added at the end of the list then we would need to scan more. We could try to check index 0 and newItemCount - oldItemCount - 1 first and fallback to full scan. Note that this case only happens when mvcp is set so it doesn't affect performance of previous VirtualizedList usage.

[NickGerleman]

I had something similar in my original implementation in the old VirtualizedList that didn't support multiple render windows, but I figured it would be better to leverage multiple windows when doing the adjustment so in case the adjustment does not happen in native (could be possible since we approximate that the adjustment will happen by checking item changes while native uses visible items layout). If the adjustment does not happen by having both old and new window rendered it would avoid rendering blank until the user scrolls and right items are rendered again.

My recommendation is that we shift the old window, to a single new location, instead of rendering two windows. Then we explicitly rely on maintainVisibleContentPosition to keep the scroll position aligned.

On data change, VirtualizedList's strategy of rendering cellsAroundViewport matching the previous index, instead of same key, means potentially very expensive renders that are not where we want. Say, our window was items 20-40 of a 100 item list, and we erase items 0-20, we end up rendering 20 new items. With FlatList defaults, this means we end up rendering 21 viewports worth of new items in the case of no alignment. And many more than we would allow to be rendered in a single batch normally (maxToRenderPerBatch).

The existing index-based alignment may produce a sroll position with is off from the previous data, but even off from the new data at the desired index. Since the previous spacer above the content was using most recent cell measurements, which may have changed. So we need maintainVisibleContentPosition for doing something sane I think.

If we instead realign cellsAroundViewport based on previous keys, instead of previous index, we can behave much more cheaply, and the logic is all confined to the invalidation step, instead of what we have right now which hooks deeply into different facets of the list. Though we do then rely on native maintainVisibleContentPosition, to correct scrolling position based on still present anchor.

Yea the best I can think of is to use the change in data size and start the lookup from there. So if items are added at the start the old first item will be at index newItemCount - oldItemCount - 1 so we would get it at first iteration. However if items are added at the end of the list then we would need to scan more. We could try to check index 0 and newItemCount - oldItemCount - 1 first and fallback to full scan.

For the reasons above, I think we may want to do this realignment in more general cases. Thinking more about this, it should be possible to do efficiently, and with a bounds in common cases. Just slower compared to us having information ahead of time.

1. Add all previous cellsAroundViewport cellKeys to a set (currently cached in _frames), then iterate through data at the new window (matched to index) to create an mapping from old index to new index.
   a. This bounds to a scan of windowSize, where we already may do a full scan of that length (though usually on hot data)
   b. We can do things like short circuit if we find the previous anchor cell we were using
2. Realign new cellsAroundViewport based on:
   a. An explicit maintainVisibleContentPosition anchor if mappable
   b. Some other deterministically chosen anchor from the set
   c. Some fallback if there is no alignment at all within the index based. Rare case of full scan of new data looking for any match, or maybe scroll to top or something like that.

That kinda veers on having maintainVisibleContentPosition enabled on invalidation, since our logic is explicitly allowing cellsAroundViewport to drift in layout space to a greater degree.

[NickGerleman]

Though also FYI @rozele @acoates-ms that would add a dependency on maintainVisibleContentPosition, which I think will soon only be a concern for Windows.

[janicduplessis]

@NickGerleman Hopefully I got your feedback right, as I'm not familiar with all of VirtualizedList code.

I changed it so that instead of creating a 2nd window with the mvcp adjustment I instead update the cellsAroundViewport.

For the reasons above, I think we may want to do this realignment in more general cases. Thinking more about this, it should be possible to do efficiently, and with a bounds in common cases. Just slower compared to us having information ahead of time.

I'm not sure I understand here what you mean by doing a realignment in more general cases, also the part about creating a mapping from old to new index 2.a An explicit maintainVisibleContentPosition anchor if mappable. Using the current technique to detect changes that would trigger native maintainVisibleContentPosition, we only rely on the first item in the list, which might not be in the currently rendered window, which is why I'm not sure I understand why a mapping using the window would help in that case.

I added a hint to _findItemIndexWithKey so we can find the item index in O(1) when adding items at the start of the list instead of the number of new items iterations. Unless the first item is removed from the list or content added at the start and another position at the same time (so our length hint is off) we don't need to do any scanning of data.

[NickGerleman]

I'm not sure I understand here what you mean by doing a realignment in more general cases

The idea is that we adjust cellsAroundViewport on data change using a key-based anchor, instead of the index based anchor, even when we are not explicitly using maintainVisibleContentPosition. This avoids some special casing, but should also fix pre-existing jank around data changes. It may be more involved than we want though (I will take a look through this PR).

[NickGerleman]

I agree with your earlier comment about this being fine for only when maintainVisibleContentPosition is enabled. If we wanted to generalize, it would be best to bound to the size of the previous window.

[NickGerleman]

Suggested change

	return VirtualizedList._keyExtractor(item, 0, props);
	return VirtualizedList._keyExtractor(item, index, props);