Fix native gesture reattach - #3672
Conversation
m-bert
left a comment
There was a problem hiding this comment.
I can see that android got a bit "out of sync" with iOS, can we make it so that one is analogous to another?
| if (!self.subviews[0]) | ||
| return; |
There was a problem hiding this comment.
Correct me if I'm wrong, but this should never happen with NativeHandler and if it does, I believe we would like to throw an error instead of silent fail.
There was a problem hiding this comment.
That was my attempt to synchronise android and iOS as on Android we had this, on Android there is a comment that explains it. I'll move it to the right place
It might happen that
attachHandlerswill be called before children are added into view hierarchy
There was a problem hiding this comment.
Ok, you are right it can really only happen in the attachHandlers, thus it's better to have a check only there. Fixed in 4916ba5.
| // It might happen that `attachHandlers` will be called before children are added into view hierarchy. In that case we cannot | ||
| // attach `NativeViewGestureHandlers` here and we have to do it in `addView` method. |
There was a problem hiding this comment.
I don't really see the point of moving this comment and removing part of its information. It was written in this place so that when you see adding to set instead of attaching handler you know why it happens.
There was a problem hiding this comment.
I thought, that the set would have been used regardless as we need it for reattach, so it wasn't the place for this comment. But ok, I don't have a strong opinion thus I restored it and synced ios, c4f18a7d
Co-authored-by: Michał Bert <63123542+m-bert@users.noreply.github.com>
## Description @tomekzaw reported a crash with the `Touchable` component when used with `createAnimatedComponent` and layout animations. The specific prop was the `exiting` animation. It caused the native detector view to outlive the relevant gesture hook, which drops the native recognizer on unmount. Due to this, the exiting detector may have tried to reattach a non-existent recognizer, causing the crash. This PR adds a check not to try reattaching already attached recognizers, which will effectively "freeze" the detector for the duration of the animation, unless its props change (which shouldn't happen, as its react node is gone at that point). Android wasn't crashing (`attachHandlerToView` no-ops on non-existent handlers), but reattaching it would cause it to be canceled. For symmetry, I added the same guard there. ## Test plan Rechecked reproducer from #3672 to make sure it didn't regress. Tested on the crashing app.
Description
In
NativeDetectornative gesture failed to reattach when child changed.Test plan
Add a logging message in attach/detach in
android/.../react/RNGestureHandlerRegistry.ktandapple/RNGestureHandlerRegistry.m`