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: issues with presenting owned modals from foreign ones #2113

Merged
merged 4 commits into from
Apr 27, 2024

Conversation

kkafar
Copy link
Member

@kkafar kkafar commented Apr 26, 2024

Description

Basically this is another edition of the issue #1829 (handled by #1912).
The issue comes down to the fact, that our ScreenStack is not aware of all modal view controllers being in presentation,
but this time it is not aware of third-party modal view controllers (I've named them "foreign" modals in opposite to "owned" modals).

This PR is not a comprehensive solution but rather just a patch aiming at fixing one particular interaction reported in #2048.

I've left verbose code comments explaining the issue and suggesting solution in the source code, including:

  // TODO: Find general way to manage owned and foreign modal view controllers and refactor this code. Consider building
  // model first (data structue, attempting to be aware of all modals in presentation and some text-like algorithm for
  // computing required operations).

Closes #2048
Closes #2085

Changes

Trigger dissmisal of foreign modal if it is presented above changeRoot modal (last modal that is to stay on stack after the updates).

Test code and steps to reproduce

Test2048 in TestsExample & FabricTestExample.

Checklist

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

kkafar added 2 commits April 26, 2024 13:33
In such case I've decided to trigger it's dismissal. This is IMO best
behaviour as this patches repored issue & it seems to me that this is
common use pattern.
@kkafar
Copy link
Member Author

kkafar commented Apr 26, 2024

TODO: Checkout whether I need to handle RNSModalScreenComponent (or whatever its name is)

Copy link
Member

@tboba tboba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On first sight, it looks good to me! Just left some small comments 🎉

ios/RNSScreenStack.mm Outdated Show resolved Hide resolved
ios/RNSScreenStack.mm Show resolved Hide resolved
ios/RNSScreenStack.mm Show resolved Hide resolved
ios/RNSScreenStack.mm Outdated Show resolved Hide resolved
@kkafar
Copy link
Member Author

kkafar commented Apr 26, 2024

Thanks for review. I'll wait with merging till I get response on #2048.

@kkafar kkafar merged commit c21de50 into main Apr 27, 2024
7 checks passed
@kkafar kkafar deleted the @kkafar/fix-presenting-from-react-native-modal branch April 27, 2024 17:53
Copy link
Member

@WoLewicki WoLewicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some spelling checks.

@@ -479,16 +482,35 @@ - (void)setModalViewControllers:(NSArray<UIViewController *> *)controllers
}
};

// changeRootController is the last controller that *is owned by this stack*, and should stay unchanged after this
// batch of transitions. Therefore changeRootController.presentedViewController is the first constroller to be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

controller

// dismissal logic also attempts to handle possible wide gamut of cases of interactions with third-party modal
// controllers, however it's not perfect.
// TODO: Find general way to manage owned and foreign modal view controllers and refactor this code. Consider building
// model first (data structue, attempting to be aware of all modals in presentation and some text-like algorithm for
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

structure

// We dismiss every VC that was presented by changeRootController VC or its descendant.
// After the series of dismissals is completed we run completion block in which
// we present modals on top of changeRootController (which may be the this stack VC)
//
// There also might the second case, where the firstModalToBeDismissed is foreign.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be the second case

@WoLewicki
Copy link
Member

Hopefully it did not destroy any scenarios where some code was aware of the problem and did its own workarounds for the issue. We should find a complete solution for the case ASAP I think.

@kkafar
Copy link
Member Author

kkafar commented Apr 29, 2024

Hopefully it did not destroy any scenarios where some code was aware of the problem and did its own workarounds for the issue.

Yeah, I can see it happen but 🤷🏻 In the end, we do not promise any stability of behaviour / API of native side code.

We should find a complete solution for the case ASAP I think.

My idea revolves around implementing something in the shape of Levenshtein distance algorithm, however it's problematic because at the level of screen stack we do not have accurate information about what modals are in presentation (there might be modals presented from other stack, or foreign modals*). Even if we computed this dynamically, e.g. by registering each presented modal in RNSScreenStackManager, which is shared between various RNSScreenStack instances, there arises issue of modals, that are not presented under separate UITransitionView 😅 How do we find such third-party modals? <-- this is open question for me. We can discuss this on some screens meeting.

I'll fix typos in follow up sometime soon

@hirbod
Copy link
Contributor

hirbod commented Jun 8, 2024

I believe that this change, or RNS in general, causes problems with hot reload when another ViewController is present.
GitHub Issue

It works fine and detaches RNS Modals on reload (pressing R), but keeps the RNS Modal attachted when another UIViewController is on top of it upon reload.

@kkafar
Copy link
Member Author

kkafar commented Jun 8, 2024

Hey @hirbod, would you mind opening a dedicated issue for the case you described? (🤞🏻 for reproduction). I'm sure we will be able to look into it.

@hirbod
Copy link
Contributor

hirbod commented Jun 8, 2024

@kkafar I already found a solution

I patched RNSScreenStack and changed invalidate

// updated this
- (void)invalidate {
    _invalidated = YES;
    [self dismissAllPresentedViewControllersFrom:_controller completion:^{
        // Ensure presented modals are removed and the controller is detached from its parent
        [_presentedModals removeAllObjects];
        [_controller willMoveToParentViewController:nil];
        [_controller removeFromParentViewController];
    }];
}

- (void)dismissOnReload {
    dispatch_async(dispatch_get_main_queue(), ^{
        [self invalidate];
    });
}

// added this
- (void)dismissAllPresentedViewControllersFrom:(UIViewController *)viewController completion:(void (^)(void))completion {
    if (viewController.presentedViewController) {
        [viewController.presentedViewController dismissViewControllerAnimated:NO completion:^{
            [self dismissAllPresentedViewControllersFrom:viewController completion:completion];
        }];
    } else {
        completion();
    }
}

@hirbod
Copy link
Contributor

hirbod commented Jun 8, 2024

I'll open a PR

@kkafar
Copy link
Member Author

kkafar commented Jun 8, 2024

Excellent, thank you!

@hirbod
Copy link
Contributor

hirbod commented Jun 8, 2024

#2175

ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
…mansion#2113)

## Description

Basically this is another edition of the issue software-mansion#1829 (handled by software-mansion#1912).
The issue comes down to the fact, that our `ScreenStack` is not aware of
all modal view controllers being in presentation,
but this time it is not aware of third-party modal view controllers
(I've named them "foreign" modals in opposite to "owned" modals).

This PR is not a comprehensive solution but rather just a patch aiming
at fixing one particular interaction reported in software-mansion#2048.

I've left verbose code comments explaining the issue and suggesting
solution in the source code, including:

```
  // TODO: Find general way to manage owned and foreign modal view controllers and refactor this code. Consider building
  // model first (data structue, attempting to be aware of all modals in presentation and some text-like algorithm for
  // computing required operations).
```

Closes software-mansion#2048
Closes software-mansion#2085

## Changes

Trigger dissmisal of foreign modal if it is presented above `changeRoot`
modal (last modal that is to stay on stack after the updates).

## Test code and steps to reproduce

`Test2048` in `TestsExample` & `FabricTestExample`.

## Checklist

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes
kkafar added a commit that referenced this pull request Oct 28, 2024
…load (#2175)

## 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 |
|--------|--------|
| <video width="320" height="240" controls
src="https://github.com/software-mansion/react-native-screens/assets/504909/1476ab3a-4bd9-4ffa-9256-760467a108bc"></video>
| <video width="320" height="240" controls
src="https://github.com/software-mansion/react-native-screens/assets/504909/e6c5eef0-16c6-49a2-9124-86467feec7f2"></video>
|

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](#2113).
The issue originally started
[here](lodev09/react-native-true-sheet#34).
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

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes

---------

Co-authored-by: Kacper Kafara <[email protected]>
Co-authored-by: adrianryt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants