react-native icon indicating copy to clipboard operation
react-native copied to clipboard

Add workaround for android API 33 ANR when inverting ScrollView

Open hannojg opened this issue 2 years ago • 1 comments

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.

hannojg avatar Jun 15 '23 13:06 hannojg

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

analysis-bot avatar Jun 15 '23 14:06 analysis-bot

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 avatar Jun 15 '23 20:06 NickGerleman

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Jun 20 '23 20:06 facebook-github-bot

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Jun 21 '23 21:06 facebook-github-bot

This pull request was successfully merged by @hannojg in 90186cd9b71bc6ffb593448c9bb5a3df66b3a0c6.

When will my fix make it into a release? | Upcoming Releases

github-actions[bot] avatar Jun 22 '23 03:06 github-actions[bot]

Will this fix make it into a 0.70.x release?

ejain avatar Jun 22 '23 03:06 ejain

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.

NickGerleman avatar Jun 22 '23 07:06 NickGerleman

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.

NickGerleman avatar Jun 22 '23 08:06 NickGerleman

This pull request has been reverted by f544376f7c8d008e79a1ab5efdb0da81ab9f73ac.

facebook-github-bot avatar Jun 22 '23 09:06 facebook-github-bot

@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?

hannojg avatar Jun 22 '23 12:06 hannojg

The next step would be to make a PR that:

  1. 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.
  2. 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.

NickGerleman avatar Jun 22 '23 17:06 NickGerleman

Hey @NickGerleman,

I will create two PRs now,

  1. One PR with the native change, but a new prop name. Then we can merge it and wait till it's rolled out.
  2. 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.

hannojg avatar Jun 26 '23 11:06 hannojg