Skip to content
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

simultaneousWithExternalGesture regression, broken with 2.17.0, used to work with 2.16.2 #2963

Closed
mgcrea opened this issue Jun 26, 2024 · 5 comments
Labels
Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided

Comments

@mgcrea
Copy link
Contributor

mgcrea commented Jun 26, 2024

Description

I have some kind of zoom view in my application that has a pan/pinch/rotation gestures attached to it, on top of it I have an svg overlay (using StyleSheet.absoluteFill) that also has a pan gesture (in my case only for mouse/stylus but it does not matter). I configure simultaneousWithExternalGesture on this extra pan gesture.

This worked well before and had both gesture working but after updating to 2.17.1 (tried 2.17.0 as well), the zoom view gesture is not working anymore.

Steps to reproduce

  1. git clone --branch bug-gesture-handler-1 --single-branch https://github.com/mgcrea/react-native-reanimated-sandbox.git
  2. pnpm install; npx pod-install
  3. check that everything is working
  4. npx npm-check-updates -f react-native-gesture-handler -u
  5. pnpm install; npx pod-install
  6. check that the gesture does not work anymore

Snack or a link to a repository

https://github.com/mgcrea/react-native-reanimated-sandbox/tree/bug-gesture-handler-1

Gesture Handler version

2.17.1

React Native version

0.74.2

Platforms

iOS

JavaScript runtime

Hermes

Workflow

React Native (without Expo)

Architecture

Paper (Old Architecture)

Build type

Debug mode

Device

Real device

Device model

iPad Pro

Acknowledgements

Yes

@github-actions github-actions bot added Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided labels Jun 26, 2024
@m-bert m-bert changed the title simultaneousWithExternalGesture regression, broken with 1.7.0, used to work with 1.6.2 simultaneousWithExternalGesture regression, broken with 2.17.0, used to work with 2.16.2 Jun 28, 2024
@m-bert m-bert changed the title simultaneousWithExternalGesture regression, broken with 2.17.0, used to work with 2.16.2 simultaneousWithExternalGesture regression, broken with 2.17.0, used to work with 2.16.2 Jun 28, 2024
@m-bert
Copy link
Contributor

m-bert commented Jun 28, 2024

Hi! I took the liberty of correcting Gesture Handler versions in PR title and description 😅

Could you please check if #2969 solves this issue?

@mgcrea
Copy link
Contributor Author

mgcrea commented Jun 28, 2024

@m-bert while it does fix it in the reproduction demo, unfortunately I'm still noticing the issue on my actual app (unless I made a mistake somewhere). Is there an easy way for me to inspect the gesture tags to help you check if the bug is still there?

Will double check.

@m-bert
Copy link
Contributor

m-bert commented Jun 28, 2024

Well, you can try to log tags from event in gesture callbacks, but I'm afraid that without diving into source code it will be hard to find what is going on.

If you'd like to, you can try to look into attachHandlers and updateHandlers since they were problematic in your reproduction.

Unfortunately, without proper reproduction we can't do much 😞

@mgcrea
Copy link
Contributor Author

mgcrea commented Jun 28, 2024

I will try to reproduce it in the repo, might be related to the number of children inside the SVGMask on the actual app, thanks for the guidance and hopefully we can find a fix!

@mgcrea
Copy link
Contributor Author

mgcrea commented Jun 28, 2024

@m-bert good news, I was wrong and the fix is indeed working, not sure why but my actual app does not use the built output from either lib/commonjs or lib/module that I had patched but the typescript code from the src/.

Fixed the patch and it works! Thanks a lot!

react-native-gesture-handler@2.17.1.patch

@mgcrea mgcrea closed this as completed Jun 28, 2024
m-bert added a commit that referenced this issue Jun 28, 2024
## Description

This PR replaces `queueMicrotask` with `requestAnimationFrame`. Turns out that introducing `useLayoutEffect` (which was introduced in #2925) ended up in gestures being attached in wrong order in some cases. 

This can be observed in #2963 (it is highly recommended to look into repro structure):

1. 3 handlers from `ZoomView` where attached - they have correct `tags`
2. Re-render happens, handlers' `tags` are now `-1`
3. `Pan` from `SVGMask` is attached - all handlers marked as `simultaneous` have `tag` `-1`, therefore relations are not set up
4. Handlers from 1. are attached again, so they get back their original `tags`

In this scenario, `simultaneous handlers` array in `Pan` in `SVGMask` was empty, effectively disabling this relation. Switching to `requestAnimationFrame` solves this problem. 

## Test plan

Verify that examples work as they used to (`draggable`, `multitap`, `transformations`)

Also tested on code from #2963 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided
Projects
None yet
Development

No branches or pull requests

2 participants