react-native
react-native copied to clipboard
Add workaround for android API 33 ANR when inverting ScrollView
Summary:
As explained in this issue:
- https://github.com/facebook/react-native/issues/35350
starting from android API 33 there are severe performance issues when using scaleY: -1 on a view, and its child view, which is what we are doing when inverting the ScrollView component (e.g. in FlatList).
This PR adds a workaround. The workaround is to also scale on the X-Axis which causes a different transform matrix to be created, that doesn't cause the ANR (see the issue for details). However, when doing that the vertical scroll bar will be on the wrong side, thus we switch the position in the native code once we detect that the list is inverted.
The goal of this PR is that react-native users can just use <FlatList inverted={true} /> without running into any ANRs or the need to apply manual hot fixes 😄
Changelog:
Test Plan:
- The change is minimal, and only affects android.
- Run the RNTesterApp for android and confirm that in the flatlist example the inverted list is still working as expected.
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| android | hermes | arm64-v8a | 8,757,479 | -204 |
| android | hermes | armeabi-v7a | 8,070,058 | -198 |
| android | hermes | x86 | 9,250,082 | -194 |
| android | hermes | x86_64 | 9,099,221 | -195 |
| android | jsc | arm64-v8a | 9,319,056 | +227 |
| android | jsc | armeabi-v7a | 8,508,955 | +219 |
| android | jsc | x86 | 9,382,540 | +223 |
| android | jsc | x86_64 | 9,635,790 | +226 |
Base commit: 7d4f233db32af0f8fbbe59de549532678ebf4b8e Branch: main
Thanks for the PR! I'm going to pass this over to @rozele who has spent a lot of time fighting with inverted FlatList, to get his take.
There is some later work we have been potentially looking at to move away from inversion based on transform (this fixes a couple other issues, like Talkback not working correctly). This issue has been raised super often though, so we definitely shouldn't block on that if we have a reliable workaround to the Android platform issue.
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
This pull request was successfully merged by @hannojg in 90186cd9b71bc6ffb593448c9bb5a3df66b3a0c6.
When will my fix make it into a release? | Upcoming Releases
Will this fix make it into a 0.70.x release?
I would like for us to live with this change for a bit before we pick.
We should get feedback soon from Meta usages, but it wouldn’t hurt to also get RC validation. Though the timing with the release window would make that a long gap.
cc @cortinico who has followed the inverted flat list/chat ui perf issues.
I took a closer look at this change and found a couple issues, one of which would lead to a regression. I tried to find a short fix, but am unlanding for now instead.
The main one, is that RN internally may ship JS changes before native changes. This means we have a configuration where VirtualizedList uses scale instead of scaleY, but the new code in the ViewManager isn't yet running. We will need to either ship the native change first, or add a signal to read in JS. I would recommend the first since I think the API for view manager constants is in flux I think.
The other issue, that is more cosmetic is around types. Right now we pass every VirtualizedListProps prop to the ScrollView, and suppress type-checking. But we should be adding inverted to the props in ScrollViewNativeComponent if we are going to start passing it. Though I saw it was already part of the View Config.
This pull request has been reverted by f544376f7c8d008e79a1ab5efdb0da81ab9f73ac.
@NickGerleman thanks for examining this closely! What are the next steps here? Should I make a change to the PR so the issue you named gets handled correctly?
The next step would be to make a PR that:
- Adds the native component prop. I might actually recommend a new name instead of inverted, in case anyone accidentally thinks it will invert their ScrollView.
- Makes sure VirtualizedList doesn't trigger the new prop, until the native change is rolled out. Then we can have VirtualizedList take advantage of it.
Hey @NickGerleman,
I will create two PRs now,
- One PR with the native change, but a new prop name. Then we can merge it and wait till it's rolled out.
- A PR containing the JS fix, which will set the new prop to apply the fix.
I think this way it should be fine right? I think regular react native users won't be affected by the issue that the JS code of react native might get updated before the native code does.