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

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

Closed
wants to merge 4 commits into from

Conversation

hannojg
Copy link
Contributor

@hannojg hannojg commented Jun 26, 2023

Summary:

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

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 #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.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 26, 2023
@analysis-bot
Copy link

analysis-bot commented Jun 26, 2023

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: 971bb81
Branch: main

@NickGerleman
Copy link
Contributor

NickGerleman commented Jun 26, 2023

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.

@hannojg
Copy link
Contributor Author

hannojg commented Jun 26, 2023

@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) 👀 ]

@NickGerleman
Copy link
Contributor

Could we update ScrollViewNativeProps here?

@hannojg
Copy link
Contributor Author

hannojg commented Jun 27, 2023

@NickGerleman Updated!

@facebook-github-bot
Copy link
Contributor

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

@rozele
Copy link
Contributor

rozele commented Jun 29, 2023

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.

@hannojg
Copy link
Contributor Author

hannojg commented Jul 3, 2023

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

@facebook-github-bot
Copy link
Contributor

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

@github-actions
Copy link

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

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

@github-actions github-actions bot added the Merged This PR has been merged. label Jul 14, 2023
juniorklawa pushed a commit to juniorklawa/react-native that referenced this pull request Jul 20, 2023
…book#38071)

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

- facebook#37913

We are trying to implement a workaround for facebook#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 facebook#37913

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

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

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

Pull Request resolved: facebook#38071

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.

Reviewed By: rozele

Differential Revision: D47062200

Pulled By: NickGerleman

fbshipit-source-id: d20eebeec757d9aaeced8561f53556bbb4a492e4
facebook-github-bot pushed a commit that referenced this pull request Jul 28, 2023
Summary:
This PR is a result of this PR, which got merged but then reverted:

- #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.

As explained in the issue, 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, using the newly added `isInvertedVirtualizedList` prop.

This is a follow up PR to:

- #38071

⚠️ **Note:** [38071](#38071) needs to be merged and shipped first! Only then we can merge this PR.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID] [FIXED] - ANR when having an inverted FlatList on android API 33+

Pull Request resolved: #38073

Test Plan:
- Check the RN tester app and see that scrollview is still working as expected
- Add the `internalAndroidApplyInvertedFix` prop as test to a scrollview and see how the scrollbar will change position.

Reviewed By: cortinico

Differential Revision: D47848063

Pulled By: NickGerleman

fbshipit-source-id: 4a6948a8b89f0b39f01b7a2d44dba740c53fabb3
billnbell pushed a commit to billnbell/react-native that referenced this pull request Jul 29, 2023
Summary:
This PR is a result of this PR, which got merged but then reverted:

- facebook#37913

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

As explained in the issue, 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, using the newly added `isInvertedVirtualizedList` prop.

This is a follow up PR to:

- facebook#38071

⚠️ **Note:** [38071](facebook#38071) needs to be merged and shipped first! Only then we can merge this PR.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID] [FIXED] - ANR when having an inverted FlatList on android API 33+

Pull Request resolved: facebook#38073

Test Plan:
- Check the RN tester app and see that scrollview is still working as expected
- Add the `internalAndroidApplyInvertedFix` prop as test to a scrollview and see how the scrollbar will change position.

Reviewed By: cortinico

Differential Revision: D47848063

Pulled By: NickGerleman

fbshipit-source-id: 4a6948a8b89f0b39f01b7a2d44dba740c53fabb3
billnbell pushed a commit to billnbell/react-native that referenced this pull request Jul 29, 2023
Summary:
This PR is a result of this PR, which got merged but then reverted:

- facebook#37913

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

As explained in the issue, 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, using the newly added `isInvertedVirtualizedList` prop.

This is a follow up PR to:

- facebook#38071

⚠️ **Note:** [38071](facebook#38071) needs to be merged and shipped first! Only then we can merge this PR.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID] [FIXED] - ANR when having an inverted FlatList on android API 33+

Pull Request resolved: facebook#38073

Test Plan:
- Check the RN tester app and see that scrollview is still working as expected
- Add the `internalAndroidApplyInvertedFix` prop as test to a scrollview and see how the scrollbar will change position.

Reviewed By: cortinico

Differential Revision: D47848063

Pulled By: NickGerleman

fbshipit-source-id: 4a6948a8b89f0b39f01b7a2d44dba740c53fabb3
billnbell pushed a commit to billnbell/react-native that referenced this pull request Jul 29, 2023
Summary:
This PR is a result of this PR, which got merged but then reverted:

- facebook#37913

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

As explained in the issue, 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, using the newly added `isInvertedVirtualizedList` prop.

This is a follow up PR to:

- facebook#38071

⚠️ **Note:** [38071](facebook#38071) needs to be merged and shipped first! Only then we can merge this PR.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID] [FIXED] - ANR when having an inverted FlatList on android API 33+

Pull Request resolved: facebook#38073

Test Plan:
- Check the RN tester app and see that scrollview is still working as expected
- Add the `internalAndroidApplyInvertedFix` prop as test to a scrollview and see how the scrollbar will change position.

Reviewed By: cortinico

Differential Revision: D47848063

Pulled By: NickGerleman

fbshipit-source-id: 4a6948a8b89f0b39f01b7a2d44dba740c53fabb3
zhxlp added a commit to zhxlp/react-native that referenced this pull request Aug 1, 2023
…book#38071)

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

- facebook#37913

We are trying to implement a workaround for facebook#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 facebook#37913

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

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

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

Pull Request resolved: facebook#38071

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.

Reviewed By: rozele

Differential Revision: D47062200

Pulled By: NickGerleman

fbshipit-source-id: d20eebeec757d9aaeced8561f53556bbb4a492e4
lunaleaps pushed a commit that referenced this pull request Aug 7, 2023
Summary:
This PR is a result of this PR, which got merged but then reverted:

- #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 #37913

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

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

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

Pull Request resolved: #38071

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.

Reviewed By: rozele

Differential Revision: D47062200

Pulled By: NickGerleman

fbshipit-source-id: d20eebeec757d9aaeced8561f53556bbb4a492e4
lunaleaps pushed a commit that referenced this pull request Aug 7, 2023
Summary:
This PR is a result of this PR, which got merged but then reverted:

- #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.

As explained in the issue, 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, using the newly added `isInvertedVirtualizedList` prop.

This is a follow up PR to:

- #38071

⚠️ **Note:** [38071](#38071) needs to be merged and shipped first! Only then we can merge this PR.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID] [FIXED] - ANR when having an inverted FlatList on android API 33+

Pull Request resolved: #38073

Test Plan:
- Check the RN tester app and see that scrollview is still working as expected
- Add the `internalAndroidApplyInvertedFix` prop as test to a scrollview and see how the scrollbar will change position.

Reviewed By: cortinico

Differential Revision: D47848063

Pulled By: NickGerleman

fbshipit-source-id: 4a6948a8b89f0b39f01b7a2d44dba740c53fabb3
Kudo pushed a commit to expo/react-native that referenced this pull request Aug 21, 2023
…book#38071)

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

- facebook#37913

We are trying to implement a workaround for facebook#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 facebook#37913

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

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

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

Pull Request resolved: facebook#38071

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.

Reviewed By: rozele

Differential Revision: D47062200

Pulled By: NickGerleman

fbshipit-source-id: d20eebeec757d9aaeced8561f53556bbb4a492e4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants