-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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 on scrollTo/scrollToEnd for horizontal scrollviews #39529
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Shared with Meta
Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
labels
Sep 19, 2023
Base commit: 3b5bea0 |
This makes sense to me! Is there something we can add to RNTester to showcase the behavior? |
@ryancat has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@lunaleaps I'm realizing there was already an example for this inside the RN Tester, I hadn't thought to check 😅
|
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.
Shared with Meta
Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Motivation
My main motivation for this is using nested horizontal
Flashlists
inside a verticalFlashlist
.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: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 momentumThe bug
When scrolling and calling
scrollTo
, scroll momentum should be stopped and we should scroll to wherescrollTo
asked for.All credit goes to @tomekzaw for #36104 who fixed it for vertical scrollviews
I realized we had the same issue for
scrollToEnd
Based on #38728 I kept all those calls to
abortAnimation
on the View ManagerChangelog:
[ANDROID] [FIXED] - Fixed horizontal ScrollView momentum not stopping when calling scrollTo programmatically
[ANDROID] [FIXED] - Fixed ScrollView momentum not stopping when calling scrollToEnd programmatically
Test Plan:
My test code is this
Basically:
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 pastedpackages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll
folder from this branch to try out my fixesThen tested the following scenarios on Android: