-
-
Notifications
You must be signed in to change notification settings - Fork 612
fix(IOS): Fix non dismissible modal presentation #3415
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
base: main
Are you sure you want to change the base?
fix(IOS): Fix non dismissible modal presentation #3415
Conversation
When a modal conforms to RNSDismissibleModalProtocol and returns NO from isDismissible, it should not be dismissed when new modals are presented. Previously, when isDismissible returned NO, the code would skip the entire dismissal block, including the logic that updates changeRootController to handle the presentation hierarchy correctly. This caused iOS to throw an error because it tried to present a new modal from a view controller that was already presenting the non-dismissible modal. This fix: - Extracts the dismissibility check into a clear boolean variable - Adds an else clause that runs when a modal is non-dismissible - Updates changeRootController to be the non-dismissible modal - Checks if the non-dismissible modal has presented any RN modals - Properly dismisses RN modals presented by the non-dismissible modal This enables third-party modal libraries (like TrueSheet, react-native-modal, etc.) to coexist with React Navigation modals by implementing the protocol. Example use case: - User opens a bottom sheet (TrueSheet) from a screen - User taps a button in the sheet to open a React Navigation modal - Modal presents successfully from the sheet - Modal can be dismissed while sheet remains open Fixes presentation errors like: "Attempt to present <RNSScreen> on <UIViewController> which is already presenting <ThirdPartyModal>" Related: Implements proper support for RNSDismissibleModalProtocol
|
@kkafar an alternative approach would be to create another method to the protocol to return a ViewController that should update the root controller to keep things safe. Let me know if that's better. Thank you! |
…ting controller - Add presentingControllerForModals optional method to RNSDismissibleModalProtocol - Allow external non-dismissible modals to provide their own presenting controller - Only use external controller if explicitly provided (non-nil) - Falls back to original implementation if method not implemented or returns nil - Add UIKit import to protocol header for UIViewController support
|
Can we merge this @kligarski @kkafar? This should be straightforward and backwards compatible. Tested and works in a production app. Thank you! FYI @erickreutz |
|
Hey, sorry for very late response here & on other PRs. I'll look into this after we release 4.19.0 (planned for upcoming week). |
|
@kkafar happy new year! do you have time to take a look now please? |
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.
Hey! Thanks for the PR. There a couple of different topics I wanna discuss here.
TLDR:
I'm reluctant to land the changes in current shape, but willing to cooperate on a fix / guiding. To do that a full explanation of the issue this PR aims to solve + issue mechanism breakdown is required.
First, I do not understand the problem yet. The PR describes it only vaguely:
Previously, when a non-dismissible modal was encountered, the stack didn't properly update the root controller for subsequent modal presentations, which could cause modals to be presented from the wrong controller or fail to dismiss modals that were presented by the non-dismissible modal.
and therefore I can't tell whether this is the appropriate solution or not. I need to understand what does it mean that "[...] the stack didn't properly update the root controller [...]" and whole error mechanism & only then we'll be able to discuss possible solutions.
I'll try to find time to setup a proper reproducer app.
Second, please note that you do not propose an fix the the library itself, but rather an additional mechanism that you want to use to integrate a third-party (from the library perspective) component with the react-native-screens and it'll be on react-native-screens side to maintain it. At the same time react-native-screens exposes presentation: 'formSheet' option, which allows to achieve similar (likely not as extensive) functionality.
I acknowledge the need for this kind of integration, and that other components often need to be able to work with react-native-screens components, but we need to understand the needs first and weight benefits from the library and users perspective.
Third - now, talking directly about the code - I do not understand the changes (and their implications) completely. This is a very fragile fragment of code that went under a lot of fixing throughout last few years:
and often when we fixed one thing we broke some other interaction. Therefore I need profound understanding of the changes before I give a green light to merge this. I'll put more time into testing this & considering different scenarios. Comprehensive test plan would be beneficial here.
Fourth - yet thing is that we are in progress of new stack implementation and we're reluctant on landing any new APIs we'll potentially have to maintain in the soon-to-be-old stack implementation.
|
Hey @kkafar, thanks for taking the time to review the PR! I understand where you're coming from. For a reproducer, I have an extensive example that integrates react-navigation (react-native-screens) with my library. You can check it out here if you want to try it: https://github.com/lodev09/react-native-true-sheet/tree/main/example Basically I was re-using the existing non-dismissible protocol (if not mistaken, was from expo's link preview). The goal was to not dismiss my sheet when presenting a modal screen. The issue is that react-native-screens will now use the wrong presenting controller so the additional protocol for my lib to provide the presenting controller was necessary. Please let me know if there's any other way, I figured a protocol is the safest approach since it doesn't introduce any breaking changes. EDIT: |
|
Thanks for the update here. I'll check out your test application (likely tomorrow) and get back to you. |
|
I remember about this PR, haven't found time yet. |
|
Is this fix related to the issue below? When navigating to the sign-in screen from the true sheet, it won't navigate or dismiss. Anyone else seen this? Simulator.Screen.Recording.-.iPhone.16.Pro.-.2026-01-12.at.23.36.20.-.compressed.mp4 |
|
I'm having this issue with TrueSheet: https://sheet.lodev09.com/guides/navigation trim.BCC2F229-D0FD-424D-A219-C684376E2A7F.MOV |
Description
This PR fixes an issue with the modal presentation chain when non-dismissible external modals (e.g., third-party modals like TrueSheet) are present in the view controller hierarchy.
Previously, when a non-dismissible modal was encountered, the stack didn't properly update the root controller for subsequent modal presentations, which could cause modals to be presented from the wrong controller or fail to dismiss modals that were presented by the non-dismissible modal.
Changes
RNSScreenStack.mm:elsebranch to handle the case whenfirstModalToBeDismissedis non-dismissiblechangeRootControlleris now updated to point to that modalScreenshots / GIFs
Before
Sheet will not present, will get warning in XCode.
After
Simulator.Screen.Recording.-.iPhone.17.Pro.Max.-.2025-11-23.at.21.47.34.mov
Test code and steps to reproduce
To test this change with a non-dismissible third-party modal (e.g., TrueSheet):
1. First, ensure your third-party modal implements the protocol (library author's responsibility):
2. Then test with this example:
RNSDismissibleModalProtocol)Checklist