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 Inverted VirtualizedList's baseline measurement for items' offset (1st of 4 problems that cause #1254) #2412

Conversation

LucioChavezFuentes
Copy link
Contributor

@LucioChavezFuentes LucioChavezFuentes commented Oct 23, 2022

Update:

This proposal is deprecated, New proposal here



I found 4 problems that cause the Inverted VirtualizedList to go wild (#1254). This PR will explain and fix the 1st of 4 problems found.

If we fix the following 4 problems, issue #1254 will be fixed and FlatList’s scroll will be more stable:

  1. Inverted VirtualizedList has incorrect baseline measurement for its items’ offset —> Problem Explanation and Solution below.

Update:

PR 2, 3, and 4 will fix 'Flatlist with expensive items breaks scroll' issue. PR 1 is enough to fix the Inverted Flatlist issue if your Flatlist's items are cheap to mount.


  1. $lead_spacer expands scroll artificially when Inverted 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 (this happens also for normal lists) —> Problem Explanation and Solution in this PR.

  3. VirtualizedList gets offsets equal to zero for items that are not the list's first item (this happens also for normal lists) —> Problem Explanation and Solution in this PR.

Inverted FlatList with all problems fixed

2022-10-23.17-49-22.mp4

1st Problem

Inverted VirtualizedList has incorrect baseline measurement for its items’ offset. (1st of 4 problems that cause #1254)

FlatList uses a virtual area (VirtualizedList) where it only renders the elements we need to see. In a FlatList with items with different heights (like report chat list), VirtualizedList mounts and measures the ‘y’ offset position of each item and saves it on an object called _frames.

Once the offset of an item is saved, VirtualizedList (on each scroll) can safely unmount the measured item if it is out of the virtual area. And mount it back if the offset position is in the current virtual area. So an accurate and complete _frames is fundamental for a smooth and fast scroll on FlatList.

Inverted FlatList has inaccurate offset values in the object _frames because it measures items’ offsets (y position) as baseline measurement at the top (y = 0), but the scroll view’s offset baseline measurement is at the bottom.

Why?

Inside OnLayout’s react-native-web implementation, y offsets measures are done with getBoundingClientRect(). This API measures the DOM elements offsets as we see it, with y = 0 at the top of our viewport (as a starting point of measure).

image8

A normal FlatList looks and measures like this:

Screen Shot 2022-10-13 at 17 26 21

The inverted FlatList uses {transformY: -1} that inverts not only our items but also our initial scroll position of the scroll view. y = 0 is now at the bottom of our scroll view:

Screen Shot 2022-10-12 at 17 53 57

But Inverted FaltList still measures item's offsets using getBoundingClientRect()(with onLayout) with y = 0 at the top:

Screen Shot 2022-10-12 at 17 56 24

As you can see the first items have the greatest offsets and our last items have offsets closer to zero. But on each scroll, FlatList's scroll view expects the opposite. The problem gets worse with longer lists because our first item will have the greatest offset, but it will be based only within the virtualized area, not on our entire list.

Solution

Use items’ offsetTop instead of onLayout’s y. offsetTop gets the correct offset for each item in normal or inverted lists because it ignores parent's transformations (like transform: [{scaleY: -1}]).

react-native-web/src/vendor/react-native/VirtualizedList/index.js
Screen Shot 2022-10-25 at 8 35 16

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 23, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5482ad5:

Sandbox Source
react-native-web-examples Configuration

@LucioChavezFuentes
Copy link
Contributor Author

@necolas Let me know what you think, I'm happy to answer any questions.

@Sharcoux
Copy link

Never seen a PR so well detailed. Nice job on that.

Copy link
Owner

@necolas necolas left a comment

Choose a reason for hiding this comment

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

Hi, thanks for taking the time to explain your PRs in detail. Several of them appear to not be web-specific and so I wonder if you've looked into how much of this code should be upstream in react-native? The VirtualizedList code is almost entirely copy-pasted from react-native every few months, and web-specific code is likely to get lost each time that happens. If you can identify only the code that needs to exist in the react-native-web fork, that would be a big help. Thanks

@LucioChavezFuentes
Copy link
Contributor Author

LucioChavezFuentes commented Oct 24, 2022

Thank you @necolas, I didn't know about the connection between react-native-web and react-native. I thought components in react-native, like VirtualizedList, were purely written in native code.

I will review my code with these insights and make PRs to the according repos.

@LucioChavezFuentes
Copy link
Contributor Author

Hi @necolas , Just gut checking, I am planning to find a solution to make Web onLayout 'y' behave the same as RN's onLayout 'y' in inverted VirtualizedList. Do you think this would be a good path to follow?

In case you agree, do you know how I can test onLayout in multiple scenarios so I can't break something?

@JWesorick
Copy link

Are these changes being submitted to react-native?

@LucioChavezFuentes
Copy link
Contributor Author

Hi @necolas, I submitted PR 2, PR 3, and PR 4 for upstream react-native. I think this PR should be in react-native-web.

@necolas
Copy link
Owner

necolas commented Nov 22, 2022

@LucioChavezFuentes Thanks! Please close any PRs here that you no longer believe necessary.

@necolas
Copy link
Owner

necolas commented Nov 22, 2022

Hi @necolas , Just gut checking, I am planning to find a solution to make Web onLayout 'y' behave the same as RN's onLayout 'y' in inverted VirtualizedList. Do you think this would be a good path to follow? In case you agree, do you know how I can test onLayout in multiple scenarios so I can't break something?

I think this is probably a good idea because we're also trying to make these lists npm packages that react-native-web can import, rather than having to maintain forked versions. And there may be other scenarios in which the difference in y values causes unexpected issues on web. In general it should be ok to make changes to onLayout, because it is used so much in APIs like the lists and other RN packages. If unexpected things break in a canary/RC release, we could always explore using patch-package in the future to make surgical changes to the list packages

@genie19197
Copy link

Great details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flatlist with expensive items breaks scroll
6 participants