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

fix: multiple modal on iOS #31498

Conversation

intergalacticspacehighway
Copy link
Contributor

@intergalacticspacehighway intergalacticspacehighway commented May 9, 2021

This PR aims to resolve.
https://react-native.canny.io/feature-requests/p/multiple-modal-components
#3445

Summary

In order to open multiple modals at once, they need to be nested within a modal on iOS. To fix this issue, we mount each modal into a new UIWindow. pageSheet and formSheet modal presentation styles are exceptions, they animate the behind view so it cannot be presented in a new window so we use the last presented controller for them to match native behavior.

Changelog:

[iOS] [Fixed] - Multiple modals

Test Plan

Verified the existing and added functionality in the RN Tester app.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 9, 2021
@analysis-bot
Copy link

analysis-bot commented May 9, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 118489f

@analysis-bot
Copy link

analysis-bot commented May 10, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,947,093 -1,807
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 9,540,791 -1,414
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 606e145
Branch: main

@lfoliveir4
Copy link

@intergalacticspacehighway why this merge is blocked?

@intergalacticspacehighway
Copy link
Contributor Author

@lfoliveir4 only collaborators can merge PR.

@lfoliveir4
Copy link

@lucianomlima When you can review and dive into this PR, can you please let us know?

@carloslibardo
Copy link

Why this PR doesn't be merged?

@carloslibardo
Copy link

carloslibardo commented Jun 16, 2021

Hello @intergalacticspacehighway , i updated my app to your branch because i'm with the same issue, and your solution works, in some cases...

When i already has a modal open and another modal will be opened, for example:

const [modalOpen, setModalOpen] = useState(false)
setTimeout(() => {
    setModalOpen(true)
}, 1000)

return(
    <>
        <Modal visible={true}/>
        <OtherModal visible={modalOpen}/>
    </>
)

works fine, but if a have a flow of modals, not, when should open the second modal, the first modal close and second modal will not be visible for example:

const [currentModal, setCurrentModal] = useState('modal1')

return(
    <>
        <Modal1 visible={currentModal === 'modal1'}><Button onClick={() => {setCurrentModal('modal2')}}/></Modal1>
        <Modal2 visible={currentModal === 'modal2'}><Button onClick={() => {setCurrentModal('modal3')}}/></Modal2>
        <Modal3 visible={currentModal === 'modal3'}/></Modal3>
    </>
)

I don't know if there is something about the way of your solution, because in the same time of Modal1 will be visible=false, Modal2 will be visible=true, i will keep investigating, but my knowledge in ObjetiveC is not good haha.

Someone have any idea?

@intergalacticspacehighway
Copy link
Contributor Author

@carloslibardo Thanks for testing this out. I tried to analyse the issue and I get this log on running your second example

 [Presentation] Attempt to present <RCTModalHostViewController: 0x7fd1a00abb20> on <RCTModalHostViewController: 0x7fd1a04137e0> (from <RCTModalHostViewController: 0x7fd1a04137e0>) whose view is not in the window hierarchy.

I think it tries to open the second modal before the first one is completely dismissed and there's where it breaks. Changing the above code to something like below works.

 <Modal
        visible={currentModal === 'modal1'}
        transparent
        onDismiss={() => {
          setCurrentModal('modal2');
        }}>
          <Button
            title="111"
            onPress={() => {
              setCurrentModal('dummyState');
            }}
          />
      <Modal visible={currentModal === 'modal2'} transparent>
          <Button
            title="button 2"
            onPress={() => {
              setCurrentModal('modal3');
            }}
          />
      </Modal>

I'll try to find a solution!

@carloslibardo
Copy link

carloslibardo commented Jun 21, 2021

@intergalacticspacehighway thanks for reply and keep trying to solve this problem =)

I tried to use your second sample to solve my problem, using modal as children of modal, but doesn't works correctly, both modals being not visible =/

@carloslibardo
Copy link

@intergalacticspacehighway some updates?

@AlkanV
Copy link

AlkanV commented Jul 6, 2021

any update on this?

@KingAmo
Copy link

KingAmo commented Jul 30, 2021

hi all , any update ?

@intergalacticspacehighway
Copy link
Contributor Author

Sorry for the delay everyone, I just got a chance to analyse the issue. Here are some findings.

  • A Modal View controller is presented using reactViewController which is a root view controller.
  • View controllers can only present one view controller at a time (like a stack and not a tree). This explains why multiple modal UI controllers cannot be opened with this single root view controller at once.
  • The solution I tried was using the last presentedViewController but this has an issue. e.g. It allows displaying multiple <Modal /> without nesting but internally this is what's happening.
// Modal1's controller will be presented by reactViewController
<Modal1 />
// Modal2's controller will be presented by Modal1's controller
<Modal2 />

This is where it gets tricky, what if the Modal1 gets dismissed, yes it'll dismiss Modal2 as well. This solution although has this limitation has a small upside IMO that it preserves the previously opened Modal and can keep Modal1 and Modal2 mounted even though they are not nested.

This can be also a bit deceptive when looked at the code as it looks like they are independent but in reality, the last opened Modal is presenting the next Modal. (Need more thoughts).

Also, I think multiple Modals most of the time are nested, and in that case, the existing or the above solution would work just fine.

I think what we may need is not multiple modal controllers but something like mounting views in UIWindow. Something like this or RN's createPortal support which works on android and iOS.

Let me know If I am missing something! :)

@lfoliveir4
Copy link

lfoliveir4 commented Feb 10, 2022

@intergalacticspacehighway What problem are we facing for this PR to be merged? Many UI's use flows with multiple modals... This PR would make life easier for many engineers... Any updates on it? @shergin @mdvacca @JoshuaGross Could any of you contributors help us move this PR forward? If not, what's the best way to proceed with this PR?

@thenriquedb
Copy link

+`1

@YangJonghun
Copy link

YangJonghun commented Feb 9, 2023

Unfortunately this PR can't be fix below case
(below code use react-native-modal for transition)

const positionStyle = [
  { top: 200, left: 200 },
  { top: 200, right: 200 },
  { bottom: 200, left: 200 },
  { bottom: 200, right: 200 },
]

const colors = ['red', 'purple', 'gray', 'orange']
const modals = [0, 1, 2, 3]

const App = () => {
  const [visible, setVisible] = useState(0)
  return (
    <>
      <View
        style={{ height: '100%', width: '100%', justifyContent: 'center', alignItems: 'center' }}
      >
        <TouchableOpacity onPress={() => setVisible(v => (v + 1) % 5)}>
          <Text>Press me</Text>
        </TouchableOpacity>
      </View>
      {modals.map((_v, i) => (
        <Modal isVisible={i + 1 === visible} key={i}>
          <View
            style={{
              position: 'absolute',
              justifyContent: 'center',
              alignItems: 'center',
              ...positionStyle[i],
              backgroundColor: colors[i],
            }}
          >
            <TouchableOpacity onPress={() => setVisible(v => (v + 1) % 5)}>
              <Text>Press me</Text>
            </TouchableOpacity>
          </View>
        </Modal>
      ))}
    </>
  )
}

The code above expects the old modal to disappear when the new one appears,
but the native implementation of this PR stacks the new modal on top of the old one,
so the new one will disappear. (Android works well)

@cctv1005s
Copy link

Hi, guys, any progress?

@YangJonghun
Copy link

YangJonghun commented Apr 13, 2023

The patch below should work fine as long as you don't use animationType or transparent prop (like react-native-modal)
also it was written for 0.71.6 and will most likely not work for 0.72.x and later, because the ReactNative team is changing the folder structure.

diff --git a/node_modules/react-native/React/Views/RCTModalHostView.m b/node_modules/react-native/React/Views/RCTModalHostView.m
index 65428a0..0b42427 100644
--- a/node_modules/react-native/React/Views/RCTModalHostView.m
+++ b/node_modules/react-native/React/Views/RCTModalHostView.m
@@ -38,6 +38,7 @@ - (instancetype)initWithBridge:(RCTBridge *)bridge
     UIView *containerView = [UIView new];
     containerView.autoresizingMask = UIViewAutoresizingFlexibleHeight | UIViewAutoresizingFlexibleWidth;
     _modalViewController.view = containerView;
+    _modalViewController.jsDismissed = YES;
     _touchHandler = [[RCTTouchHandler alloc] initWithBridge:bridge];
     _isPresented = NO;
 
diff --git a/node_modules/react-native/React/Views/RCTModalHostViewController.h b/node_modules/react-native/React/Views/RCTModalHostViewController.h
index b12b0f7..34491fd 100644
--- a/node_modules/react-native/React/Views/RCTModalHostViewController.h
+++ b/node_modules/react-native/React/Views/RCTModalHostViewController.h
@@ -13,4 +13,6 @@
 
 @property (nonatomic, assign) UIInterfaceOrientationMask supportedInterfaceOrientations;
 
+@property (nonatomic, assign) BOOL jsDismissed;
+
 @end
diff --git a/node_modules/react-native/React/Views/RCTModalHostViewManager.m b/node_modules/react-native/React/Views/RCTModalHostViewManager.m
index 4b9f9ad..dbfaaab 100644
--- a/node_modules/react-native/React/Views/RCTModalHostViewManager.m
+++ b/node_modules/react-native/React/Views/RCTModalHostViewManager.m
@@ -51,6 +51,8 @@ @interface RCTModalHostViewManager () <RCTModalHostViewInteractor>
 
 @implementation RCTModalHostViewManager {
   NSPointerArray *_hostViews;
+  dispatch_semaphore_t _viewControllerSemaphore;
+  dispatch_queue_t _modalTaskQueue;
 }
 
 RCT_EXPORT_MODULE()
@@ -62,6 +64,12 @@ - (UIView *)view
   if (!_hostViews) {
     _hostViews = [NSPointerArray weakObjectsPointerArray];
   }
+  if (!_viewControllerSemaphore) {
+    _viewControllerSemaphore = dispatch_semaphore_create(1);
+  }
+  if (!_modalTaskQueue) {
+    _modalTaskQueue = dispatch_queue_create(nil, DISPATCH_QUEUE_SERIAL);
+  }
   [_hostViews addPointer:(__bridge void *)view];
   return view;
 }
@@ -75,13 +83,28 @@ - (void)presentModalHostView:(RCTModalHostView *)modalHostView
       modalHostView.onShow(nil);
     }
   };
-  dispatch_async(dispatch_get_main_queue(), ^{
+  dispatch_async(_modalTaskQueue, ^{
     if (self->_presentationBlock) {
       self->_presentationBlock([modalHostView reactViewController], viewController, animated, completionBlock);
     } else {
-      [[modalHostView reactViewController] presentViewController:viewController
-                                                        animated:animated
-                                                      completion:completionBlock];
+      dispatch_semaphore_wait(self->_viewControllerSemaphore, DISPATCH_TIME_FOREVER);
+      dispatch_async(dispatch_get_main_queue(), ^{
+        UIViewController *lastPresentedViewController = modalHostView.reactViewController;
+        UIViewController *presentedViewController = nil;
+
+        while (lastPresentedViewController != nil || presentedViewController == viewController) {
+          presentedViewController = lastPresentedViewController;
+          lastPresentedViewController = lastPresentedViewController.presentedViewController;
+        }
+
+        viewController.jsDismissed = NO;
+        [presentedViewController presentViewController:viewController
+                                 animated:animated
+                                 completion:^{
+            dispatch_semaphore_signal(self->_viewControllerSemaphore);
+            !completionBlock ?: completionBlock();
+        }];
+      });
     }
   });
 }
@@ -95,11 +118,45 @@ - (void)dismissModalHostView:(RCTModalHostView *)modalHostView
       [[self.bridge moduleForClass:[RCTModalManager class]] modalDismissed:modalHostView.identifier];
     }
   };
-  dispatch_async(dispatch_get_main_queue(), ^{
+  dispatch_async(_modalTaskQueue, ^{
     if (self->_dismissalBlock) {
       self->_dismissalBlock([modalHostView reactViewController], viewController, animated, completionBlock);
     } else {
-      [viewController.presentingViewController dismissViewControllerAnimated:animated completion:completionBlock];
+      dispatch_semaphore_wait(self->_viewControllerSemaphore, DISPATCH_TIME_FOREVER);
+      dispatch_async(dispatch_get_main_queue(), ^{
+        if (viewController.presentedViewController == nil) {
+          UIViewController *parentViewController = viewController.presentingViewController;
+
+          [parentViewController dismissViewControllerAnimated:animated completion:completionBlock];
+
+          NSPointerArray *modalViewControllers = [NSPointerArray weakObjectsPointerArray];
+
+          while(parentViewController != modalHostView.reactViewController && ([parentViewController isKindOfClass:[RCTModalHostViewController class]] && ((RCTModalHostViewController *)parentViewController).jsDismissed == YES)) {
+
+            parentViewController = parentViewController.presentingViewController;
+
+            if (parentViewController) {
+              [modalViewControllers addPointer: (__bridge void *)(parentViewController)];
+            }
+          }
+          if (modalViewControllers.count == 0) {
+            dispatch_semaphore_signal(self->_viewControllerSemaphore);
+            return;
+          }
+          for (NSInteger i = 0; i < modalViewControllers.count; i++) {
+            UIViewController *parentVC = [modalViewControllers pointerAtIndex:i];
+            [parentVC dismissViewControllerAnimated:nil completion:^{
+              if (i == modalViewControllers.count - 1) {
+                dispatch_semaphore_signal(self->_viewControllerSemaphore);
+              }
+            }];
+          }
+        } else {
+          viewController.jsDismissed = YES;
+          dispatch_semaphore_signal(self->_viewControllerSemaphore);
+          !completionBlock ?: completionBlock();
+        }
+      });
     }
   });
 }

I think it's an unavoidable limitation of ReactNative that we can't display multiple Modal(ViewControllers) at once because iOS doesn't support it.

Therefore, this patch may cause unintended dismiss behavior if you have a modal open and try to clear the first visible modal, and that modal uses an animationType prop of "slide", "fade", or leaves "transparent" set to true.
But at least it won't cause ANR

I tested 4 cases

  • multiple modal present/dismiss at the same time
  • multiple modal present/dismiss in any order
  • Make both modals dismiss and present at the same time
  • Make modals appear nested

@intergalacticspacehighway
Copy link
Contributor Author

@carloslibardo i pushed a few changes to the PR. Can you try your usecase again?

@erquhart
Copy link

erquhart commented Sep 26, 2023

This is the most upvoted open PR on the repo, would be amazing to get multiple modals working. A bit crippling for app developoers trying to go all in on RN.

@KarthikBaleneni
Copy link

Why this PR is yet to be merged ?
Do we have any Open issues from this PR ?

@fabOnReact
Copy link
Contributor

This PR is included in the react-native-improved library:

react-native-improved

  • Supports ONLY react-native 0.73.
  • Supports only old architechture (new architechture is WIP)

Set-up

In package.json

 "scripts": {
+  "postinstall": "yarn run react-native-patch"
 }

Then

npm

npm install react-native-improved --save-dev

yarn v1

yarn add react-native-improved --dev

@react-native-bot
Copy link
Collaborator

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@react-native-bot react-native-bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Aug 3, 2024
@erquhart
Copy link

erquhart commented Aug 3, 2024

Still the most upvoted PR on the repo.

@react-native-bot react-native-bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Aug 4, 2024
@intergalacticspacehighway intergalacticspacehighway deleted the fix/multiple-modal-ios branch August 9, 2024 22:43
@erquhart
Copy link

pour one out

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. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.