fix(Paper,iOS): dismiss all attached view controllers correctly on reload

[hirbod]

Description

This PR addresses an issue with react-native-screens where modals were not being dismissed correctly during app reloads when combined with foreign view controllers. The fix involves enhancing the invalidate method to recursively dismiss all presented view controllers, ensuring a clean state on reload.

This is not a development-only problem; this fix also addresses reloads from OTA updates.

Changes

• Enhanced invalidate method in RNSScreenStack.mm to recursively dismiss all presented view controllers.
• Ensured _presentedModals is cleared and _controller is detached from its parent during invalidation.
• Added a helper method dismissAllPresentedViewControllersFrom to handle recursive dismissal logic.

Screenshots / GIFs

Before	After
before.mp4	after.mp4

The red background is from a transparentModal by RNS, the sheet is a foreign view controller. Before the change, react-native-screens would break on reload if a foreign view controller was mounted on top. I came to the solution after finding this PR. The issue originally started here. After my changes, RNS works correctly on reload with third-party controllers.

I have no experience with Fabric, so I can't help with that. Feel free to update the solution for Fabric if needed.

Test code and steps to reproduce

Test2175

Checklist

• Included code example that can be used to test this change
• Ensured that CI passes

[kkafar]

Thank you! The code changes look good, however I'll need to test it & see what's going on on Fabric. Will be back with more insights once I do that (most likely Monday / Tuesday).

[hirbod]

dismissOnReload is not implemented for the new architecture anyway, so this should not be blocker for now

react-native-screens/ios/RNSScreenStack.mm

Lines 696 to 704 in 5cfafbb

	- (void)dismissOnReload
	{
	#ifdef RCT_NEW_ARCH_ENABLED
	#else
	dispatch_async(dispatch_get_main_queue(), ^{
	[self invalidate];
	});
	#endif // RCT_NEW_ARCH_ENABLED
	}

EDIT: I removed couple of messages, sorry. It was a mistake

[hirbod]

@kkafar friendly reminder :)

[kkafar]

Yeah, thanks for the reminder. I'm convinced that GitHub needs a option of posting a periodic notification until something is done.

I'll allow myself liberty of pushing onto your branch, hope you don't mind.

[kkafar]

Ok, I'm thinking through this change right now & I see a potential issue.

This goes through system chain of presentedModals and removes them one-by-one back-to-front.
Given that I can imagine a situation where there are modals presented from various stacks, then one stack is popped and all modals are popped all together, where desired behaviour would be to drop only ones owned by to-be-popped stack. This is kinda weird scenario, but I can imagine someone doing that.

Second remark is that is should be sufficient to pop only bottom-most modal to dismiss whole chain, w/o the need for recursion, but that's implementation detail I'll correct later.

Right know I want to ask you if you have throughput to provide me (& the PR) with issue reproduction so that I can play with it right now & someone has opportunity to test this behaviour later.

[hirbod]

This PR aims to fix issues with reloading (pressing R) when RNS is used in conjunction with third-party UIViewControllers. We have a pretty complex setup, and I did not encounter any regressions with my proposed fix. Your described scenario is exactly one of the issues this PR solves because it closes the modals.

I’d suggest we merge this and address any regressions if they are reported (which I highly doubt). I also tested scenarios where you navigate from an open modal to a different bottom tab, and everything behaved exactly the same with the change.

Given that I can imagine a situation where there are modals presented from various stacks, then one stack is popped and all modals are popped all together, where desired behaviour would be to drop only ones owned by to-be-popped stack

I don’t think this is a thing in iOS. Modals never really belong to a specific stack; they are always pushed on top (they can have an inline stack, though, which is unrelated in my opinion)

[kkafar]

I'll insist on proper reproduction here, so that I can play around & test this. I'll have some throughput to prepare one myself most likely early next week.

I don’t think this is a thing in iOS. Modals never really belong to a specific stack; they are always pushed on top (they can have an inline stack, though, which is unrelated in my opinion)

You are right with respect to the native platform. I meant by "modals owned by stack" these screens, that have modal presentation and are rendered directly as children of given ScreenStack in the JS layer.

[hirbod]

I'll create a repro for you.

[hirbod]

While working on your repro, I discovered that my PR fixes all related modal problems on reload, not just those by third parties. Currently, RNS closes all modals but keeps the last one mounted. This also makes the yellowbox log unclickable until all modals are closed. Little side bug I've discovered: when pulling on modals the header height gets totally borked. It's also visible in my video, but maybe something for a different PR. Repro coming in 10min.

issue.mp4

[hirbod]

Hey @kkafar, here is the link to Repro; https://github.com/hirbod/RNSModalDismissRepro
along with a video showcasing the issue. Additionally, this PR addresses #2125. Due to the way modals and UIPresentationController behave on iOS, I haven't found any other solution than forcibly closing all modals on reload.

Make sure to check the (i), tab one, and tab two. The video makes things clearer in my opinion. Whenever you see the "Metro reloading" bar, I pressed R.

full-repro.mp4

Once you apply my PR, all the issues are gone, reloading will always close the modals, and navigating from a modal to a screen will correctly close all open modals and not leave anything open which breaks touch inputs. (Both RNS and third party)

[hirbod]

Hi @kkafar, did you get a chance to look into this? 😊

[kkafar]

No, haven't sit down to yet, sorry. I'll put it much higher on my board rn

[lucacaputo]

any update on this one? 👀

[hirbod]

@tboba any chance you could look into this?

[lucacaputo]

I found out that it works when setting animation='none', although it's not going to look nice. My current workaround is the following:

import { useState, useEffect, useMemo, ComponentProps } from 'react';
import Animated, {
  useSharedValue,
  useAnimatedReaction,
  runOnJS,
  FadeOut,
} from 'react-native-reanimated';

const useModalVisibilityState = (isVisibleProp: boolean | undefined) => {
  const isSharedVisible = useSharedValue(isVisibleProp);
  const [isModalVisible, setIsModalVisible] = useState(isVisibleProp);

  useEffect(() => {
    if (isVisibleProp) {
      isSharedVisible.value = true;
    }
  }, [isSharedVisible, isVisibleProp]);

  useAnimatedReaction(
    () => isSharedVisible.value,
    (curr) => runOnJS(setIsModalVisible)(curr),
  );

  const fadeOutAnimation = useMemo<
    Exclude<ComponentProps<typeof Animated.View>['exiting'], undefined>
  >(
    () =>
      FadeOut.withCallback((finished) => {
        if (finished) {
          isSharedVisible.value = false;
        }
      }),
    [isSharedVisible],
  );

  return useMemo(
    () => ({
      isModalVisible,
      fadeOutAnimation,
    }),
    [fadeOutAnimation, isModalVisible],
  );
};

export default useModalVisibilityState;

then set the modal's animation prop to 'none' and wrap your children in an Animated.View, use the isModalVisible as the visible prop of the modal, and add a prop to your component that toggles the content visibility (hence triggering the unmount animation of the modal contents).

[hirbod]

@lucacaputo not sure why you are hijacking this PR and what your snippet aims to solve?

[lucacaputo]

@hirbod sorry I thought I was commenting on a related issue

[kkafar]

Hey, I finally have time to look into it. I'm sorry for such long delay.

I've rebased the PR on main and tested the issue on a added reproduction (Test2175 in example apps). I can't reproduce the issue.

I don't want to use Expo-based reproduction, because expo often runs its own logic which might interfere, thus I've prepared one (see files of this PR) based solely on react-navigation@v7 attempting to mimic structure you presented.

The example has stack navigator, with few screens: first contains tab navigator, the rest of them are push screen, transparent modal screen & modal screen.

Inside tab navigator there are buttons to open the modals from outer stack navigator. I can't trigger the behaviour you described in any navigation scenario. Might it be possible, that this issue has been fixed some other way? We had merged few PRs related to foreign modals behaviour last few weeks.

Caution

I've crudely force-pushed after rebase, consider pulling to a new local branch.

[kkafar]

I see also high chance that I've messed up the reproduction, lemme know when you have a moment

[hirbod]

@kkafar Current versions still break. We're using Expo Router, and my reproduction is still working. Upgrading to v7 is not an option for us right now. My PR fixes the issue. Honestly, it's tough to look into this again after almost 4 months.

[kkafar]

I see, but I'm not willing to merge this, since I'm not able to reproduce the issue without Expo involved. I'll ask @adrianryt to take a look with fresh pair of eyes.

[hirbod]

Expo Router is one of the most significant use cases for react-native-screens, and having Expo involved is a big deal.

[adrianryt]

I think the current reproduction provided by @kkafar is not correct. I was able to recreate the bug without expo - with the use of Kacper's test. I will get back to this tomorrow and investigate it further.

[adrianryt]

I allowed myself to push changes to test file. I want to note 4 scenarios related to the issue:

Scenario 1

Repro:

• Start the Test2175 app and click show owned modal button.
• click "Push foreign modal" (not "Push foreign modal(inside Screen Component)")
• Reload app
  Video:

Screen.Recording.2024-09-27.at.13.35.35.mov

Summary:
In this scenario, the behaviour with oldInvalidateImpl is correct.

Scenario 2

Repro:

• Start the Test2175 app and click show owned modal button.
• click "Push foreign modal" then again click "Push foreign modal" and again.
• Reload app

Video:

Screen.Recording.2024-09-27.at.13.39.37.mov

Summary:
In this scenario, modal is not dismissed and changing ios code to use newInvalidateImpl solve the issue.

Scenario 3

Repro:

• Start the Test2175 app and click show owned modal button.
• click "Push foreign modal (inside Screen Component)" button
• Reload app
  Video:

Screen.Recording.2024-09-27.at.13.46.15.mov

Summary:
In this scenario, modal is not dismissed and changing ios code to use newInvalidateImpl solve the issue.

Scenario 4

Repro:

• Start the Test2175 app and click show owned transparentModal button.
• click "Push another modal" button
• Reload app
  Video:

Screen.Recording.2024-09-27.at.13.53.09.mov

Summary:
In this scenario, transparentModal is not dismissed, it is not possible to interact with the app anymore and changing ios code to use newInvalidateImpl solve the issue.

Conclusions

• From scenario 2 and 3 I would say that whenever multiple modals wrapped in Stack.Screen component are pushed on the navigation stack, reloading the app will cause described issue.
• Solution provided by hirbod solves the issue, however, recursive removing the modals can be seen on the screen.
• I also checked #2125, I managed to create the reproduction but unfortunately this solution did not work for me. Reproduction is available here Add reproduction to Issue #2125 #2357

cc @kkafar

[kkafar]

Thank you @adrianryt! I'll work on the implementation suggestion (link) and proceed with merging.

[hirbod]

How is the status here?

[kkafar]

Thanks for reminder. I've adjusted the implementation & tested it.

It turned out we missed a message to _controller (the UINavigationController itself).

All test cases described earlier by @adrianryt seem to work & all the cases I've came up with also work now.

What's left is:

1. Test react-navigation: App randomly freezes when react-native Modal is shown while preventing remove screen with usePreventRemove on iOS #2125 whether we have it now solved or not - it is definitely not solved and seems rather unrelated. Initial debugging suggests that either we don't prevent the to-be-dismissed controller from being detached or there is bug in assertion in UIKit or there is bug in react-native modal presenting code. Anyway, not related to this PR.
2. Wait for the CI to pass & merge this PR,
3. Check how its going on Fabric - if needed we'll fix it in another PR; seems that Fabric runs smoothly

I'm on it right now

[kkafar]

Took a long time - sorry for that.

I've changed the implementation - instead of implementing recursive dismissal by hand we now dismiss all controllers presented from _controller or "down" the presented view controllers hierarchy by letting system do its job. With these changes view controllers should be removed immediately on reload, w/o glitchy effect reported by @adrianryt.

If a controller was presented outside the screen stack (foreign modal not inside a screen) it should be responsible for its own dismissal.