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

Fix ScrollView momentum not stopping when calling scrollTo programmatically #36104

Closed
wants to merge 1 commit into from
Closed

Conversation

tomekzaw
Copy link
Contributor

@tomekzaw tomekzaw commented Feb 9, 2023

Summary

Fixes #32235.

See #32235 (comment) for details.

Here's what I did:

  1. Set a breakpoint in method scrollTo in class android.view.View (make sure the API level of your emulator matches compileSdkVersion so the sources are aligned with the bytecode)
  2. Try to scroll the ScrollView, make sure the breakpoint is hit, continue execution
  3. Add condition y == 0 to the breakpoint so it doesn't pause execution when scrolling
  4. Scroll the ScrollView again so that it still scrolls with its momentum
  5. Click the button that calls scrollTo command with y = 0
  6. Breakpoint should be hit, see ReactScrollViewCommandHelper in the stack trace
  7. Remove condition y == 0 from the breakpoint, continue execution
  8. Breakpoint is hit again with y != 0 (in my case 49153 or something) – this is incorrect behavior as it overwrites the effect of the previous call
  9. Notice the stack trace goes through a series of native calls, probably there's some mechanism that kicks in here
  10. Add mScroller.abortAnimation(); just out of curiosity
  11. It actually works!
  12. Submit this PR

Before:

recording.mov

After:

after.mov

Changelog

[ANDROID] [FIXED] - Fixed ScrollView momentum not stopping when calling scrollTo programmatically

Test Plan

Reproducer: https://github.com/tomekzaw/Issue32235/blob/master/App.tsx

@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 Feb 9, 2023
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,475,963 +62
android hermes armeabi-v7a 7,797,634 +69
android hermes x86 8,952,068 +68
android hermes x86_64 8,809,790 +69
android jsc arm64-v8a 9,113,628 -51
android jsc armeabi-v7a 8,310,507 -40
android jsc x86 9,165,293 -51
android jsc x86_64 9,424,096 -42

Base commit: 6d1667c
Branch: main

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Feb 9, 2023
@facebook-github-bot
Copy link
Contributor

@cortinico merged this pull request in 681b35d.

@tomekzaw tomekzaw deleted the patch-2 branch February 10, 2023 07:41
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…ically (facebook#36104)

Summary:
Fixes facebook#32235.

See facebook#32235 (comment) for details.

Before:

https://user-images.githubusercontent.com/20516055/217268275-7ec9a228-bbd6-4294-aa1f-a43c4268984c.mov

After:

https://user-images.githubusercontent.com/20516055/217786242-f44b008f-6c6d-4f11-a7bd-b7a01150f3fb.mov

## Changelog

[ANDROID] [FIXED] - Fixed ScrollView momentum not stopping when calling scrollTo programmatically

Pull Request resolved: facebook#36104

Test Plan: Reproducer: https://github.com/tomekzaw/Issue32235/blob/master/App.tsx

Reviewed By: christophpurrer

Differential Revision: D43153500

Pulled By: cortinico

fbshipit-source-id: ac9c5ed754ed8ba72fe45d506c76f52d795dc83e
facebook-github-bot pushed a commit that referenced this pull request Aug 2, 2023
…inside a KeyboardAvoidingView (#38728)

Summary:
Starting from RN 0.72.0, when we nest a ScrollView inside a KeyboardAvoidingView, the ScrollView doesn't respond properly to the Keyboard on Android.

https://github.com/facebook/react-native/assets/32062066/a62b5a42-6817-4093-91a2-7cc9e4a315bb

This issue is due to a change made in #36104, which was added to fix #32235.

That commit changed this line of code to abort the Scroller animation if a new call to the `scrollTo` method was made:

https://github.com/facebook/react-native/blob/aab52859a447a8257b106fe307008af218322e3d/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java#L1073

Apparently, this is the same method that scrolls the ScrollView in response to the Keyboard opening on Android.

So, here comes my proposal for a fix that doesn't break #36104 and fixes #38152.

When we open the Keyboard, the call stack is as follows:
- InputMethodManager
- AndroidIME
- InsetsController
- `ReactScrollView.scrollTo` gets called

When we use the ScrollView method `scrollTo` directly from the UI, the call stack is different as it goes through:
- ReactScrollViewCommandHelper
- ReactScrollViewManager
- `ReactScrollView.scrollTo` gets called

We can move `mScroller.abortAnimation();` from `ReactScrollView.scrollTo` to the `ReactScrollViewManager.scrollTo` method so that it gets called only when we call `scrollTo` from the UI and not when the `scrollTo` method is called by other sources.

https://github.com/facebook/react-native/assets/32062066/9c10ded3-08e5-48e0-9a85-0987d62de011

## Changelog:

[ANDROID] [FIXED] - Fixed ScrollView not responding to Keyboard events when nested inside a KeyboardAvoidingView

Pull Request resolved: #38728

Test Plan: You can see the issue and the proposed fixes in this repo: [kav-test-android](https://github.com/andreacassani/kav-test-android). Please refer to the `kav_test_fix` folder and to the [readme](https://github.com/andreacassani/kav-test-android/blob/main/README.md).

Reviewed By: NickGerleman

Differential Revision: D47972445

Pulled By: ryancat

fbshipit-source-id: e58758d4b3d5318b947b42a88a56ad6ae69a539c
lunaleaps pushed a commit that referenced this pull request Aug 7, 2023
…inside a KeyboardAvoidingView (#38728)

Summary:
Starting from RN 0.72.0, when we nest a ScrollView inside a KeyboardAvoidingView, the ScrollView doesn't respond properly to the Keyboard on Android.

https://github.com/facebook/react-native/assets/32062066/a62b5a42-6817-4093-91a2-7cc9e4a315bb

This issue is due to a change made in #36104, which was added to fix #32235.

That commit changed this line of code to abort the Scroller animation if a new call to the `scrollTo` method was made:

https://github.com/facebook/react-native/blob/aab52859a447a8257b106fe307008af218322e3d/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java#L1073

Apparently, this is the same method that scrolls the ScrollView in response to the Keyboard opening on Android.

So, here comes my proposal for a fix that doesn't break #36104 and fixes #38152.

When we open the Keyboard, the call stack is as follows:
- InputMethodManager
- AndroidIME
- InsetsController
- `ReactScrollView.scrollTo` gets called

When we use the ScrollView method `scrollTo` directly from the UI, the call stack is different as it goes through:
- ReactScrollViewCommandHelper
- ReactScrollViewManager
- `ReactScrollView.scrollTo` gets called

We can move `mScroller.abortAnimation();` from `ReactScrollView.scrollTo` to the `ReactScrollViewManager.scrollTo` method so that it gets called only when we call `scrollTo` from the UI and not when the `scrollTo` method is called by other sources.

https://github.com/facebook/react-native/assets/32062066/9c10ded3-08e5-48e0-9a85-0987d62de011

## Changelog:

[ANDROID] [FIXED] - Fixed ScrollView not responding to Keyboard events when nested inside a KeyboardAvoidingView

Pull Request resolved: #38728

Test Plan: You can see the issue and the proposed fixes in this repo: [kav-test-android](https://github.com/andreacassani/kav-test-android). Please refer to the `kav_test_fix` folder and to the [readme](https://github.com/andreacassani/kav-test-android/blob/main/README.md).

Reviewed By: NickGerleman

Differential Revision: D47972445

Pulled By: ryancat

fbshipit-source-id: e58758d4b3d5318b947b42a88a56ad6ae69a539c
Kudo pushed a commit to expo/react-native that referenced this pull request Aug 21, 2023
…inside a KeyboardAvoidingView (facebook#38728)

Summary:
Starting from RN 0.72.0, when we nest a ScrollView inside a KeyboardAvoidingView, the ScrollView doesn't respond properly to the Keyboard on Android.

https://github.com/facebook/react-native/assets/32062066/a62b5a42-6817-4093-91a2-7cc9e4a315bb

This issue is due to a change made in facebook#36104, which was added to fix facebook#32235.

That commit changed this line of code to abort the Scroller animation if a new call to the `scrollTo` method was made:

https://github.com/facebook/react-native/blob/aab52859a447a8257b106fe307008af218322e3d/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java#L1073

Apparently, this is the same method that scrolls the ScrollView in response to the Keyboard opening on Android.

So, here comes my proposal for a fix that doesn't break facebook#36104 and fixes facebook#38152.

When we open the Keyboard, the call stack is as follows:
- InputMethodManager
- AndroidIME
- InsetsController
- `ReactScrollView.scrollTo` gets called

When we use the ScrollView method `scrollTo` directly from the UI, the call stack is different as it goes through:
- ReactScrollViewCommandHelper
- ReactScrollViewManager
- `ReactScrollView.scrollTo` gets called

We can move `mScroller.abortAnimation();` from `ReactScrollView.scrollTo` to the `ReactScrollViewManager.scrollTo` method so that it gets called only when we call `scrollTo` from the UI and not when the `scrollTo` method is called by other sources.

https://github.com/facebook/react-native/assets/32062066/9c10ded3-08e5-48e0-9a85-0987d62de011

## Changelog:

[ANDROID] [FIXED] - Fixed ScrollView not responding to Keyboard events when nested inside a KeyboardAvoidingView

Pull Request resolved: facebook#38728

Test Plan: You can see the issue and the proposed fixes in this repo: [kav-test-android](https://github.com/andreacassani/kav-test-android). Please refer to the `kav_test_fix` folder and to the [readme](https://github.com/andreacassani/kav-test-android/blob/main/README.md).

Reviewed By: NickGerleman

Differential Revision: D47972445

Pulled By: ryancat

fbshipit-source-id: e58758d4b3d5318b947b42a88a56ad6ae69a539c
facebook-github-bot pushed a commit that referenced this pull request Sep 20, 2023
…zontal scrollviews (#39529)

Summary:
### Motivation
My main motivation for this is using nested horizontal `Flashlists` inside a vertical `Flashlist`.
Like a `RecyclerView`, since my horizontal lists get recycled, if I scroll say the first horizontal list and scroll down, then when this list gets recycled it continues scrolling even if the content is new:

![nested-list-recycling](https://github.com/facebook/react-native/assets/4534323/f75a2a9b-6ab5-4554-811f-8d85ddfb81de)

To handle this, I want to call `scrollTo` everytime a new row appears to reset the scroll offset, however I've realized this doesn't stop the scroll momentum

### The bug

When scrolling and calling `scrollTo`, scroll momentum should be stopped and we should scroll to where `scrollTo` asked for.
All credit goes to tomekzaw for #36104 who fixed it for vertical scrollviews

I realized we had the same issue for
- horizontal scroll views
- when calling `scrollToEnd`

| Vertical scrollview (working ✅) | Horizontal scrollview (before fix, not stopping ❌) | Horizontal scrollview (after fix ✅)|
|--------|--------|--|
| ![vertical-scrolltoffset-working](https://github.com/facebook/react-native/assets/4534323/884d95ce-9b09-47aa-99a7-99ca579a8fc4)|![horizontal-scrolloffset-bug](https://github.com/facebook/react-native/assets/4534323/0065a9d0-f905-4354-9caa-29a0a1f914ba)|![horizontal-scrolloffset-fixed](https://github.com/facebook/react-native/assets/4534323/ab49c356-23e8-464a-83a9-c4632b9d673e)|

Based on  #38728 I kept all those calls to `abortAnimation` on the View Manager

## Changelog:

[ANDROID] [FIXED] - Fixed horizontal ScrollView momentum not stopping when calling scrollTo programmatically
[ANDROID] [FIXED] - Fixed ScrollView momentum not stopping when calling scrollToEnd programmatically

Pull Request resolved: #39529

Test Plan:
My test code is [this](https://gist.github.com/Almouro/3ffcfbda8e2239e64bee93985e243000)
Basically:
- a scrollview with a few elements
- some buttons to trigger a `scrollTo`

To reproduce the bug, I scroll then click one of the buttons triggering a `scrollTo`

I added `react-native@nightly` to my project, and copy pasted `packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll` folder from this branch to try out my fixes

Then tested the following scenarios on Android:

| List layout  | Method | Not animated | Animated|
|--------|--------|--|--|
| horizontal | scrollTo | ![horizontal-scrollTo-no](https://github.com/facebook/react-native/assets/4534323/bda87a19-43cf-4fe9-9340-f70a3d5cd6a4)| ![horizontal-scrollto-yes](https://github.com/facebook/react-native/assets/4534323/8628d7ff-ee28-4185-81d1-1783f0fd990f) |
| horizontal | scrollToEnd  |![horizontal-scrolltoend-no](https://github.com/facebook/react-native/assets/4534323/594bb539-cb27-4234-8a8f-74d5b2bbe25f) | ![horizontal-scrolltoend-yes](https://github.com/facebook/react-native/assets/4534323/7366abcd-95fb-42ab-89e1-38fd4b10d966)|
| vertical | scrollTo | ![vertical-scrollto-no](https://github.com/facebook/react-native/assets/4534323/54555250-d4df-4bc2-b20f-46c706e8c726)|![vertical-scrollto-yes](https://github.com/facebook/react-native/assets/4534323/0c109f81-0bbd-475b-90f1-f1467317c799) |
| vertical | scrollToEnd  | ![vertical-scrolltoend-no](https://github.com/facebook/react-native/assets/4534323/f48c8196-8f2f-4d98-b750-91c067d1a063) | ![vertical-scrolltoend-yes](https://github.com/facebook/react-native/assets/4534323/04bb06dc-7e20-40de-a40d-e1da1fec491a)|

Reviewed By: javache

Differential Revision: D49437886

Pulled By: ryancat

fbshipit-source-id: 358c9b0eed7dabcbc9b87a15d1a757b414ef514b
ShevO27 pushed a commit to ShevO27/react-native that referenced this pull request Sep 26, 2023
…zontal scrollviews (facebook#39529)

Summary:
### Motivation
My main motivation for this is using nested horizontal `Flashlists` inside a vertical `Flashlist`.
Like a `RecyclerView`, since my horizontal lists get recycled, if I scroll say the first horizontal list and scroll down, then when this list gets recycled it continues scrolling even if the content is new:

![nested-list-recycling](https://github.com/facebook/react-native/assets/4534323/f75a2a9b-6ab5-4554-811f-8d85ddfb81de)

To handle this, I want to call `scrollTo` everytime a new row appears to reset the scroll offset, however I've realized this doesn't stop the scroll momentum

### The bug

When scrolling and calling `scrollTo`, scroll momentum should be stopped and we should scroll to where `scrollTo` asked for.
All credit goes to tomekzaw for facebook#36104 who fixed it for vertical scrollviews

I realized we had the same issue for
- horizontal scroll views
- when calling `scrollToEnd`

| Vertical scrollview (working ✅) | Horizontal scrollview (before fix, not stopping ❌) | Horizontal scrollview (after fix ✅)|
|--------|--------|--|
| ![vertical-scrolltoffset-working](https://github.com/facebook/react-native/assets/4534323/884d95ce-9b09-47aa-99a7-99ca579a8fc4)|![horizontal-scrolloffset-bug](https://github.com/facebook/react-native/assets/4534323/0065a9d0-f905-4354-9caa-29a0a1f914ba)|![horizontal-scrolloffset-fixed](https://github.com/facebook/react-native/assets/4534323/ab49c356-23e8-464a-83a9-c4632b9d673e)|

Based on  facebook#38728 I kept all those calls to `abortAnimation` on the View Manager

## Changelog:

[ANDROID] [FIXED] - Fixed horizontal ScrollView momentum not stopping when calling scrollTo programmatically
[ANDROID] [FIXED] - Fixed ScrollView momentum not stopping when calling scrollToEnd programmatically

Pull Request resolved: facebook#39529

Test Plan:
My test code is [this](https://gist.github.com/Almouro/3ffcfbda8e2239e64bee93985e243000)
Basically:
- a scrollview with a few elements
- some buttons to trigger a `scrollTo`

To reproduce the bug, I scroll then click one of the buttons triggering a `scrollTo`

I added `react-native@nightly` to my project, and copy pasted `packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll` folder from this branch to try out my fixes

Then tested the following scenarios on Android:

| List layout  | Method | Not animated | Animated|
|--------|--------|--|--|
| horizontal | scrollTo | ![horizontal-scrollTo-no](https://github.com/facebook/react-native/assets/4534323/bda87a19-43cf-4fe9-9340-f70a3d5cd6a4)| ![horizontal-scrollto-yes](https://github.com/facebook/react-native/assets/4534323/8628d7ff-ee28-4185-81d1-1783f0fd990f) |
| horizontal | scrollToEnd  |![horizontal-scrolltoend-no](https://github.com/facebook/react-native/assets/4534323/594bb539-cb27-4234-8a8f-74d5b2bbe25f) | ![horizontal-scrolltoend-yes](https://github.com/facebook/react-native/assets/4534323/7366abcd-95fb-42ab-89e1-38fd4b10d966)|
| vertical | scrollTo | ![vertical-scrollto-no](https://github.com/facebook/react-native/assets/4534323/54555250-d4df-4bc2-b20f-46c706e8c726)|![vertical-scrollto-yes](https://github.com/facebook/react-native/assets/4534323/0c109f81-0bbd-475b-90f1-f1467317c799) |
| vertical | scrollToEnd  | ![vertical-scrolltoend-no](https://github.com/facebook/react-native/assets/4534323/f48c8196-8f2f-4d98-b750-91c067d1a063) | ![vertical-scrolltoend-yes](https://github.com/facebook/react-native/assets/4534323/04bb06dc-7e20-40de-a40d-e1da1fec491a)|

Reviewed By: javache

Differential Revision: D49437886

Pulled By: ryancat

fbshipit-source-id: 358c9b0eed7dabcbc9b87a15d1a757b414ef514b
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.

[Android] ScrollView momentum not stopping when calling scrollTo programmatically
3 participants