Skip to content

Fix scrollview momentum not stopping on scrollTo/scrollToEnd for horizontal scrollviews - #39529

Closed
Almouro wants to merge 2 commits into
react:mainfrom
Almouro:fix/horizontal-abort-animation
Closed

Fix scrollview momentum not stopping on scrollTo/scrollToEnd for horizontal scrollviews#39529
Almouro wants to merge 2 commits into
react:mainfrom
Almouro:fix/horizontal-abort-animation

Conversation

@Almouro

@AlmouroAlmouro commented Sep 19, 2023

Copy link
Copy Markdown
Contributor

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

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-workinghorizontal-scrolloffset-bughorizontal-scrolloffset-fixed

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

Test Plan:

My test code is this
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 layoutMethodNot animatedAnimated
horizontalscrollTohorizontal-scrollTo-nohorizontal-scrollto-yes
horizontalscrollToEndhorizontal-scrolltoend-nohorizontal-scrolltoend-yes
verticalscrollTovertical-scrollto-novertical-scrollto-yes
verticalscrollToEndvertical-scrolltoend-novertical-scrolltoend-yes

@facebook-github-botfacebook-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
@analysis-bot

analysis-bot commented Sep 19, 2023

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,333,652-31
androidhermesarmeabi-v7an/a--
androidhermesx86n/a--
androidhermesx86_64n/a--
androidjscarm64-v8a9,573,367+87
androidjscarmeabi-v7an/a--
androidjscx86n/a--
androidjscx86_64n/a--

Base commit: 3b5bea0
Branch: main

@github-actions

github-actionsBot commented Sep 19, 2023

Copy link
Copy Markdown
Warnings
⚠️One hour and a half have passed and the E2E jobs haven't finished yet.

Generated by 🚫 dangerJS against 6b6c066

@lunaleaps

Copy link
Copy Markdown
Contributor

This makes sense to me! Is there something we can add to RNTester to showcase the behavior?

@lunaleapslunaleaps self-assigned this Sep 19, 2023
@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@Almouro

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps I'm realizing there was already an example for this inside the RN Tester, I hadn't thought to check 😅

Before fix ❌After fix ✅
rntester-beforefixrntester-afterfix

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label Sep 20, 2023
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@ryancat merged this pull request in 2f86aaf.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Shared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Almouro@analysis-bot@lunaleaps@facebook-github-bot