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

Inconsistent documentation: onShouldBlockNativeResponder works on iOS #40750

Conversation

elliottkember
Copy link
Contributor

@elliottkember elliottkember commented Oct 9, 2023

Summary:

This line of the documentation says that onShouldBlockNativeResponder in PanResponder only works on Android. However, I just tried it and it seems to be working very nicely on iOS.

Changelog:

[IOS] [FIXED] - Inconsistent documentation: onShouldBlockNativeResponder works on iOS

Test Plan:

Code comment change only. No functional changes should require no testing

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Oct 9, 2023
@javache
Copy link
Member

javache commented Oct 9, 2023

@javache javache closed this Oct 9, 2023
@github-actions
Copy link

github-actions bot commented Oct 9, 2023

Warnings
⚠️ One hour and a half have passed and the E2E jobs haven't finished yet.

Generated by 🚫 dangerJS against f7658c2

@elliottkember
Copy link
Contributor Author

elliottkember commented Oct 9, 2023

@javache This prop is not being completely ignored, as changing it to return true or false directly alters the behaviour in iOS.

For context, I am using a PanResponder inside a react-navigation native modal stack, and this allows me to use pan responders there. Without setting this property, the gestures bubble up to the (RNScreens) native modal view. Without this callback, the modal is dismissed if I pull down. The events are bubbling up to the native view.

It may be ignored at the native level, but is this perhaps related to these lines which handle this in the JS level? I don't know enough about the workings to be absolutely sure. I may even be using the wrong prop – but so far this callback works for me on iOS and is extremely useful!

@elliottkember
Copy link
Contributor Author

OK, I've got it @javache – I figured out what caused this to work for me!

It turns out I was using a library that implements react-native-gesture-handler and this was handling the gestures for me. I don't know how yet, but using onShouldBlockNativeResponder made that PanGestureHandler reliably pick up and handle the events, which I thought was really strange.

For now, the solution will be to wrap my pan responder in a PanGestureHandler so that I can use pan responders in modals. I'll see if there is any other documentation on it.

@elliottkember
Copy link
Contributor Author

Yep – react-native-gesture-handler overrides setJSResponder which causes this prop to start working. (Relevant issue props to @thomasttvo for finding it).

Last question – is there a deliberate reason this isn't implemented? Can/should it be? It's nice to finally be able to put pan responders into modal views like this. The gesture handler is a fine workaround, just wondering whether there's some deliberate decision not to implement this in RN itself.

Thank you!

@javache
Copy link
Member

javache commented Oct 10, 2023

Yep – react-native-gesture-handler overrides setJSResponder which causes this prop to start working. (Relevant issue props to @thomasttvo for finding it).

Overriding UIManager methods like this is extremely fragile. This is guaranteed to break in the new architecture.

Last question – is there a deliberate reason this isn't implemented? Can/should it be? It's nice to finally be able to put pan responders into modal views like this. The gesture handler is a fine workaround, just wondering whether there's some deliberate decision not to implement this in RN itself.

I think iOS' model for blocking other gestures is significantly different from how Android approaches this, it would be great to unify it though. We currently just have ad-hoc logic checking for responders, eg https://github.com/facebook/react-native/blob/main/packages/react-native/React/Views/ScrollView/RCTScrollView.m#L65-L75

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants