Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix items skip measure when items are expensive to render #35414

Conversation

LucioChavezFuentes
Copy link

@LucioChavezFuentes LucioChavezFuentes commented Nov 20, 2022

Summary

Flatlist on Web has scroll issues when used with expensive items.

I noticed this issue in an App called Expensify. It uses Flatlist for a chat of reports that supports text, images, emojis, and reports as items (and perhaps others that I am not aware of). These items have complex code to support interactions and features on them. As you scroll in the report chat in Web, especially if you scroll fast, you will notice scroll issues like scroll jumps, items appearing and disappearing, or items not showing at all.

Here's Expensify Flatlist Web current state:

Flatlist.Web.Broken.mp4

I recreate a sandbox with expensive items where you can experience the scroll issues I mentioned here.

Steps to reproduce the scroll issues in the sandbox:

  1. Scroll down for a while until you render a few items beyond the virtual area.
  2. Use the mouse and drag the scroll bar up and down.
  3. Things should start looking weird: scroll jumps, items appearing, and disappearing.

I take on the task to improve the scroll experience of react-native-web's Flatlist. I found 3 problems that cause this issue and present their corresponding solution:

  1. $lead_spacer expands scroll artificially when VirtualizedList is mounting new items —> Problem Explanation and Solution in this PR.

  2. VirtualizedList skip items for offset measuring when the user scrolls very fast while new items are mounted and measured —> Problem Explanation and Solution below.

  3. VirtualizedList gets offsets below or equal to zero for items that are not the list's first item —> Problem Explanation and Solution in this PR.

These solutions involve adding or modifying VirtualizedList.js but they improve drastically the scroll experience on Web without causing any regression on Android or iOS.

Also here's Expensify's App after solutions (plus another solution for Inverted VirtualizedLists in react-native-web):

Flatlist.Web.Good.mp4

This PR is the Second Part solution to fix the 'Flatlist with expensive items breaks scroll' issue in react-native-web.

VirtualizedList skip items for offset measuring when the user scrolls very fast while new items are mounted and measured

There are two important variables (react states of VirtualizedList) that determine the virtual area: first and last. These variables represent a range of values from the data array (which is raw information of items provided to FlatList as data prop) that we use to render items on the virtual area.

On every scroll, first and last get updated according to two priority updates: Low Priority and High Priority.

Low Priority updates help us to wait for new items to be mounted and measured. These updates are scheduled in a queue and are useful when the user is scrolling on unmeasured area.

High Priority updates help us to mount items as fast as possible. These updates occur when users scroll fast: if a High Priority update is triggered, every Low Priority update on the queue is canceled.

When the user scrolls up to unmeasured area, ideally (on every update) the prev last should be greater than the next first. This ensures that every item is measured and saved in _frames, like this:

Screen Shot 2022-10-12 at 18 06 56

But If we scroll very fast towards unmeasured area and the list is still rendering new items, a race condition happens between measuring new items in onLayout and the High Priority update. This is because onLayout (executed on unmeasured items) takes some time to return the offset value that we need after the item is mounted. If High Priority wins, the next first will be greater than prev last, and as a consequence, some items will be skipped for measuring:

Screen Shot 2022-10-12 at 17 25 01

More than one item can be skipped, which makes the problem worse. The more items are skipped to be measured, the greater the probability to experiment scroll issues (like weird jumps or white space in the visible area).

Solution:

Screen Shot 2022-11-18 at 18 25 15

1.-Description of the first line of code:

Screen Shot 2022-11-18 at 18 40 24

We use a variable called totalCellLength, it represents the sum of heights of all items measured.
If totalCellLength is greater than scroll offset + visibleLength / 2 (half of our Visible Area) it means the measure of new items is keeping up with the scroll toward unmeasured area or that we are in an area where the cells had been already measured so we allow High Priority updates as usual:

Screen Shot 2022-10-12 at 18 13 08

If totalCellLength is not greater than scroll offset + visibleLength / 2 then VirtulizedList prevents High Priority updates, allowing Low Priority updates on queue to complete so measure of items can catch up with scroll and prevents subsequent updates where next first is greater than prev last:

Screen Shot 2022-10-12 at 18 14 41

Why visibleLength / 2?

It represents a balanced ‘bar check’ to be sure that all items are gonna be measured and mounted as fast as possible (when user scrolls quickly towards unmeasured area). I tested with other two approaches:

  • totalCellLength > offset + visibleLength
  • totalCellLength > offset

With offset + visibleLength (top of Visible Area) we set the ‘bar check’ too high: the check will be false as soon as a new item is mounted. This denies any margin to make High Priority Updates when users keep scrolling up to unmeasured area.

With totalCellLength > offset (bottom of Visible Area), the check will be true only if one new item is measured in our visible area. This will trigger more High Priority updates than it should, Low Priority updates will be canceled more often, and we lose the certainty that no items are gonna skip measure.

I tested previous scenarios and I conclude scroll offset + visibleLength / 2 is the fastest way to scroll safely toward unmeasured area.

2.-Description of the second line of code:

Screen Shot 2022-11-18 at 18 43 33

Prevents the checking of the first line of code (allows High Priority updates as usual) if the FlatList doesn't have enough rendered items to fill both the Visible Area and the initialNumToRender items (the number of items at the beginning of FlatList that never unmount). This preserves a fast list initialization.

Changelog

[GENERAL] [FIXED] prevents VirtualizedList skips measure items when items are expensive to render

Test Plan

You can test the Flatlist with all solutions applied in this sandbox

Naturally, expensive items will take time to show but you should find no issues on scroll fast.

Also tested in Expensify's App I am working on (see video above)

No problem with iOS

iOS.Flatlist.Compressed.mp4

No problem with Android

Android.Flatlist.mp4

Thank you for reading, let me know what you think!

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,102,233 -207
android hermes armeabi-v7a 6,470,308 -185
android hermes x86 7,519,730 -165
android hermes x86_64 7,378,388 -185
android jsc arm64-v8a 8,967,170 -139
android jsc armeabi-v7a 7,698,020 -132
android jsc x86 9,029,332 -86
android jsc x86_64 9,507,143 -120

Base commit: 81e441a
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 81e441a
Branch: main

@pull-bot
Copy link

PR build artifact for 6887404 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for 6887404 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

Copy link
Contributor

@NickGerleman NickGerleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the super detailed writeup!

Trying to wrap my head around this race condition a bit more. It sounds like the sequence where it happens is:

  1. Low priority update updates the displayed range, and we wait for onLayout before measurement
  2. High priority update (before onLayout returns)

Are you suggesting that the intermediate items are never measured? Or is it just that we are using cells that we have not yet received layout information for? Reading through the previous logic, a high pri update cancels the previous low pri update, and call updateCellsToRender a single time with previous list state. updateCellsToRender will call into computeWindowedRenderLimits with the previous state. So computeWindowedRenderLimits should have full access to decide how to limit the new range, if it needs to (though I am not positive this is what happens).

In the default case, low pri renders given to batchinator are given a whole 50ms delay, which could inadvertently mask timing related issues.

Comment on lines +1596 to +1600
// We should trigger only high pirority updates on area with cells already measured or if measure new cells
// is keeping it up with scroll toward unmeasured area. Otherwise scrolling too fast toward unmeasured area
// while new items are being measured may skip cells to be measured and cause scroll issues on scroll back to skipped area.
// 'visibleLength / 2' represents a balanced ‘bar check’ to be sure that all items
// are gonna be measured and mounted as fast as possible when user scrolls quickly towards unmeasured area.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: spelling/typos and grammar

E.g. "We should trigger only high priority updates" reads as "we should never trigger low priority updates", where I think this was meant to be "We should trigger high priority updates only if".

@react-native-bot react-native-bot added the No CLA Authors need to sign the CLA before a PR can be reviewed. label Nov 22, 2022
@LucioChavezFuentes
Copy link
Author

Are you suggesting that the intermediate items are never measured? Or is it just that we are using cells that we have not yet received layout information for?

Hello again @NickGerleman. It's more like some cells are never mounted or cells get offset equal to zero.
Here some cells start returning zero:
Screen Shot 2022-11-22 at 17 45 58

Here some cells are skipped:
Screen Shot 2022-11-22 at 17 46 32

Here's a video of our list with all solutions applied except the solution in this PR.

Note: the list in the video also has the offsetTop solution for react-native-web to get correct offsets in Inverted Lists in Web.

2022-11-22.17-36-08.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No CLA Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants