App: [HOLD #147480][$40,000] React Native ScrollView bug: maintainVisibleContentPosition

All of the PRs have now landed upstream!

https://github.com/facebook/react-native-website/pull/3721 merged. Closing this out! Great job @janicduplessis

Upstream PR is in review: https://github.com/facebook/react-native/pull/35993

PR was just merged into the bidirectional-scrolling feature branch!

Updated https://github.com/Expensify/react-native/pull/8 to use the first item key as indicator of items being added, the bidirectional paging example now works properly when scrolling in both directions.

After further investigation my initial thought are following:

• maintainVisibleContentPosition won’t behave correctly when used under FlatList because self->_contentView.subviews.count here https://github.com/facebook/react-native/blob/main/React/Views/ScrollView/RCTScrollView.m#L910 will have an amount of items rendered by virtualizer, not the actual array length of data prop passed to the FlatList
• Therefore the first entirely visible view that we are trying to calculate here https://github.com/facebook/react-native/blob/main/React/Views/ScrollView/RCTScrollView.m#L926 is always wrong when prepending items.
• It incorrectly calculates the position of the first visible item, resulting position to be 0px. Which becomes 0 - 0px here https://github.com/facebook/react-native/blob/main/React/Views/ScrollView/RCTScrollView.m#L954
• I’m most positive that’s because items in view are shifted, due to Flatlist is getting items by index https://github.com/facebook/react-native/blob/main/Libraries/Lists/VirtualizedList.js#L1814

I will give more detailed analysis with the proposal soon.

Hey everyone, I am a developer at Steuerbot and me and our frontend team are struggling with this issue for a long time, because our app needs to hold the position even if new elements were added at the top of the chat list. For that reason we hoped an official android support for the maintainVisibleContentPosition property would be developed, but this never happened 😕

So we tried many different other approaches: Clientside-only hacks, patching RN-Core and other things. Finally we came up with a solution, which fixes the issue on our side 🚀 What did we do? We are providing our own version of the Android RN ScrollView implementation (overriding functions from the RN class). This way we can correct the scroll position based on information the JS side provides us (this works also during a “fling animation” by restarting the fling with corrected position!).

I sharing the link to our developed RN module with you; maybe this will work for your project too 😃 https://github.com/steuerbot/react-native-bidirectional-flatlist

Just give it a try. You can use it as a FlatList-Replacement. We are looking forward to your feedback!

This is a draft, I will ping reviewers once completed

FlatList

• Middle block called Virtualized is the only part that will move here, therefore method that calculates first frame is always wrong due to content shift caused by virtualization
• Note that FlatList example in RNTester, has the HeaderComponent and FooterComponent, HeaderComponent is always rendered as the first subview inside ScrollView self->_contentView.subviews[0]!

<ScrollView>
{head.map(renderItem)} → https://github.com/Expensify/react-native/blob/master/Libraries/Lists/VirtualizedList.js#L910-L926
{virtualized.map(renderItem)} → https://github.com/Expensify/react-native/blob/master/Libraries/Lists/VirtualizedList.js#L962-L969
{tail.map(renderItem)} → https://github.com/Expensify/react-native/blob/master/Libraries/Lists/VirtualizedList.js#L991-L998
</ScrollView>

Output of NSLog(@"Subviews count: %lu", self->_contentView.subviews.count); at prependUIBlock:

• render cycles of virtualizer items
• only 100 items are passed into FlatList (500-600)
• onStartReached is turned off
• onEndReached is turned off

2022-03-22 18:25:23.468276+0500 MVCPExample[66740:6211501] Subviews count: 13
2022-03-22 18:25:23.468414+0500 MVCPExample[66740:6211501] Subviews count: 13
2022-03-22 18:25:23.468551+0500 MVCPExample[66740:6211501] Subviews count: 13
2022-03-22 18:25:23.468674+0500 MVCPExample[66740:6211501] Subviews count: 13
2022-03-22 18:25:23.468751+0500 MVCPExample[66740:6211501] Subviews count: 13
2022-03-22 18:25:23.469204+0500 MVCPExample[66740:6211501] Subviews count: 13
2022-03-22 18:25:23.474187+0500 MVCPExample[66740:6211501] Subviews count: 13
2022-03-22 18:25:23.545705+0500 MVCPExample[66740:6211501] Subviews count: 13
2022-03-22 18:25:23.627980+0500 MVCPExample[66740:6211501] Subviews count: 23
2022-03-22 18:25:23.707817+0500 MVCPExample[66740:6211501] Subviews count: 33
2022-03-22 18:25:23.788797+0500 MVCPExample[66740:6211501] Subviews count: 43
2022-03-22 18:25:23.855413+0500 MVCPExample[66740:6211501] Subviews count: 53
2022-03-22 18:25:23.921377+0500 MVCPExample[66740:6211501] Subviews count: 63
2022-03-22 18:25:23.988802+0500 MVCPExample[66740:6211501] Subviews count: 73
2022-03-22 18:25:24.064840+0500 MVCPExample[66740:6211501] Subviews count: 83
2022-03-22 18:25:24.129288+0500 MVCPExample[66740:6211501] Subviews count: 93
2022-03-22 18:25:24.187206+0500 MVCPExample[66740:6211501] Subviews count: 95

Proposal One

https://github.com/Expensify/react-native/blob/master/React/Views/ScrollView/RCTScrollView.m#L922

// add self->_scrollView.contentOffset.y > 0 

CGFloat y = self->_scrollView.contentOffset.y + bottomInset;
hasNewView = subview.frame.origin.y > y && y > 0;

This will cause https://github.com/Expensify/react-native/blob/master/React/Views/ScrollView/RCTScrollView.m#L925

ii == self->_contentView.subviews.count - 1

To grab last subview until virtualizer has completed it’s job with rendering actual items (around 4 component render cycles for the sample of 100 items)

Proposal Two

Save the content height in prependUIBlock

[manager
      prependUIBlock:^(__unused RCTUIManager *uiManager, __unused NSDictionary<NSNumber *, UIView *> *viewRegistry) {
self->_prevContentSizeHeight = self->_scrollView.contentSize.height;

https://github.com/Expensify/react-native/blob/master/React/Views/ScrollView/RCTScrollView.m#L954

CGFloat deltaY = self->_scrollView.contentSize.height - self->_prevContentSizeHeight;

https://user-images.githubusercontent.com/4882133/159498505-1a80f709-3d84-4d74-b989-30dae1c598e3.mp4

Adjust content height when new items are prepended, minIndexForVisible is omited here

Proposal three

Pass range returned by computeWindowedRenderLimits at VirtualizedList into ScrollView to get frame of the actual first visible item.

Interestingly, https://github.com/facebook/react-native/pull/33184 listed in the reproduction steps can be fixed by removing the autoScrollToTopThreshold parameter entirely. While this might be a hint at how to solve this, it does not seem to be a viable solution for us. Even without an autoScrollToTopThreshold parameter, the same problem occurs in https://github.com/Expensify/react-native/pull/7. It’s unclear why removing the autoScrollToTopThreshold parameter fixes the problem, but setting a value of 0 does not. 🤔

The answer is <= comparison operator:

// RCTScrollView.m

/**
 * This is why removing the autoScrollToTopThreshold parameter entirely works
 */
if (autoscrollThreshold != nil) {
  /**
   * having autoscrollThreshold === 0; will evaluate to 0 <= 0
   */
  if (self->_scrollView.contentOffset.y - deltaY <= [autoscrollThreshold integerValue]) {
    [self scrollToOffset:CGPointMake(self->_scrollView.contentOffset.x, 0) animated:YES];
  }
}

Where fix for this would be:

if (autoscrollThreshold != nil && autoscrollThreshold != 0) {

I’m assuming no contributors are due compensation via Upwork. Please comment if you disagree.

Well I guess that was quick 😃

It’s just a documentation update, but I think the last PR we want to see merged before this is 100% complete is https://github.com/facebook/react-native-website/pull/3721

Since this is a big change, I can help review and test fixes if needed 🙂

Thanks! We’ve actually completed a bidirectional pagination solution ourselves in our React Native fork, available on npm. @janicduplessis implemented maintainVisibleContentPosition directly in our React Native fork, and we’re hoping to open PRs to upstream these features soon.

Putting this on hold while we work. It seems like this particular bug is fixed, but we’ll keep this open until the fix is verified in our app.

Hey @mallenexpensify!

I’m currently working with @roryabraham on https://github.com/Expensify/react-native/pull/8. Addressing minor review comments.

What would be involved in tracking visibility? I have a couple questions that make me think using the first item in the list might not be ideal:

Visibility tracking should be relatively straightforward as VirtualizedList has context for scroll metrics and all item positions. However I’m not sure it is needed. Basically the goal of this piece of code is to determine if the native implementation of maintainVisibleContentPosition will be triggered, and by how many our items have been moved by new items added before our virtualization window (first state).

Detecting items added at the start of the list is good enough for bidirectional pagination, but of course will break if items are added at other positions.

1. I don’t think this matters as VirtualizedList has knowledge of all items, even those not rendered. I can test to confirm, but I did play with that value a bit and seemed fine.

2. Right now we don’t consider this value, I think its main use is to ignore changes to header views, which VirtualizedList separate from data, so as far as I know it is fine to ignore, at least when using first item to detect changes.

Ideally we wouldn’t rely on this assumption. I agree it’s unlikely that content will be added in the middle of a list, but is it possible that deletion of content might simulate this case?

Good point, I haven’t tested how maintainVisibleContentPosition behaves when removing items, we just need to make sure the JS implementation does the same thing so if content will be adjusted by the native implementation we also adjust the VirtualizedList window. In practice this probably won’t break unless deleting a large amount of items at once that would cause a shift that moves visible content outside the virtualization window.

Right now when using the first item, a delete in the middle would cause no changes to the virtualization window since the first item did not change, which I think might be correct.

Again, not sure if it will be a problem for us or not, but we should call this out in documentation updates. Also, can we succinctly define what is meant by “stable”?

This could be added to documentation for maintainVisibleContentPosition of VirtualizedList. By stable I mean keys are id based and not index based. This is already the preferred way to handle keys in VirtualizedList, but we could detect it and show a warning if used incorrectly.

So for example when adding new items key should be a unique id for the item like:

Good, id based key:

// Good we can detect where new item was added.
[{key: 'a'}, {key: 'b'}] -> [{key: 'c'}, {key: 'a'}, {key: 'b'}]

Bad, index based key:

// Looks like item was added at the end, even if it was added at the start since all key shifted.
[{key: 1}, {key: 2}] -> [{key: 1}, {key: 2}, {key: 3}]

@roryabraham can you please review Azim’s proposal above?

I tried to understand your proposal @azimgd but I didn’t completely get the good sense of what are you trying to achieve by all of those changes? I know this is a draft but thought of sharing the review with you.

I am new to native code but I can understand the logic. It would be helpful for me if you can also tell us what are those variables you referenced in your proposals. You reviewed the native code and understand it but they might not be known to others (like me).

so e.g. self->_contentView => renderedContent subview.frame => Item Dimensions.

Proposal one, two, etc will be better tagged as steps.

My understanding is that you are trying to get the correct first visible item on the content view when new items are prepended. Please let me know if I am wrong. Thanks. I will try to learn and understand the changes as much as possible.

Hey @fabriziobertoglio1987,

Hope are you doing great. Sorry to ping you without notice. But I have seen your work on RN issues and really fond of your efforts to make RN better. I thought you might be interested in this job.

Thanks.

Thanks @zoontek for the detailed analysis:

Should scroll position be preserved if the item at minIndexForVisible is not visible?

I would think so, yes. Let’s assume we had this maintainVisibleContentPosition configuration:

maintainVisibleContenPosition = {
    minIndexForVisible: 0,
    autoScrollToTopThreshold: 25,
};

I imagine that if you are in the middle of a list (index 0 is not visible, and we are more than 25px away from the start), then new content is prepended to the list, we would maintain scroll position.

Again: there is no real changes here (except to improve items removal handling)

Agree. Looks like those are good improvements but they don’t address the issue we’re facing.

but given my explanations (mainly about autoscrollToTopThreshold behaviour), does the initial issue described in the ticket really exists?

So there was definitely a flaw in the original post for the issue. The minimal reproduction we provided (using ScrollView alone) does not actually reflect the issue, but was a red herring that @azimgd explained the cause for in this comment.

We can remove autoscrollToTopThreshold entirely and the issue still exists. In order to reproduce it, try building the rn-tester app off of this branch and look at the FlatList example. By adjusting the page size you can see that the issue is only consistently reproducible when the new content has enough height, and is likely caused by some race condition.

Just want to make sure you run npx patch-package

Aha, I missed that. Ops.

Let me know if you have any more questions and please keep in mind that we are still in the proposal stage

Yup. I agree. But as you asked me to test your repo and I am trying to do that.

@parasharrajat Sorry, I changed visibility to public.

We live! https://www.upwork.com/jobs/~01620b5ab4dee0a9f0