-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix useAnimatedScrollHandler
not intercepting momentum events on Android
#3948
Conversation
useAnimatedScrollHandler
not intercepting momentum eventsuseAnimatedScrollHandler
not intercepting momentum events on Android
1f2c5fe
to
40fdea1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested that it works perfect with my other fix here #4238
(smooth Scroll not working on Android)
Before | After |
Screen.Recording.2023-03-21.at.09.54.10.mov |
Screen.Recording.2023-03-21.at.09.53.39.mov |
@tomekzaw, is anything holding this PR back? I just stumbled across an issue where onMomentumEnd is not working correctly on Android, especially in cases where the scroll was triggered via ref and imperative functions. |
@tomekzaw I tried your approach, and it does fix the issue only when the scroll is made with a drag move. However, it doesn't trigger when I scroll via |
@hirbod I'm not particularly happy with adding a dummy listener (
I'm afraid this might be a separate problem. This PR already adds a dummy listener for momentum events so it should work the same way as in React Native. Perhaps we should install more listeners? |
Yeah, the dummy listener is not the greatest solution, but somehow acceptable for now. I guess you're right that there is another problem, but not having onMomentumScrollEnd triggered with the imperative scroll methods is a bummer :/ Haven't tested if thats the case with stock Scrollview as well, I would need to verify it. |
Seems to be working already 👍 |
Summary
Fixes #2735, see #2735 (comment) for details.
The main idea behind this PR is to enable emitting momentum events from the native side by enforcing
sendMomentumEvents
to be true.First, I came up with the following patch in
createAnimatedComponent.tsx
, however this doesn't fully resolve the issue assetNativeProps
is not supported on Fabric.Since we cannot use
setNativeProps
, I had to come up with a solution that changes the props and thus works on both architectures. Here's how it works: ifuseAnimatedScrollHandler
has a momentum event listener and simultaneouslyAnimated.ScrollView
doesn't have any regular JS momentum event listener defined, we add a dummy listener foronMomentumEnd
(chosen arbitrarily by me) which causessendMomentumEvents
to be true and thus enables emitting momentum events on the native side. Currently, the only two momentum events supported by React Native areonMomentumBegin
andonMomentumEnd
, see here, so I hard-coded these names as for now.Another approach would be to call
setSendMomentumEvents(true)
in the native code which is probably better than adding a dummy listener on JS context.Test plan
This PR can be tested on the example from issue description #2735.
Note that
onMomentumEnd
event callback will be called exactly three times, it's not an issue with Reanimated, see #2735 (comment) for details.Also keep in mind that although
_filterNonAnimatedProps
gets executed during fast refresh (which is correct), for some reasonvalue.current.eventNames
isn't updated (i.e. it contains the event names from before the update, even if you add or remove some entries insideuseAnimatedScrollHandler
), therefore when testing out the changes most likely you will need to reload the app.Make sure to test the following cases:
useAnimatedScrollHandler
, no JS momentum event listener inAnimated.ScrollView
→ dummy listener shouldn't be registered 😴useAnimatedScrollHandler
, but no JS momentum event listener inAnimated.ScrollView
→ dummy listener should be registered ✅useAnimatedScrollHandler
and some JS momentum event listener inAnimated.ScrollView
→ dummy listener shouldn't be registered (or overwritten) 😴