-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Ignore transformation and cancel callback measure for unmounted elements #2501
Conversation
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 9008a8b:
|
Thanks again for the clear and detail-oriented work. Great job with your test cases. Since this is technically a "fix" to match RN, do you think we can put this out in a patch release (0.19.1)? |
Sure, so... can I say this PR will be merged? |
Yes, I can get it out in 0.19.1 today if you rebase your PR |
Also, FYI
|
…n measure layout of unmounted nodes fix onLayout unmounted element race condition t -C "/Users/luciochavez/projects/expensify-react-native-web" -c "core.editor=code --wait --reuse-window" branch --delete --force fix-onLayout-unmounting-race-condition git -C "/Users/luciochavez/projects/expensify-react-native-web" -c "core.editor=code --wait --reuse-window" branch -m fix-onLayout-unmounting-race-condition-2 fix-onLayout-unmounting-race-condition git -C "/Users/luciochavez/projects/expensify-react-native-web" -c "core.editor=code --wait --reuse-window" branch --delete fix-onLayout-unmounting-race-condition change measure implementation in order to ignore transformations use isConnected
65be413
to
9008a8b
Compare
@necolas, the PR has been rebased! |
The PR fixes these issues:
This PR does not fix the following:
As for this issue: The measures are correct with this PR's changes and with the current implementation (as long as there are no transformations) but the |
Hello again @necolas, this the updated proposal to fix the VirtualizedList in web.
After investigation and feedback I have a new proposal of pull requests to fix the Inverted VirtualizedList in web. Some of the root issues are in react-native-web's end, another issue is fixed updating VirtualizedList from react-native:
Problem 1: VirtualizedList's code relies on that
onLayout
measures items without considering transformations —> Problem Explanation and Solution below.Problem 2: Exists inaccurate measures in onLayout when the setTimeout is executed and any of the targeted nodes (node or relativeNode) for measure are unmounted. —> Problem Explanation and Solution below.
Problem 3: The scroll position can be forced to areas where items have not been measured and the list skips measuring items if the user scrolls too fast. —> Problem Explanation and Solution in this PR.
Problem 1
VirtualizedList's code relies on that
onLayout
measures items without considering transformationsReact-native-web's onLayout, unlike its native counterpart, accounts for element transformations and its ancestors which leads to inaccurate measures for Inverted VirtualizedList with "scaleY / X: -1" transformation.
Solution:
Compose onLayout with only Web measurement API that ignores transformation following the W3C and WHATGH standards.
I know the solution is similar to @davepack's PR so Why should we merge it now? What is different?
The @davepack's solution is good, the problem back then was a bug in Firefox. Firefox wasn't following the standard specification for offsetTop. It wasn't fixed until Firefox >71 (the @davepack PR was tested in Firefox 61).
Now I can confirm the PR's changes works in Firefox >71 and in the rest of the browsers including the mobile versions. I tested multiple scenarios here and also tested in the @davepack sandbox.
The solution works the same as getBoundingclientrect but without considering transformations with a margin of error of +/- 1 pixel because the variables like offsetTop and clientTop round the values and return integers only.
How should we handle problems like this?
WHATGH is a standard specification for browsers created by the browsers' creators (Mozilla, apple, Microsoft). It reached an agreement with W3C and now both standards reference each other and recommend how the browser API should work.
Different results among browsers with the same code is likely that some them are not following the WHATGH and W3C specifications and can be reported as a bug in the browser's end.
Problem 2
Exists inaccurate measures in onLayout when the setTimeout is executed and any of the targeted nodes (node or relativeNode) for measure are unmounted.
Solution
Check if the layout boxes of nodes are still present inside the setTimeout callback.
Problem 3 —> Problem Explanation and Solution in this PR.
With all problems resolved and updating VirtualizedList from react-native we have a solid VirtualizedList in web, inverted or non-inverted.
Thank you for reading @necolas, if you have any questions or concerns please let me know how I can help.