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

[roryabraham]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

Here is a very minimal reproduction of this bug in the rn-tester sample project, that's not dependent upon any new code in the React Native codebase. Run the rn-tester app, find the ScrollView example, and observe the following behavior:

If minIndexForVisible is 0, then the scroll position will be maintained iff:

1. The first item in the list is not visible, AND
2. The current contentOffset is at least y, where y is the height of the new content being prepended

Similarly, if minIndexForVisible is 1, then the scroll position will be maintained iff:

1. The second item in the list is not visible, AND
2. The current contentOffset is at least y, where y is the height of the new content being prepended

And so on... If either of those conditions are not met, the scroll position will not be maintained.

Expected Result:

As long as the list has enough content that it is scrollable, the contentOffset should be adjusted such that the scroll position is maintained when new items are added to the start or end of the list.

Actual Result:

The contentOffset is not adjusted, and the scroll position is not maintained.

Additional Details

Background

How does the maintainVisibleContentPosition prop work?

At a high level, when new UI elements are added to the start of the list, React Native will:

1. Measure the position of the first visible item before the new items are added
2. Add the new items to the view.
3. Measure the difference in position between the first visible item found in step 1 with its new position.
4. Increase the contentOffset of the scroll container by the amount calculated in the previous step, such that:
   1. The same first item is visible before and after adding the new items to the list, and
   2. All the newly-prepended items are out of view, and further towards the start of the list.

However, this prop does not work consistently – sometimes in step 3 the difference in position is incorrectly calculated to be zero. Furthermore, we have noticed that this seems only to happen consistently when the content length of newly-prepended list items is long.

Motivation

For a few months now, we have been endeavoring to get a working solution for a bidirectional-scrolling virtualized list in React Native. After working through many potential solutions, we have come very close to a working solution directly in React Native's VirtualizedList through the code in this PR. However, after lots of debugging we determined that the issues we were seeing weren't caused by the JS / VirtualizedList at all, but instead by this bug in ScrollView's maintainVisibleContentPosition prop, which is implemented in the native layer.

The result of this bug is that our implementation of the onStartReached prop in VirtualizedList suffers from the following issue:

1. When you reach the start of the list (contentOffset of 0), onStartReached is called.
2. The callback to onStartReached prepends new items into the list.
3. maintainVisibleContentPosition fails to update the contentOffset to account for those new list items.
4. The new list items are rendered, but the contentOffset is still 0, so the list position jumps to the start of the new content.
5. Because the contentOffset is 0, onStartReached is called again, and we get an infinite loop (at least, until there's no more content to load).

Android considerations

Another important piece of information is that the maintainVisibleContentPosition prop is not yet available on Android (implementation in progress). We have examined the in-progress Android implementation and found that it is very similar to the iOS one, and likely shares the same problem.

For the sake of this issue, the scope is focused on iOS, but we believe that the solution in one platform will be applicable in the other.

Potential cause

According to review comments from a Meta engineer, this bug is likely caused by a race condition between the items being added to the list and content offset being adjusted.

They also suggest implementing a binary search for the first visible item, which seems like it might improve the issue, but (in my opinion) is unlikely to resolve the race condition entirely.

Evidence of potential race condition?

In the FlatList example linked above, if you tweak these parameters as follows:

const PAGE_SIZE = 10;
const INITIAL_PAGE_OFFSET = 50;
const NUM_PAGES = 100;

The problem is mitigated but not completely solved (a few pages load before maintainVisibleContentPosition seems to "catch up" and function as expected). This hints that the problem may indeed be a race condition as suggested above.

autoScrollToTopThreshold wonkiness

According to the React Native documentation:

The optional autoscrollToTopThreshold can be used to make the content automatically scroll to the top after making the adjustment if the user was within the threshold of the top before the adjustment was made.

This suggests that with an autoScrollToTopThreshold of 0, then no auto-scrolling should occur if you have a non-zero contentOffset before new items are appended to the list. We have observed that this is not the case by:

1. Scrolling down a few pixels (without taking the minIndexForVisible item out of view)
2. Prepending an item.

Despite having an autoScrollToTopThreshold of 0 and a non-zero contentOffset, the ScrollView auto-scrolls to the top.

Interestingly, this particular ScrollView example 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 this FlatList example. It's unclear why removing the autoScrollToTopThreshold parameter fixes the problem, but setting a value of 0 does not. 🤔

Workaround:

While there may be workarounds possible via hacks in the JS code involving setTimeout or extra calls to scrollToIndex/scrollToOffset, these would not solve the root problem. In order to have a proposal accepted, it must fix the problem in the React Native codebase itself, probably in the native layer.

Platform:

Right now, this problem has been confirmed on iOS. It very likely exists on Android as well in this implementation. We are working on confirming the issue there, and will be following the progress of that pull request.

For the scope of this issue, we'll only require a fix in iOS or Android (preferably iOS), submitted as a PR against the our react-native fork: https://github.com/Expensify/react-native. Applying the same fix in the other platform should be comparatively easy and can be treated as a follow-up.

---

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fe321bebf9b78f69
• Upwork Job ID: 1606337510652706816
• Last Price Increase: 2022-12-23

[MelvinBot]

Triggered auto assignment to @kadiealexander (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[roryabraham]

@kadiealexander reassigning to @mallenexpensify because we discussed this 1:1

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

[MelvinBot]

Current assignee @roryabraham is eligible for the Exported assigner, not assigning anyone new.

[mallenexpensify]

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

[zoontek]

I'm not sure I get the issue correctly since, when setting:

<ScrollView
  automaticallyAdjustContentInsets={false}
  maintainVisibleContentPosition={{ minIndexForVisible: 0 }}
  // …

contentOffset is indeed adjusted, scroll position maintained:

RPReplay_Final1646045092.MP4

---

<ScrollView
  automaticallyAdjustContentInsets={false}
  maintainVisibleContentPosition={{
    minIndexForVisible: 0,
    autoscrollToTopThreshold: 0,
  }}

contentOffset is adjusted, scroll position is updated if minIndexForVisible element +- autoscrollToTopThreshold is visible (approximately, sure).

RPReplay_Final1646045125.MP4

---

<ScrollView
  automaticallyAdjustContentInsets={false}
  maintainVisibleContentPosition={{
    minIndexForVisible: 1,
    autoscrollToTopThreshold: 1,
  }}

This case is really broken, with contentOffset being incorrect when removing elements. But it's not the purpose of this issue.

RPReplay_Final1646045208.MP4

@roryabraham Maybe I'm missing something?

[roryabraham]

@zoontek Thanks for following up. You may have missed this piece of the issue description:

Interestingly, facebook/react-native#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 Expensify/react-native#7. It's unclear why removing the autoScrollToTopThreshold parameter fixes the problem, but setting a value of 0 does not. 🤔

[zoontek]

@roryabraham No, I read it, but as I understand setting any autoScrollToTopThreshold value will activate auto scroll to top if the given index is visible (when a new element is added). Which it does (?)

[roryabraham]

Hi @zoontek – so you're saying that autoScrollToTopThreshold doesn't represent a pixel offset, but an offset represented in visual items? Interesting, as I did not glean that from the RN docs or implementation.

Nonetheless, I think this is still a valid bug report in that maintainVisibleContentPosition does not work consistently / appears subject to some race condition(s). Perhaps our minimal reproduction example is faulty / needs improvement, but the bug we observed is clearly reproducible in this PR branch, and I believe that bug surfaces in the native layer at the location described in the issue template.

[roryabraham]

@mallenexpensify Let's double this.

[mallenexpensify]

Price doubled to $20,000

[azimgd]

@roryabraham could you take a look at this code please: https://github.com/azimgd/MVCPExample/

make sure to npx patch-package after npm install

Here is the demo which maintains visible content position in both directions, is that the actual result you are looking for in this issue?:

Simulator.Screen.Recording.-.iPhone.13.-.2022-03-09.at.21.34.44.mov

CODEFIX DIFF

       CGRect newFrame = self->_firstVisibleView.frame;
-      CGFloat deltaY = newFrame.origin.y - self->_prevFirstVisibleFrame.origin.y;
-      if (ABS(deltaY) > 0.1) {
+      CGFloat deltaY = self->_scrollView.contentSize.height - self->_prevContentSizeHeight;
+        
+      if (ABS(deltaY) > 0.1 && newFrame.origin.y != self->_prevFirstVisibleFrame.origin.y) {

This will store the content size before and after new items are added. Delta will be calculated accordingly and correct content offset is applied without triggering onScroll event.

Changing minIndexForVisible is not focusing on that exact index right now (always defaults to current offset), but I will get that working once I validate the exact requirements.

[Sarveshwins]

could you take a look at this code please: https://github.com/Sarveshwins/ScrollViewbug

Here is the demo which maintains visible content position in both directions.

Screen.Recording.2022-03-10.at.12.05.33.PM.mov

const [sizeOfHeight, setsizeOfHeight] = useState(40);
const [bottomSizeOfHeight, setBottomSizeOfHeight] = useState(40);
const ChangeHeight = () => {
setsizeOfHeight(prevState => {
return prevState === 40 ? 90 : 40;
});
};
const ChangeBottomHeight = () => {
setBottomSizeOfHeight(prevState => {
return prevState === 40 ? 90 : 40;
});
};
function getRandomColor() {
let letters = '0123456789ABCDEF';
let color = '#';
for (let i = 0; i < 6; i++) {
color += letters[Math.floor(Math.random() * 16)];
}
return color;
}
const generate = (size = 10) => {
const rows = new Array(size).fill(true).map((item, index) => ({
key: index,
size: Math.round(Math.random() * 100),
color: getRandomColor(),
}));
return rows;
};
const [dataForVisible, setdataForVisible] = useState(generate());
const OnAppend = () => {
setdataForVisible(state => {
return [
{
size: Math.round(Math.random() * 100),
color: getRandomColor(),
widthsize: sizeOfHeight,
},

    ...state,
  ];
});

};
const onFilter = () => {
setdataForVisible(state => {
return state.filter((item, index) => index != 0);
});
};
const addAtBottom = () => {
setdataForVisible(state => {
return [
...state,
{
size: Math.round(Math.random() * 100),
color: getRandomColor(),
widthsize: bottomSizeOfHeight,
},
];
});
};

const DeletAtBottom = () => {
setdataForVisible(state => {
return state.filter((item, index) => index != state.length - 1);
});
};

[parasharrajat]

Looking at proposals shortly... Need to understand the problem first.

[mallenexpensify]

Is there a link to comment linking discussions or a doc being worked on?

[roryabraham]

• issue: https://github.com/Expensify/Expensify/issues/147480
• doc: https://docs.google.com/document/d/1v-ZaIRTZL5LIsyPWB0IopBNNgCajf5WC1OA8cVKtd8I/edit#heading=h.ns12dr1f35rx

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01fe321bebf9b78f69

[melvin-bot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @aimane-chnaif (Internal)

[JmillsExpensify]

Not sure if C+ is explicitly required at this point, I mainly wanted to clear up that we have a plan that we are working on internally, thus I update the issue with the internal label appropriately.

[aimane-chnaif]

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

[mallenexpensify]

@janicduplessis @roryabraham can you provide an update?

[roryabraham]

Upstream PR is in review: facebook/react-native#35993

[mallenexpensify]

@janicduplessis can you reply to the comments in the PR - facebook/react-native#35993 (comment)

Pasting here in case others have ideas

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:

User adds data before the viewport
VirtualizedList responds to the new data, by rendering an index-based window around the new data
maintainVisibleContentPosition doesn't save us and adjust scroll position because the old item isn't still rendered
What I am wondering, could we attack facebook/react-native#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.

and

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).

[mallenexpensify]

From @janicduplessis last week

I will also be away for the next few weeks so I will work on addressing your last round of feedback when I am back.

[janicduplessis]

All of the PRs have now landed upstream!

[mallenexpensify]

It looks like facebook/react-native#35993 (comment) is closed, is there anything else we're waiting on here?

[roryabraham]

It's just a documentation update, but I think the last PR we want to see merged before this is 100% complete is facebook/react-native-website#3721

[darkbasic]

Well I guess that was quick :)

[melvin-bot]

📣 @darkbasic! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[roryabraham]

facebook/react-native-website#3721 merged. Closing this out! Great job @janicduplessis

[mallenexpensify]

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