[Android ]fix onMomentumScrollEnd similar to iOS

[Biki-das]

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.

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

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

[Biki-das]

cc @NickGerleman @javache

[Biki-das]

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

[analysis-bot]

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: e6dd44d6e5dca07d2fcd37eba7625fec75de7fd6 Branch: main

[cipolleschi]

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]

• 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]

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]

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

[Abbondanzo]

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

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

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

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

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

[Biki-das]

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

[Biki-das]

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

[Biki-das]

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]

As a follow-up, I've made two modifications to iOS that should fix the way scroll momentum events are fired: https://github.com/facebook/react-native/pull/47468 https://github.com/facebook/react-native/pull/47471

[Abbondanzo]

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]

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

[Biki-das]

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

@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: https://github.com/facebook/react-native/blob/0ee963ea65bcc88122044d51027511e611bde584/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewHelper.kt#L392-L410

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]

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

https://github.com/facebook/react-native/blob/0ee963ea65bcc88122044d51027511e611bde584/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewHelper.kt#L392-L410

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]

@Abbondanzo merged this pull request in facebook/react-native@c69e330324724a1e363f4786b2b03ee7ff4b35c5.

[react-native-bot]

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

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

[react-native-bot]

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

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