react-native-screens icon indicating copy to clipboard operation
react-native-screens copied to clipboard

fix(iOS): onNativeDismissCancelled called too early during modal dismissal

Open zetavg opened this issue 1 year ago • 2 comments

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
Ideal Before After
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 to presentationControllerDidAttemptToDismiss - 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:

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:
    • [ ] 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

zetavg avatar May 12 '24 16:05 zetavg

Note: both #1959 and this PR can achieve #1950. But I think this should be the default behavior of onNativeDismissCancelled and usePreventRemove - firing the callback when the user releases the drag.

zetavg avatar May 12 '24 17:05 zetavg

Hi @zetavg, thanks for preparing these changes :tada:. 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 avatar May 15 '24 11:05 kkafar

@kkafar Giving myself a notification to take care of this PR somewhere next week.

kkafar avatar Jul 12 '24 08:07 kkafar

Will merge once the CI passes.

kkafar avatar Aug 06 '24 09:08 kkafar