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

[Native part] Add workaround for android API 33 ANR when inverting ScrollView

Open hannojg opened this issue 2 years ago • 9 comments

Summary:

This PR is a result of this PR, which got merged but then reverted:

  • https://github.com/facebook/react-native/pull/37913

We are trying to implement a workaround for #35350, so react-native users on android API 33+ can use <FlatList inverted={true} /> without running into ANRs.

This is the native part, where we add a new internal prop named isInvertedVirtualizedList, which can in a follow up change be used to achieve the final fix as proposed in https://github.com/facebook/react-native/pull/37913

However as @NickGerleman pointed out, its important that we first ship the native change.

Changelog:

[ANDROID] [ADDED] - Native part of fixing ANR when having an inverted FlatList on android API 33+

Test Plan:

  • Check the RN tester app and see that scrollview is still working as expected
  • Add the isInvertedVirtualizedList prop as test to a scrollview and see how the scrollbar will change position.

hannojg avatar Jun 26 '23 16:06 hannojg

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,041,691 -5,813
android hermes armeabi-v7a 8,292,728 -4,195
android hermes x86 9,558,910 -4,884
android hermes x86_64 9,400,740 -5,212
android jsc arm64-v8a 9,601,315 -4,905
android jsc armeabi-v7a 8,729,512 -3,422
android jsc x86 9,689,270 -3,992
android jsc x86_64 9,935,285 -4,260

Base commit: 971bb81d131089e3c4201a19f3fa69b24dc72cb0 Branch: main

analysis-bot avatar Jun 26 '23 17:06 analysis-bot

I think we need this in two more places:

  1. The type in ScrollViewNativeComponent?
  2. The ViewConfig (I think also in ScrollViewNativeComponent). inverted was already there before for reasons I don't know (it was not passed as a prop before). This runtime structure tells React Native to pass the prop from JS to native.

NickGerleman avatar Jun 26 '23 17:06 NickGerleman

@NickGerleman thank you for having a look, I updated the files!

[Interestingly this change was already working without these changes (running in fabric on new arch) 👀 ]

hannojg avatar Jun 26 '23 18:06 hannojg

Could we update ScrollViewNativeProps here? https://github.com/facebook/react-native/blob/e1fd4a8fcd3c7b06d37dda6890137e344b416aec/packages/react-native/Libraries/Components/ScrollView/ScrollViewNativeComponentType.js#L19

NickGerleman avatar Jun 26 '23 20:06 NickGerleman

@NickGerleman Updated!

hannojg avatar Jun 27 '23 05:06 hannojg

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

facebook-github-bot avatar Jun 27 '23 16:06 facebook-github-bot

Personally, I prefer inverted to internalAndroidApplyInvertedFix. The main reason for this is that we need to pass inverted to the native ScrollView component for react-native-windows and react-native-macos anyway to implement keyboard / mouse wheel friendly inversion.

If @NickGerleman finds inverted to be too general / likely to lead to confusion, then removing Android from the prop name and instead using something like isInvertedVirtualizedList would be prefered.

In fact, this isn't the first thing we've wanted to pass context that the ScrollView is responsible for rendering a VirtualizedList / FlatList before. In react-native-windows, we wanted to build in functionality to default to arrow-key based keyboard navigation, but only when rendered in the context of a FlatList.

rozele avatar Jun 29 '23 13:06 rozele

@rozele I don't have a opinion on that and go with what the you the core maintainers are saying 😅 For now I renamed it to isInvertedVirtualizedList, happy to change that any time to something else if needed!

hannojg avatar Jul 03 '23 16:07 hannojg

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

facebook-github-bot avatar Jul 06 '23 04:07 facebook-github-bot

This pull request was successfully merged by @hannojg in 6d206a3f54725f7f53692222293a9d1e58b11ca4.

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

github-actions[bot] avatar Jul 14 '23 02:07 github-actions[bot]