-
Notifications
You must be signed in to change notification settings - Fork 25k
fix(iOS): allow to interactively swipe down the modal #51483
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
Conversation
.../react-native/React/Fabric/Mounting/ComponentViews/Modal/RCTFabricModalHostViewController.mm
Show resolved
Hide resolved
|
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This is only changing the new architecture component. Should we do the same for legacy arch for consistency? This also needs a breaking changelog entry, and perhaps some more warnings. If you do not implement onRequestClose, the user can dismiss it and the application will get stuck. |
|
@javache I've cloned the implementation for old architecture. Additionally I've added warnings and updated types. Also the PR is now marked as |
b6b970a to
f31369c
Compare
|
Let's update the changelog a bit to make it explicit what developers need to use if they use Modal on iOS. Otherwise looks good. |
|
@javache Let me know if I should add something more to this description: |
|
Hey @javache @cipolleschi is there anything else needed to get this PR merged? |
|
@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Thanks for the ping here. I think we can safely make this a non-breaking change by making this controlled through an iOS-specific prop (allowSwipeDismissal) which we default to false, and we only log the new warning when that props is enabled. What do you think? |
398d064 to
7276d13
Compare
7276d13 to
c00ea41
Compare
|
Hey @javache thanks for your suggestion, I've added this as a prop. Can you re-review? |
|
@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Can you also send a PR for the docs? https://reactnative.dev/docs/modal The current docs seem incorrect with the description in the PR here:
According to the docs, the previous callback would've also been delivered from presentationControllerDidAttemptToDismiss: |
|
@javache I've added PR to the docs: facebook/react-native-website#4658 I'm not sure If I understand your concern about current documentation state because before the onRequestClose would be also called on iOS but users had to update state in there to hide the modal. With this new prop we allow users to fully swipe down the modal and afterwards we call onRequestClose so in both cases its called |
|
This pull request was successfully merged by @okwasniewski in 28986a7 When will my fix make it into a release? | How to file a pick request? |
Summary:
This PR allows to interactively close the modal using the swipe down gesture.
It fixes 5 year old issue: #29319
In short it removes
modalInPresentationwhich according to the documentation causes: "UIKit ignores events outside the view controller’s bounds and prevents the interactive dismissal of the view controller while it is onscreen.".It also adds another delegate event to call onRequestClose whenever modal is closed by gesture.
CleanShot.2025-05-20.at.22.35.46.mp4
Changelog:
[IOS] [ADDED] - Allow to interactively swipe down the modal.
Add allowSwipeDismissal prop.
Test Plan:
Test if swiping down the modal calls onRequestClose