-
-
Notifications
You must be signed in to change notification settings - Fork 514
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(iOS): onNativeDismissCancelled called too early during modal dismissal #2129
fix(iOS): onNativeDismissCancelled called too early during modal dismissal #2129
Conversation
Hi @zetavg, thanks for preparing these changes 🎉. I took a glance & these changes look good, however I'll need to find time to test this! Will get back you once I do. |
@kkafar Giving myself a notification to take care of this PR somewhere next week. |
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.
LGTM!
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.
I finally got to test the changes, sorry for such delay. I've merged main & merged this implementation with recent changes from #2184
Note
We should deprecate emitting gestureCancel
in presentationControllerDidAttemptToDismiss
in favour of emitting only dismissCancelled
to align fully with react-navigation v7 API. Created ticket for this.
Thank you for your contribution @zetavg!
Will merge once the CI passes. |
…issal (software-mansion#2129) ## Description Currently, on iOS, `onNativeDismissCancelled` is being called while the user begins to drag down a modal (which calls the callback function of the `usePreventRemove` hook, if `preventRemove` is `true`). This is not a common behavior of iOS apps and can sometimes be annoying: the callback will be triggered while the user is scrolling around a `ScrollView` in the modal and has hit the top, which is interrupting. | Ideal | Before | After | |-------|--------|-------| | <a href="https://github.com/software-mansion/react-native-screens/assets/3784687/167828c8-eb3a-4b4b-99a7-b9eb7ec55f06"><img alt="Ideal" width="240" src="https://github.com/software-mansion/react-native-screens/assets/3784687/fca2e909-a523-422a-80a5-3ac301539486" /></a> | <a href="https://github.com/software-mansion/react-native-screens/assets/3784687/6c166757-8c73-4e90-9236-9faf84514a14"><img alt="Before" width="240" src="https://github.com/software-mansion/react-native-screens/assets/3784687/812426eb-5e5d-4da5-855b-8ff6d1af14ef" /></a> | <a href="https://github.com/software-mansion/react-native-screens/assets/3784687/7993f36c-3455-4b23-8a28-f46956655f8a"><img alt="After" width="240" src="https://github.com/software-mansion/react-native-screens/assets/3784687/62ab528a-4675-44fe-a5ae-682c2d155817" /></a> | | The event fires when the user **releses** the drag. If the user did not intend to dismiss the modal (the user did not drag it down far enough or dragged it back to its original position before release), the event will not fire. | The event fires as soon as the modal is dragged down, regardless of whether the user intends to dismiss the modal or not. (In this case, the user just wants to scroll to the top.) | Same as "Ideal". | This PR adjusts this to the more ideal behavior. ## Changes Move the call of `notifyDismissCancelled` from [`presentationControllerShouldDismiss`](https://developer.apple.com/documentation/uikit/uiadaptivepresentationcontrollerdelegate/3229890-presentationcontrollershoulddism) to [`presentationControllerDidAttemptToDismiss`](https://developer.apple.com/documentation/uikit/uiadaptivepresentationcontrollerdelegate/3229888-presentationcontrollerdidattempt) - which "Notifies the delegate that a user-initiated attempt to dismiss a view was prevented", and has the same platform avalibality as `presentationControllerShouldDismiss`. ## Test code and steps to reproduce Use the new "Prevent Remove" example: <img width="300" src="https://github.com/software-mansion/react-native-screens/assets/3784687/622673dc-d520-4828-a2c3-83de7f2e3bef" /> ### Full test recordings **Modal** https://github.com/software-mansion/react-native-screens/assets/3784687/16777817-3949-413c-83ad-f0f7136af158 **Normal Screen** (this should not be affected since I believe that preventing going back from a normal screen is not handled here) https://github.com/software-mansion/react-native-screens/assets/3784687/41f25247-eb54-4ce8-9348-690d0a925e8f ## Checklist - [x] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Updated documentation: <!-- For adding new props to native-stack --> - [ ] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [ ] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
Description
Currently, on iOS,
onNativeDismissCancelled
is being called while the user begins to drag down a modal (which calls the callback function of theusePreventRemove
hook, ifpreventRemove
istrue
).This is not a common behavior of iOS apps and can sometimes be annoying: the callback will be triggered while the user is scrolling around a
ScrollView
in the modal and has hit the top, which is interrupting.This PR adjusts this to the more ideal behavior.
Changes
Move the call of
notifyDismissCancelled
frompresentationControllerShouldDismiss
topresentationControllerDidAttemptToDismiss
- which "Notifies the delegate that a user-initiated attempt to dismiss a view was prevented", and has the same platform avalibality aspresentationControllerShouldDismiss
.Test code and steps to reproduce
Use the new "Prevent Remove" example:
Full test recordings
Modal
Full.Test.-.Modal.-.05.mp4
Normal Screen (this should not be affected since I believe that preventing going back from a normal screen is not handled here)
Full.Test.-.Screen.-.05.mp4
Checklist