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(iOS): modal not presenting when deep in stack #2335

Merged
merged 2 commits into from
Sep 12, 2024
Merged

Conversation

maksg
Copy link
Contributor

@maksg maksg commented Sep 5, 2024

Description

When 3 screens with a ‘modal’ presentation mode are stacked, navigating to a screen outside of the current stack is not functioning correctly.

Changes

Removed dismissing logic for lastModalVc.

Screenshots / GIFs

Before

30-40-xoxak-qawl4.mp4

After

Simulator.Screen.Recording.-.iPad.Pro.13-inch.M4.-.2024-09-05.at.14.01.40.mp4

Test code and steps to reproduce

TestModalNavigation was added to tests.

Checklist

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

Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
Comment on lines -516 to -522

UIViewController *lastModalVc = [self lastFromPresentedViewControllerChainStartingFrom:firstModalToBeDismissed];

if (lastModalVc != firstModalToBeDismissed) {
[lastModalVc dismissViewControllerAnimated:shouldAnimate completion:finish];
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Have you checked if this doesn't break anything else? According to blame, this check has been here added in purpose. See: 471127e
Could you check if Test1829.tsx works properly with changes from this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we had a call with @kkafar trying to figure out the proper fix prior to making this PR and we checked a few scenarios including Test1829.
Unless there is some hidden scenario which breaks by this, there shouldn't be any issues with the removal of those lines.

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Hey, I confirm that we tested this code and it seems that some other changes in code made the to-be-removed lines obsolete or even "misbehaving" (they introduce a bug now, instead of improving the situation).
We've also tested common cases and navigation scenarios & it seems we're good on this front.

Let's solve the merge conflicts and we can proceed.

@kkafar
Copy link
Member

kkafar commented Sep 12, 2024

Also for some reason iOS e2e CI is failing - I've restarted it. Let's make sure it passes.

@maksg
Copy link
Contributor Author

maksg commented Sep 12, 2024

@kkafar iOS tests seem to be failing since last week so I'm not sure what's exactly the cause of that.

Anyway, I've resolved conflicts.

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Let's go. Thanks for these changes.

@maksg maksg merged commit 61ef186 into main Sep 12, 2024
4 of 5 checks passed
@maksg maksg deleted the @maksg/fix-ios-modals branch September 12, 2024 11:52
ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
)

## Description

When 3 screens with a ‘modal’ presentation mode are stacked, navigating
to a screen outside of the current stack is not functioning correctly.

## Changes

Removed dismissing logic for `lastModalVc`.

## Screenshots / GIFs

### Before

https://github.com/user-attachments/assets/d4991253-52f1-4f35-8698-242d98b218d1

### After

https://github.com/user-attachments/assets/571ebd45-7319-4225-ab5b-4f68027f4f09

## Test code and steps to reproduce

`TestModalNavigation` was added to tests.

## Checklist

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

Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants