-
-
Notifications
You must be signed in to change notification settings - Fork 998
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
Fix: onPress callbacks are invoked for all nested Pressables #3295
Conversation
Could you add a test case for the nested pressables in the example app? Ideally with a few different levels of nesting. |
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.
Good catch, it seems like the touch propagation logic was already implemented for pressInHandler
but it was completely missing from the pressOutHandler
😅
I found a couple of new issues i haven't seen before, but as far as I see, all of them were already present before this PR:
Collapsed issues
- On both android and iOS, given 2 nested
Pressable
s in aScrollView
, starting the touch on the inner pressable, and dragging out of it leads to the outerPressable
becoming unresponsive. - On android, given 2
Pressable
s in a non-scrollableView
, long pressing on the inner one, then dragging the finger out of both pressables leads toonPressIn
andonPressOut
also being called on the outerPressable
.
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.
This file could be merged with the existing nestedPressables.tsx
example.
I think it should be sufficient to add the console logs to nestedPressables.tsx
, and delete nested_pressable/index.tsx
altogether.
Description
Fixes #3282
The onStart and onBegin callbacks are invoked on the deepest responder on iOS and Android respectively. Inside those methods the
isTouchPropagationAllowed
ref is set to true. Base on that we can assume whichPressable
should be the responder of the touch on the JS side. Yet there is nothing that would stop invokingonPress
callbacks if theisTouchPropagationAllowed
is set to false in pressOutHandler. The fix introduces early return in thepressOutHandler
if clickedPressable
is not the deepest one.On the old architecture the measure method is called asynchronously, usually after the
onStart
. In nested pressables, if the deepest one is clicked it still callsonTouchesDown
andonTouchesUp
, which sets shouldPreventNativeEffects totrue
, omitting theonStart
, which sets it back tofalse
. Because of that, when now we tried to click on the outer Pressable it would be stopped in onStart shouldPreventNativeEvents check without setting theisTouchPropagationAllowed
totrue
and this is the exact same state that led to setting theshouldPreventNativeEffects
totrue
so it is a cycle. The simplest solution for this is to check if theonStart
is called before themeasure
inshouldPreventNativeEffects
check and early return if it's not.Test plan
Click on the red box and notice that callbacks on its parent are not invoked. Click on the text input to see that the callbacks on its Pressable parent are also not invoked.
Code