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

Orientation Bugs with transparentModal and containedTransparentModal #2006

Closed
amadeus opened this issue Jan 9, 2024 · 3 comments · Fixed by #2011
Closed

Orientation Bugs with transparentModal and containedTransparentModal #2006

amadeus opened this issue Jan 9, 2024 · 3 comments · Fixed by #2011
Labels
Missing info The user didn't precise the problem enough Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided

Comments

@amadeus
Copy link
Contributor

amadeus commented Jan 9, 2024

Description

I've found two classes of orientation bugs related to transparentModal and containedTransparentModal. I've also created a small demo react native project that shows off these two bugs in isolation. One interesting thing to note -- when I was trying to reproduce these in an Expo project, the transparentModal bug did not reproduce, only in a vanilla react-native project which I've attached a link for here: https://github.com/amadeus/react-navigation-bug

These bugs apply to iOS only, as far as I can tell.

Steps to reproduce

transparentModal Bug:

  1. Spawn a screen with transparentModal
  2. Dismiss that view
  3. Notice how safe areas are now locked to the portrait values regardless of orientation change?

This bug does not get triggered by any other modal type. Here's a video showing the bug:

RPReplay_Final1702609019.1.mov

Notice after i've spawned the transparentModal, all subsequent rotations show portrait safe areas in landscape orientation?

It also gets a bit weird if you spawn the modals in landscape the entire time -- notice when finally dismissing the transparent modal, the safe areas only break then:

RPReplay_Final1702609237.mov

containedTransparentModal Bug:

In trying to work around this issue on Discord with containedTransparentModal, I found another bug with containedTransparentModal where it breaks if you rotate the screen with the containedTransparentModal showing:

  1. Spawn a containedTransparentModal
  2. Rotate screen
  3. Notice how containedTransparentModal doesn't seem to update it's dimensions with the new rotated size?
RPReplay_Final1704762550.mov

Snack or a link to a repository

https://github.com/amadeus/react-navigation-bug

Screens version

3.25.0 (but it appears to even repro in the latest version)

React Native version

0.73.0

Platforms

iOS

JavaScript runtime

Hermes

Workflow

React Native (without Expo)

Architecture

Paper (Old Architecture)

Build type

Release mode

Device

Real device

Device model

No response

Acknowledgements

Yes

@github-actions github-actions bot added Repro provided A reproduction with a snack or repo is provided Missing info The user didn't precise the problem enough labels Jan 9, 2024
Copy link

github-actions bot commented Jan 9, 2024

Hey! 👋

It looks like you've omitted a few important sections from the issue template.

Please complete Steps to reproduce section.

@github-actions github-actions bot added the Platform: iOS This issue is specific to iOS label Jan 9, 2024
@amadeus
Copy link
Contributor Author

amadeus commented Jan 9, 2024

I literally filled out the steps to reproduce with a ton of steps and videos 😬

@tboba
Copy link
Member

tboba commented Jan 9, 2024

@amadeus Yeah, looks like our triage bot didn't detect them 😅
I've just tested the repro and it is reproducible for me. Thanks for submitting this issue!

tboba added a commit that referenced this issue Jan 24, 2024
## Description

On iOS there's a bug with contained (and contained transparent) modals,
where the modal does not change its orientation when the whole interface
(and/or device) is being rotated.
That's because contained modals will keep its orientation when the
interface is being rotated, as their controllers don't update the bounds
-> `viewDidLayoutSubviews` and `updateBounds` methods were not called at
the moment of changing the orientation.

This PR fixes this bug by updating the bounds when `layoutSubviews`
method from RNSScreenStack is being called, meaning that no matter if
there were previously any modals that have been dismissed or not, screen
stack will always update the bounds of the modal views.

Fixes #2006.

## Changes

- Fixed the problem with invalid orientation of contained modals by
updating their bounds.

## Screenshots / GIFs

### Before


https://github.com/software-mansion/react-native-screens/assets/23281839/069a2df7-ebb6-4de5-b8be-d5e1d045421b


### After


https://github.com/software-mansion/react-native-screens/assets/23281839/c3cbd102-4c68-4749-b2a0-4eaba6b8703f

## Test code and steps to reproduce

You can use Test2008 from TestsExample and FabricTestExample to test the
change from this PR.

## Checklist

- [X] Included code example that can be used to test this change
- [x] Ensured that CI passes
ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this issue Oct 9, 2024
)

## Description

On iOS there's a bug with contained (and contained transparent) modals,
where the modal does not change its orientation when the whole interface
(and/or device) is being rotated.
That's because contained modals will keep its orientation when the
interface is being rotated, as their controllers don't update the bounds
-> `viewDidLayoutSubviews` and `updateBounds` methods were not called at
the moment of changing the orientation.

This PR fixes this bug by updating the bounds when `layoutSubviews`
method from RNSScreenStack is being called, meaning that no matter if
there were previously any modals that have been dismissed or not, screen
stack will always update the bounds of the modal views.

Fixes software-mansion#2006.

## Changes

- Fixed the problem with invalid orientation of contained modals by
updating their bounds.

## Screenshots / GIFs

### Before


https://github.com/software-mansion/react-native-screens/assets/23281839/069a2df7-ebb6-4de5-b8be-d5e1d045421b


### After


https://github.com/software-mansion/react-native-screens/assets/23281839/c3cbd102-4c68-4749-b2a0-4eaba6b8703f

## Test code and steps to reproduce

You can use Test2008 from TestsExample and FabricTestExample to test the
change from this PR.

## Checklist

- [X] Included code example that can be used to test this change
- [x] Ensured that CI passes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Missing info The user didn't precise the problem enough Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants