[Android] Fix onMomentumScrollEnd similar to iOS #45187

Biki-das · 2024-06-26T15:04:32Z

Summary:

in iOS on a scroll generated programatically, the onMomentScrollEnd is fired, though in case of android the same does not happen, this PR tries to implement the same behaviour for android as well, while diving through the code it seems we have two extra onMomentumScrollEnd events. Only one event should be fired.

iOS Behaviour on Programmatic Scroll

Screen.Recording.2024-06-26.at.8.18.46.PM.mov

Screen.Recording.2024-06-26.at.8.50.00.PM.mov

Android Behaviour on Programmatic Scroll

Screen.Recording.2024-06-26.at.8.19.28.PM.mov

Screen.Recording.2024-06-26.at.8.51.15.PM.mov

If closely observed we can see the onMomentumScrollEnd does not gets called in Android unlike to iOS.

Changelog:

[Android] [Fixed] - fix onMomentum scroll end similar to iOS

Test Plan:

i have added updates to the FlatList example and ScrollViewSimple
here is a ScreenRecording of onMomentumScrollEnd firing in android after the code changes

Screen.Recording.2024-06-26.at.8.12.08.PM.mov

Screen.Recording.2024-06-26.at.8.41.55.PM.mov

Biki-das · 2024-06-26T15:07:58Z

cc @NickGerleman @javache

Biki-das · 2024-06-26T15:17:03Z

@cipolleschi would also love to know your thoughts as well.

analysis-bot · 2024-06-26T15:42:02Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	20,300,225	-15,772
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	23,497,181	-16,121
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: e6dd44d
Branch: main

cipolleschi · 2024-06-27T12:37:55Z

Android is not really my area of expertise, so @javache and @cortinico would be better reviewers.

A couple of points though:

The 2 different events could be there because of Old Arch/New Arch variants
The Android code in the PR seems to tackle only the Old Architecture. Could you see if the New Architecture has the same problem? We are hyper focused on the New Arch, so Old Arch-only fixes are lower priorities for us.

cortinico · 2024-07-04T08:34:06Z

The Android code in the PR seems to tackle only the Old Architecture. Could you see if the New Architecture has the same problem? We are hyper focused on the New Arch, so Old Arch-only fixes are lower priorities for us.

Yup basically this.
The PR looks quite involved so I'm unsure we have capacity to review/import it at the moment unless is a New Architecture regression @Biki-das

Abbondanzo · 2024-11-05T15:31:39Z

This looks like an issue with both New Architecture and Old (bridgeless mode is enabled from screen captures). I'm happy to work with you @Biki-das to import and land these changes since this is a gap in platforms we'd like to close

Biki-das · 2024-11-05T16:54:44Z

@Abbondanzo happy to coordinate to land this, let me know how can i help 😃

Abbondanzo · 2024-11-05T17:01:47Z

@Abbondanzo happy to coordinate to land this, let me know how can i help 😃

Awesome! Would you be able to rebase and convert your changes to Kotlin? Both ReactScrollView and ReactHorizontalScrollView have been converted to Kotlin recently. I can import after that 😄

Biki-das · 2024-11-05T17:08:33Z

@Abbondanzo happy to coordinate to land this, let me know how can i help 😃

Awesome! Would you be able to rebase and convert your changes to Kotlin? Both ReactScrollView and ReactHorizontalScrollView have been converted to Kotlin recently. I can import after that 😄

Cool,i would try to do the same tomorrow, its been a long time of the code, need to check it back and forth, thanks for your help, seeking to land this.

Biki-das · 2024-11-06T15:26:35Z

@Abbondanzo ? were the files you mentioned converted to Kotlin, as i rebased it to the latest main,? am i doing something wrong?
As far as i can see Both ReactScrollView and ReactHorizontalScrollView are still .java files.

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

https://github.com/facebook/react-native/blob/main/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java

Abbondanzo · 2024-11-06T15:34:51Z

@Abbondanzo ? was this file converted to Kotlin, as i rebased it to the latest main,? am i doing something wrong?

Ah, nope, my mistake! It hasn't been converted to Kotlin yet. Saw someone else working on a Kotlin conversion but that hasn't merged yet, so no need to wait

facebook-github-bot · 2024-11-06T15:35:52Z

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

Biki-das · 2024-11-06T16:08:33Z

i see an internal linter fail action, obviously only meta employees are allowed to seek that, @Abbondanzo could you check and confirm the same.

Abbondanzo

i see an internal linter fail action, obviously only meta employees are allowed to seek that, @Abbondanzo could you check and confirm the same.

Yep, that's an easy fix on our end. I pulled this in and gave it a test. I've got a few change requests first but otherwise this is looking stellar!

Abbondanzo · 2024-11-06T16:12:59Z

...ve/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java

@@ -926,6 +927,7 @@ private void handlePostTouchScrolling(int velocityX, int velocityY) {
    }

    if (mSendMomentumEvents) {
+      enableFpsListener();


Could these tweaks to the FPS listener be removed? Event dispatching for touch events shouldn't be changing

shall i just remove them as far as i can remember they are for per monitoring.

I would restore the original behavior here, removing modifications of the enable/disableFpsListener methods from this method

Abbondanzo · 2024-11-06T16:47:32Z

...tAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollViewManager.java

    } else {
      scrollView.scrollTo(data.mDestX, data.mDestY);
+      ReactScrollViewHelper.emitScrollMomentumEndEvent(scrollView);


While iOS does this for non-animated views, this is a bug on the iOS side that we should avoid introducing into Android for the sake of parity. The onMomentumScrollEnd event should only fire after onMomentumScrollBegin, driven by a user-activated fling or animated scrollTo

Abbondanzo · 2024-11-06T16:48:33Z

...react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java

@@ -733,7 +734,6 @@ public void run() {
                mPostTouchRunnable = null;
                if (mSendMomentumEvents) {
                  ReactScrollViewHelper.emitScrollMomentumEndEvent(ReactScrollView.this);
-                }


mSendMomentumEvents should only be used to guard from emitting events to JS, the NativeAnimatedModule should still receive scroll end here regardless (and FPS listener disabled)

? could you brief this up and what should i do, i seems to have lost context of the code.

The brace should be added back here and the if block should only contain the event emit, not the few lines that follow it.

...ve/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java

...react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java

Biki-das · 2024-11-06T17:14:40Z

thanks for the review, will need to take a look and remember since its been a while i wrote the fixes.

Biki-das

@Abbondanzo

Have introduced a few changes

A boolean flag mMomentumScrollBeginFired is introduced to track whether onMomentumScrollBegin has been fired.
The handleSmoothScrollMomentumEvents method checks this flag before emitting the onMomentumScrollEnd event.
The fling method is overridden to set this flag to true when onMomentumScrollBegin is fired.

Biki-das · 2024-11-06T17:39:12Z

...ve/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java

@@ -926,6 +927,7 @@ private void handlePostTouchScrolling(int velocityX, int velocityY) {
    }

    if (mSendMomentumEvents) {
+      enableFpsListener();


shall i just remove them as far as i can remember they are for per monitoring.

…react/views/scroll/ReactScrollView.java Co-authored-by: Peter Abbondanzo <peter@abbondanzo.com>

Biki-das · 2024-11-06T18:56:35Z

Oops i see we have a fling method already, i have losed a lot of context with the code actually, so any extra help is appreciated on the requests raised.

Abbondanzo · 2024-11-06T21:50:27Z

As a follow-up, I've made two modifications to iOS that should fix the way scroll momentum events are fired:
#47468
#47471

Abbondanzo · 2024-11-06T21:52:25Z

Oops i see we have a fling method already, i have losed a lot of context with the code actually, so any extra help is appreciated on the requests raised.

I can reimport this PR in its current state and make a few modifications internally. If you want to reintroduce FPS listener stuffs, it can go in a separate PR if you're okay with that.

facebook-github-bot · 2024-11-07T15:24:11Z

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

Biki-das · 2024-11-07T15:56:08Z

@Abbondanzo sorry for the late reply, sure that works , we can introduce the FPS listener stuff on a seperate PR mentioning the same PR, so we can keep track of the same rest i shall revert it back to where we did not had the fling override? i see you imported the PR again so let me know how can i make this easy for you to make modifications internally.

Abbondanzo · 2024-11-07T16:11:57Z

@Abbondanzo i shall revert it back to where we did not had the fling override? i see you imported the PR again so let me know how can i make this easy for you to make modifications internally.

@Biki-das It's okay as-is. After taking a closer look into how we perform animations for smooth scrolling, I'm exploring moving this all into ReactScrollViewHelper. More specifically, I think we can just emit momentum scroll events from the animation listener here:

react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewHelper.kt

Lines 392 to 410 in 0ee963e

    
                     object : Animator.AnimatorListener { 
        
                       override fun onAnimationStart(animator: Animator) { 
        
                         val scrollState = scrollView.reactScrollViewScrollState 
        
                         scrollState.isCanceled = false 
        
                         scrollState.isFinished = false 
        
                       } 
        
                       override fun onAnimationEnd(animator: Animator) { 
        
                         scrollView.reactScrollViewScrollState.isFinished = true 
        
                         updateFabricScrollState<T>(scrollView) 
        
                       } 
        
                       override fun onAnimationCancel(animator: Animator) { 
        
                         scrollView.reactScrollViewScrollState.isCanceled = true 
        
                       } 
        
                       override fun onAnimationRepeat(animator: Animator) = Unit 
        
                     }) 
        
           }

This way, we need not maintain a runnable in each scroll view class and we don't have to manually invoke the smooth scroll helper in each callsite. Also, it ensures momentum scroll events are properly fired for scroll views that use pagination (which privately invoke smooth scroll via fling under the hood).

Biki-das · 2024-11-07T16:17:30Z

@Abbondanzo i shall revert it back to where we did not had the fling override? i see you imported the PR again so let me know how can i make this easy for you to make modifications internally.

@Biki-das It's okay as-is. After taking a closer look into how we perform animations for smooth scrolling, I'm exploring moving this all into ReactScrollViewHelper. More specifically, I think we can just emit momentum scroll events from the animation listener here:

react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewHelper.kt

Lines 392 to 410 in 0ee963e

object : Animator.AnimatorListener {

override fun onAnimationStart(animator: Animator) {

val scrollState = scrollView.reactScrollViewScrollState

scrollState.isCanceled = false

scrollState.isFinished = false

}

override fun onAnimationEnd(animator: Animator) {

scrollView.reactScrollViewScrollState.isFinished = true

updateFabricScrollState<T>(scrollView)

}

override fun onAnimationCancel(animator: Animator) {

scrollView.reactScrollViewScrollState.isCanceled = true

}

override fun onAnimationRepeat(animator: Animator) = Unit

})

}

This way, we need not maintain a runnable in each scroll view class and we don't have to manually invoke the smooth scroll helper in each callsite. Also, it ensures momentum scroll events are properly fired for scroll views that use pagination (which privately invoke smooth scroll via fling under the hood).

wow, that's a good approach i see, By centralising this functionality, we can avoid duplication, improve consistency, and ensure momentum scroll events are properly fired even for pagination-based scroll views. I am happy to see this fix being landed, it bugged me off few months back on the product experience, trying to do a specific feature and not being able to do it, and i felt, this is not just limited to me, a lot of other devs would also get bitten of this, so thank you for helping us out there, means a lot ❤️.

facebook-github-bot · 2024-11-12T18:43:36Z

@Abbondanzo merged this pull request in c69e330.

react-native-bot · 2024-11-12T18:43:38Z

This pull request was successfully merged by @Biki-das in c69e330

^{When will my fix make it into a release? | How to file a pick request?}

Summary: in iOS on a scroll generated programatically, the `onMomentScrollEnd` is fired, though in case of android the same does not happen, this PR tries to implement the same behaviour for android as well, while diving through the code it seems we have two extra `onMomentumScrollEnd` events. Only one event should be fired. **iOS Behaviour on Programmatic Scroll** https://github.com/facebook/react-native/assets/72331432/fb8f16b1-4db6-49fe-83a1-a1c40bf49705 https://github.com/facebook/react-native/assets/72331432/9842f522-b616-4fb3-b197-40817f4aa9cb **Android Behaviour on Programmatic Scroll** https://github.com/facebook/react-native/assets/72331432/c24d3f06-4e2a-4bef-81af-d9227a3b1a4a https://github.com/facebook/react-native/assets/72331432/d4917843-730b-4bd7-90d9-33efb0f471a7 If closely observed we can see the `onMomentumScrollEnd` does not gets called in Android unlike to iOS. [Android][Fixed] - Dispatch onMomentumScrollEnd after programmatic scrolling Pull Request resolved: #45187 Test Plan: i have added updates to the FlatList example and ScrollViewSimple here is a ScreenRecording of `onMomentumScrollEnd` firing in android after the code changes https://github.com/facebook/react-native/assets/72331432/f036d1a5-6ebf-47ba-becd-4db98a406b15 https://github.com/facebook/react-native/assets/72331432/8c788c39-3392-4822-99c5-6e320398714b Reviewed By: javache Differential Revision: D65539724 Pulled By: Abbondanzo fbshipit-source-id: f3a5527ac5979f5ec0c6ae18d80fdc20c9c9c14b

react-native-bot · 2024-12-04T17:24:49Z

This pull request was successfully merged by @Biki-das in abbe117

^{When will my fix make it into a release? | How to file a pick request?}

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 Jun 26, 2024

Biki-das changed the title ~~[Android ]fix onMomentum scroll end similar to iOS~~ [Android ]fix onMomentumScrollEnd similar to iOS Jun 26, 2024

fix momenutm scroll end

0e382c3

Biki-das force-pushed the fix-android-momentumScroll branch from 109a3b5 to 0e382c3 Compare November 6, 2024 15:23

Abbondanzo requested changes Nov 6, 2024

View reviewed changes

ensured onMomentumScrollEnd event only fires after onMomentumScrollBegin

7e54bfb

Biki-das commented Nov 6, 2024

View reviewed changes

Biki-das and others added 2 commits November 6, 2024 23:55

Update packages/react-native/ReactAndroid/src/main/java/com/facebook/…

9eed625

…react/views/scroll/ReactScrollView.java Co-authored-by: Peter Abbondanzo <peter@abbondanzo.com>

Update packages/react-native/ReactAndroid/src/main/java/com/facebook/…

d7f8afc

…react/views/scroll/ReactScrollView.java Co-authored-by: Peter Abbondanzo <peter@abbondanzo.com>

Abbondanzo mentioned this pull request Nov 11, 2024

[FlatList] Android: onMomentumScrollEnd doesn't trigger when use scrollToOffset #21718

Closed

3 tasks

Abbondanzo changed the title ~~[Android ]fix onMomentumScrollEnd similar to iOS~~ [Android] Fix onMomentumScrollEnd similar to iOS Nov 11, 2024

dhruvbhatia mentioned this pull request Nov 11, 2024

onMomentumEnd function not firing in Animated FlashList on Android when using ref.current.scrollToIndex software-mansion/react-native-reanimated#6605

Open

facebook-github-bot closed this in c69e330 Nov 12, 2024

facebook-github-bot added the Merged This PR has been merged. label Nov 12, 2024

matinzd mentioned this pull request Nov 15, 2024

[0.76.2] Fix onMomentumScrollEnd similar to iOS reactwg/react-native-releases#627

Closed

cipolleschi mentioned this pull request Dec 9, 2024

[RN][Codegen] Better support filtering out non linked platforms #48183

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android] Fix onMomentumScrollEnd similar to iOS #45187

[Android] Fix onMomentumScrollEnd similar to iOS #45187

Biki-das commented Jun 26, 2024 •

edited

Loading

Biki-das commented Jun 26, 2024

Biki-das commented Jun 26, 2024

analysis-bot commented Jun 26, 2024

cipolleschi commented Jun 27, 2024

cortinico commented Jul 4, 2024

Abbondanzo commented Nov 5, 2024

Biki-das commented Nov 5, 2024

Abbondanzo commented Nov 5, 2024

Biki-das commented Nov 5, 2024

Biki-das commented Nov 6, 2024 •

edited

Loading

Abbondanzo commented Nov 6, 2024

facebook-github-bot commented Nov 6, 2024

Biki-das commented Nov 6, 2024

Abbondanzo left a comment

Abbondanzo Nov 6, 2024

Biki-das Nov 6, 2024

Abbondanzo Nov 6, 2024

Abbondanzo Nov 6, 2024

Abbondanzo Nov 6, 2024

Biki-das Nov 6, 2024

Abbondanzo Nov 6, 2024

Biki-das commented Nov 6, 2024

Biki-das left a comment

Biki-das Nov 6, 2024

Biki-das commented Nov 6, 2024

Abbondanzo commented Nov 6, 2024

Abbondanzo commented Nov 6, 2024

facebook-github-bot commented Nov 7, 2024

Biki-das commented Nov 7, 2024 •

edited

Loading

Abbondanzo commented Nov 7, 2024

Biki-das commented Nov 7, 2024 •

edited

Loading

facebook-github-bot commented Nov 12, 2024

react-native-bot commented Nov 12, 2024

react-native-bot commented Dec 4, 2024

[Android] Fix onMomentumScrollEnd similar to iOS #45187

[Android] Fix onMomentumScrollEnd similar to iOS #45187

Conversation

Biki-das commented Jun 26, 2024 • edited Loading

Summary:

Changelog:

Test Plan:

Biki-das commented Jun 26, 2024

Biki-das commented Jun 26, 2024

analysis-bot commented Jun 26, 2024

cipolleschi commented Jun 27, 2024

cortinico commented Jul 4, 2024

Abbondanzo commented Nov 5, 2024

Biki-das commented Nov 5, 2024

Abbondanzo commented Nov 5, 2024

Biki-das commented Nov 5, 2024

Biki-das commented Nov 6, 2024 • edited Loading

Abbondanzo commented Nov 6, 2024

facebook-github-bot commented Nov 6, 2024

Biki-das commented Nov 6, 2024

Abbondanzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Biki-das commented Nov 6, 2024

Biki-das left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Biki-das commented Nov 6, 2024

Abbondanzo commented Nov 6, 2024

Abbondanzo commented Nov 6, 2024

facebook-github-bot commented Nov 7, 2024

Biki-das commented Nov 7, 2024 • edited Loading

Abbondanzo commented Nov 7, 2024

Biki-das commented Nov 7, 2024 • edited Loading

facebook-github-bot commented Nov 12, 2024

react-native-bot commented Nov 12, 2024

react-native-bot commented Dec 4, 2024

Biki-das commented Jun 26, 2024 •

edited

Loading

Biki-das commented Nov 6, 2024 •

edited

Loading

Biki-das commented Nov 7, 2024 •

edited

Loading

Biki-das commented Nov 7, 2024 •

edited

Loading