[HOLD #7925] [$16000] Pagination seems broken for chats

[mvtglobally]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Navigate to chat with Chronos
2. Scroll through multiple messages and check pagination

Expected Result:

Messages should load without any issue

Actual Result:

Messages get stuck and pagination doesn't load properly

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.1.39-1 Reproducible in staging?: Y Reproducible in production?: Y Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/43995119/155066831-17521f7e-5a47-452a-9b13-6cf48efcf8d0.mp4

Expensify/Expensify Issue URL: Issue reported by: @marcaaron @kidroca Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1644435691436529 https://expensify.slack.com/archives/C01GTK53T8Q/p1641586928143800?thread_ts=1641315371.490700&cid=C01GTK53T8Q

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @neil-marcellini (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[neil-marcellini]

I was able to reproduce this on Web and Desktop, particularly if I scrolled up quickly. As shown in the video, this occurs on any chat, not just with Chronos.

[MelvinBot]

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

[MelvinBot]

Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[MelvinBot]

Still overdue 6 days?! Let's take care of this!

[MelvinBot]

8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[adelekennedy]

jeez - just getting to this Internal External

[MelvinBot]

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

[MelvinBot]

Triggered auto assignment to @AndrewGable (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[adelekennedy]

From this slack convo this may be related to the flatlist fix (purely for context)

[MelvinBot]

Triggered auto assignment to @bfitzexpensify (Internal) because issue was reported by a contributor who needs to be compensated if this issue is fixed

[MelvinBot]

Current assignee @bfitzexpensify is eligible for the External assigner, not assigning anyone new.

[AndrewGable]

I agree with @adelekennedy that this might be related to the FlatList bug, so let's double check that before diving too deep into proposals.

[AndrewGable]

Let's double the price @bfitzexpensify !

[Santhosh-Sellavel]

@AndrewGable Lets update the bug title to

Pagination seems to broken for chats ~~with Chronos~~

[AndrewGable]

Updated!

[bfitzexpensify]

Doubled price and updated job title in Upwork.

[AndrewGable]

Let's double again

[bfitzexpensify]

another 7 days without any action here - doubled again

[AndrewGable]

Let's double again.

[bfitzexpensify]

doubled again 🤑

[bfitzexpensify]

Still no action here - doubled to $16,000

[izhan]

Anyone know of a reliable repro? I see this maybe 5% of the time (testing on desktop, Chrome, OSX), curious if anyone sees this more often / has any insight there.

EDIT: never mind, found the root cause! will post a proposal shortly

[izhan]

Proposal

TLDR; fixing how we measure layout in BaseInvertedFlatList

Problem

The easiest way to repro this problem is to actually upload a bunch of images that have a different size than our placeholder image, then scroll up quickly (explanation why images matter is below in Issue 2). The root cause is that layout measured in BaseInvertedFlatList suffers from two issues right now:

Issue 1: Layout is not called in an order that lets us compute an offset from a previous offset, leading to incorrectly measured offsets

We measure offset relative to the previous offset, which if not available defaults to 0 https://github.com/Expensify/App/blob/6333e6aef5e3abb500889b86b5a5e6cee41aeeae/src/components/InvertedFlatList/BaseInvertedFlatList.js#L113

Thus we get incorrect offsets when previousOffset is not available. This is most apparent in the initial mount. measureItemLayout is called with the order of last item to first item, meaning every single item is initialized with an offset of 0.

You can see this if you peek at this.sizeMap (our cached list of offsets / lengths) right after mount:

Issue 2: Images have a different rendered height, after it's loaded

When images are loading, we have a placeholder spinner. After the image loads however, the height of the row changes.

https://user-images.githubusercontent.com/4621962/165632988-980f7e15-aa42-4ad3-95a1-f663bb2e219b.mov

However, we no longer update the calculated row heights / offsets in sizeMap: https://github.com/Expensify/App/blob/main/src/components/InvertedFlatList/BaseInvertedFlatList.js#L102

This leads to incorrectly calculated heights / offsets, an issue that further compounds because all offsets are relative to previous offsets. Thus, not only does the row with the image have the wrong offsets / height, but every single row after it also has the incorrect offsets / height.

This explain my trouble with reproing – it was near impossible to reproduce...until I added a bunch of images (which is in the PR description recording as well!)

Solution

getItemLayout asks us to return offset, the distance from top edge of the item to the top of the content. We have an inverted list, so our "top" is actually the "bottom". So instead of trying to calculate relative offsets based on the previous item, let's try to calculate the distance from the bottom edge of an item to the bottom of the content.

In a nutshell, given a row, the formula for calculating its "bottom" is: Parent's scroll distance + Height of parent - (Distance from item's top edge to top of parent - Height of Item)

A quick explanation:

• Parent's scroll distance + Height of parent is the distance from the bottom of the content to the top edge of the parent
• Distance from item's top edge to top of parent - Height of Item is distance from top edge of parent to bottom of edge of item
• Both values are relative to the top edge of the parent. So finding the difference between the two values will just leave us with distance between bottom edge of item to bottom of content

I know it might be a bit confusing but I'm also happy to draw a diagram if helpful

Here are the changes needed:

1. Add to the FlatList props https://github.com/Expensify/App/blob/6333e6aef5e3abb500889b86b5a5e6cee41aeeae/src/components/InvertedFlatList/BaseInvertedFlatList.js#L142

onLayout={({nativeEvent}) => {
  this.windowLength = nativeEvent.layout.height;
}}
onScroll={({nativeEvent}) => {
  this.scrollPosition = nativeEvent.contentOffset.y;
}}

2. Add to https://github.com/Expensify/App/blob/6333e6aef5e3abb500889b86b5a5e6cee41aeeae/src/components/InvertedFlatList/BaseInvertedFlatList.js#L38

this.windowLength = 0
this.scrollPosition = 0

3. Replace all of measureItemLayout with following https://github.com/Expensify/App/blob/6333e6aef5e3abb500889b86b5a5e6cee41aeeae/src/components/InvertedFlatList/BaseInvertedFlatList.js#L96

this.sizeMap[index] = {
    length: nativeEvent.layout.height,
    offset: this.windowLength + this.scrollPosition - (nativeEvent.layout.top - nativeEvent.layout.height),
};

[marcaaron]

Interesting and thank you for the thorough proposal @izhan! FWIW we identified something similar in this PR while working with @chiragsalian to fix scrollToIndex().

What's the ultimate root cause here? It feels like there is some bug in react-native (or possibly related to react-native-web) where the heights are not being measured? IIRC we implemented that code to solve a rendering issue first described in react-native-web described in these issues (Issue 1 / Issue 2). But obviously we did something wrong and did not catch that some things were not measured (only recently though and haven't fixed it yet) :D

So, is it an acceptable workaround to supply the correct heights to the sizeMap? Or is this an issue with react-native and we should fix there? Curious to hear more opinions as we've been fighting to improve FlatList for a long time.

cc @roryabraham as well since you've also worked a lot with FlatList

[marcaaron]

Also curious if those crazy scrolling issues we ran into would be fixed by https://github.com/necolas/react-native-web/pull/2248 and if so, whether we could remove this old hack and also stop measuring the items entirely. Not sure if we can cleanly test that version of react-native-web with the new vendor files because we are running a fork (someone correct me if I'm wrong there).

[izhan]

Great questions @marcaaron – you were the guy I was hoping would chime in here! While creating the proposal, I studied those linked issues as well as your original PR https://github.com/Expensify/App/pull/536 extensively.

I think what would help my understanding is any context you have on https://github.com/Expensify/App/pull/536 – do you remember what was specifically "broken" that needed this hack? Copy / paste, scrolling, pagination, etc

Also what was the crazy scrolling issues you're referring to? My hunch is it could be related to incorrect offset calculations in sizeMap. While creating this proposal, I saw this crazy scrolling behavior more frequently when I calculated incorrect offsets; if you leave out this.scrollPosition in the proposal and change it to offset: this.windowLength - (nativeEvent.layout.top - nativeEvent.layout.height) for instance, you'll see what I mean.

I think measurement issues is less a specific bug in react-native, but more-so how virtualization was implemented due to limitations of how browsers think about scrollTop. inverted for VirtualizedList is tricky because most of React Native's measurements is relative to the top edge, not the bottom edge. So it's "broken' in the sense you'll need to fix a few places in VirtualizedList in a similar manner to this proposal (keep track of height of window, scroll, do some math, etc).

To your point about root cause, we can also move the logic in my proposal into react-native as well and help resolve onEndReached issues in https://github.com/necolas/react-native-web/issues/1254. We'll need to do the "math" in only a few places, namely https://github.com/facebook/react-native/blob/ff80493d6e80dd0a05c76849c8e13d4b3089d919/Libraries/Lists/VirtualizedList.js#L1503 based on the inverted flag. Haven't tried that out yet but can test it if you want.

[marcaaron]

do you remember what was specifically "broken" that needed this hack? Copy / paste, scrolling, pagination, etc

• When scrolling large lists the cursor position would jump around erratically (basically stuff described here). This was resolved by adding the measureItemLayout (despite it being implemented incorrectly).

• The web wrapper around FlatList we are using is working around this issue see https://github.com/Expensify/App/blob/main/src/components/InvertedFlatList/index.js. It looks like we tried to remove it but later reverted that change.

• The copy / paste was fixed by @azimgd which led us to create a fork of react-native-web here (can see the PRs submitted so far here)

we can also move the logic in my proposal into react-native

I think that would be preferable, but will let others chime in.

[kidroca]

The copy / paste was fixed by @azimgd which led us to create a fork of react-native-web here (can see the PRs submitted so far here)

Everything started here ☝️

The layout calculation used to work correctly or hide it's flaws because the inversion happened through css. Meaning that even if you don't get the correct size/offset content was pushed in a direction that would not affect current scroll position To achieve the same without css inversion there have to be a precise layout calculation in order to preserve scroll position The css inversion was removed to fix the "copy / paste" issue. Instead items are reversed through reversing the data array (I think)

Would there be a problem if we include a fix in react-native?

1. It seems our layout calculation is wrong - causing the jumps
2. The fix for inverted (that introduced the issue) was in react-native-web

@izhan

getItemLayout asks us to return offset, the distance from top edge of the item to the top of the content. We have an inverted list, so our "top" is actually the "bottom".

Is this also true for ios/android? The problem so far is only on web correct? So changing that logic might affect ios/android in a buggy way

[kidroca]

One more thing to point out for the sizeMap would be to capture dimensions by id or sequenceNumber - this way when, due to virtualization, an item is added back to the virtual window we would have the correct dimensions (indexes change)

This can be achieved by using the keyExtractor

measureItemLayout(nativeEvent, item) {
  const key = this.props.keyExtractor(item);
  /*...*/
  this.sizeMap[key] = size;
}

getItemLayout(data, index) { 
  const key = this.props.keyExtractor(data[index]);
  /* ... */
}

We might need to do something about item offset changing

onLayout would be called only for items that are mounted (inside the virtual window) So captured sizes in the sizeMap for items that are no longer mounted would have the correct height but not necessarily the correct offset:

• offset can change for the same reason index changes (older comments getting deleted)
• a comment is edit and this causes a height change
• or when images render and change size

These would trigger onLayout only if the item is still mounted

We'll probably need to recalculate other item offsets when a comment changes dimensions, so that when they're added back we know the offset ahead of time, instead of after render triggers onLayout