Implement onRequestClose for iOS 13+ modals #27618
Conversation
facebook-github-bot
added
the
CLA Signed
react-native-bot
added
Component: Modal
Platform: iOS
|
Cc @hramos |
There was a problem hiding this comment.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Getting this failure at FB: |
Summary: # Disclaimer: I might be missing something as the solution I implemented here seems like something that was considered by original author. If this solution isn't good, I have a plan B. # Problem: `onDismiss` prop isn't being called once the modal is dismissed, this diff fixes it. Also I've noticed that `onDismiss` is meant to only work on iOS, why is that? By landing this diff, it'll be called on Android as well so we need to change the docs (https://facebook.github.io/react-native/docs/modal.html#ondismiss). ## Video that shows the problem Following code is in playground.js P70222409 which just increments number everytime onDismiss is called {F166303269} Reviewed By: shergin Differential Revision: D16109536 fbshipit-source-id: 3fba56f5671912387b217f03b613dffd89614c9d
|
Oh, I forgot to mention, https://github.com/facebook/react-native/blob/master/React/Fabric/Mounting/ComponentViews/Modal/RCTModalHostViewComponentView.mm probably needs updating as well, but I tried to enable fabric in the RNTester Podfile and was stuck with errors: |
|
Yea I guess we can remove the change from the fabric implementation and file a task to fix it later when it can compile properly in OSS. @cpojer what do you think? |
|
Is there anything I can do to help land this (I doubt it since it sounds like internal FB failures but thought I'd ask) ? |
|
It fixes the dismiss not being propagates issue in pageSheet modals but it seems to make all other modals be pageSheet regardless or the presentationStyle prop used. Anyone can reproduce the same issue with this PR? |
|
This pull request was successfully merged by @koke in 8e5fac8. When will my fix make it into a release? | Upcoming Releases |
|
@jpamarohorta I believe it's not the PR itself that does it but the fact that iOS detects that the new API is being used, thus it auto-upgrades the entire app to the new iOS 13 default. I think it's by design. |
|
How can I get this fix into my project.? please help me, someone. |
|
@HarishMurru You can use the steps @scarlac detailed in this comment: #26892 (comment) Otherwise, you'll probably have to wait until the next release. |
|
@scarlac so with this PR will we be able to use a sheet modal and a fullscreen one in the same app? |
|
@koke Seems, it's breaks |
|
I looked into it. The root cause is that we are changing the delegate which implies that some methods are called. If they are unimplemented, default presentation style is used instead. // Method must be implemented, otherwise iOS defaults to 'automatic' (pageSheet on >= iOS 13.0)
- (UIModalPresentationStyle)adaptivePresentationStyleForPresentationController:(UIPresentationController *)controller traitCollection:(UITraitCollection *)traitCollection
{
return self.presentationStyle;
}You won't normally find this via StackOverflow answers because most examples don't change the delegate. I will also modify my gist to include this change. |
| @@ -257,4 +264,11 @@ - (UIInterfaceOrientationMask)supportedOrientationsMask | |||
| } | |||
| #endif | |||
|
|
|||
There was a problem hiding this comment.
We'll need to add this to support changing the presentation style after setting the delegate
// Method must be implemented, otherwise iOS defaults to 'automatic' (pageSheet on >= iOS 13.0)
- (UIModalPresentationStyle)adaptivePresentationStyleForPresentationController:(UIPresentationController *)controller traitCollection:(UITraitCollection *)traitCollection
{
return self.presentationStyle;
}| @@ -111,6 +111,7 @@ - (instancetype)initWithFrame:(CGRect)frame | |||
| _viewController = [RCTFabricModalHostViewController new]; | |||
| _viewController.modalTransitionStyle = UIModalTransitionStyleCoverVertical; | |||
| _viewController.delegate = self; | |||
| _viewController.presentationController.delegate = self; | |||
There was a problem hiding this comment.
See below/above. We may need to add similar code in this file as well.
|
I know I'm not the author but I'm trying to solve this as well. This PR will likely suffer from the same issue as my Gist: ViewControllers underneath the modal will be unmounted after the modal finishes its transition. The net effect is that transparent modals are transparent while animating but once the animation finishes the background turns fully black (despite using if (@available(iOS 13.0, *)) {
_modalViewController.presentationController.delegate = self;
}This fixes the transparency issue to some extent but transparent iOS 13 modals will cast a tiny shadow which will most likely result in new issues being reported to the repo. The reason for that is that iOS 13 now renders an extra shadow view behind the new modal that are out of our control. In most cases users won't notice but a keen eye will notice a very faint shadow while they drag down on the page sheet modals. Issue is described here:
The overall conclusion is that the transition to iOS 13-style modals is troublesome and will likely cause reports like "transparency doesn't work on iOS 13" with screenshots of vague shadows (where you see "Private UIKit View(s)") despite using |
Summary:
Starting on iOS 13, a View Controller presented modally will have a "bottom sheet" style unless it's explicitly presented full screen.
Before this, modals on iOS were only being dismissed programatically by setting `visible={false}`. However, now that the dismissal can happen on the OS side, we need a callback to be able to update the state.
This PR reuses the `onRequestClose` prop already available for tvOS and Android, and makes it work on iOS for this use case.
Should fix #26892
## Changelog
[iOS] [Added] - Add support for onRequestClose prop to Modal on iOS 13+
Pull Request resolved: #27618
Test Plan:
I tested this using the RNTester app with the Modal example:
1. Select any presentation style other than the full screen ones
2. Tap Present and the modal is presented
3. Swipe down on the presented modal until dismissed
4. Tap Present again and a second modal should be presented
Differential Revision: D19235758
Pulled By: shergin
fbshipit-source-id: c0f1d946c77ce8d1baab209eaef7eb64697851df
|
@scarlac Are the issues you mentioned only for the page sheet modals, or does the change also cause issues in other modal styles too? |
|
@jacobp100 Only for pageSheet modals. The shadow that appears is an iOS bug for page and form sheets aka “cards”. Regular old full screen modals are fine AFAIK. |
Summary:
Starting on iOS 13, a View Controller presented modally will have a "bottom sheet" style unless it's explicitly presented full screen.
Before this, modals on iOS were only being dismissed programatically by setting `visible={false}`. However, now that the dismissal can happen on the OS side, we need a callback to be able to update the state.
This PR reuses the `onRequestClose` prop already available for tvOS and Android, and makes it work on iOS for this use case.
Should fix facebook#26892
## Changelog
[iOS] [Added] - Add support for onRequestClose prop to Modal on iOS 13+
Pull Request resolved: facebook#27618
Test Plan:
I tested this using the RNTester app with the Modal example:
1. Select any presentation style other than the full screen ones
2. Tap Present and the modal is presented
3. Swipe down on the presented modal until dismissed
4. Tap Present again and a second modal should be presented
Differential Revision: D19235758
Pulled By: shergin
fbshipit-source-id: c0f1d946c77ce8d1baab209eaef7eb64697851df
|
The issue related to this PR (#26892) is still not resolved, see discussion on the closed issue. Is there a way to reopen the issue? |
|
@noambonnie the reason why doesn't solve the problem is that RN - team doesn't release a version that contain this merge. I have checked the code v0.62.0 - v0.62.1 , it doesn't contain this change at all. so i have to change the native code by myself and it worked perfectly |
|
@Foveluy what changes did you make, to "enable" this functionality for 0.62? |
This change went into v0.62.0, see the CHANGELOG. The reason you couldn’t find it is perhaps because it was cherry-picked into the v0.62-stable branch and thus its commit hash changed: 6d00239 |
Done. |
|
@koke @alloy since I'm using patch-package to fix this, so I've updated my patch: https://gist.github.com/mrousavy/b4a4ce2df5ff61d12f46829edb132c7c Did I miss something? |
|
Sorry, I'm not working with React Native any longer |
ref: 1e7ed81 you can undo this change if you want this behavior |
Summary
Starting on iOS 13, a View Controller presented modally will have a "bottom sheet" style unless it's explicitly presented full screen.
Before this, modals on iOS were only being dismissed programatically by setting
visible={false}. However, now that the dismissal can happen on the OS side, we need a callback to be able to update the state.This PR reuses the
onRequestCloseprop already available for tvOS and Android, and makes it work on iOS for this use case.Should fix #26892
Changelog
[iOS] [Added] - Add support for onRequestClose prop to Modal on iOS 13+
Test Plan
I tested this using the RNTester app with the Modal example: