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): invalid initial orientation of screen underneath full screen modal #1848

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

kkafar
Copy link
Member

@kkafar kkafar commented Jul 28, 2023

Description

In case of fullScreenModal we want to ask for orientation specifically the screen underneath,
so that when the modal is dismissed the controller underneath is already in expected orientation, without the need to first wait for the dimissal to complete and only then to rotate.

This still is not perfect, as you see the visual glitches / jumping on videos below, however it improves the situation.

Changes

Asked the screen that presents another screen in fullScreenModal stack presentation for interface orientation mask.

Before

Simulator.Screen.Recording.-.iPhone.14.-.2023-07-31.at.10.51.05.mp4

After

Simulator.Screen.Recording.-.iPhone.14.-.2023-07-31.at.10.48.29.mp4

Test code and steps to reproduce

Test1775 in TE

Checklist

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

@LucasHimelfarb
Copy link

this PR should be merged

@kkafar kkafar force-pushed the @kkafar/another-rotation-issue branch from 18dd3d2 to d9c3cdb Compare November 16, 2023 14:22
@matthargett
Copy link

is this waiting on something in particular before it's merged and released?

@Jpunt
Copy link

Jpunt commented Feb 2, 2024

I'm also wondering: Is this going to be merged any time soon?

@kkafar
Copy link
Member Author

kkafar commented Feb 2, 2024

This PR is waiting for me having some free throughput and finializing it. As far as I remember the solution was not perfect and didn't solve the problem completely.
Have you tested this PR, does it work for you?

(you can install react-native-screens directly from github by putting "react-native-screens": "software-mansion/react-native-screens#@kkafar/another-rotation-issue" in your package json)

@Jpunt
Copy link

Jpunt commented Feb 12, 2024

@kkafar the solution seems to be working great in our first testing! One thing we've spotted so far is that the home-indicator (for iPhones without home-button) transitions a bit wonky.

@jim-lub can you remember anything else?

@kkafar
Copy link
Member Author

kkafar commented Feb 13, 2024

Hey, I'll look into it rn, will report back to you once I have something

@kkafar
Copy link
Member Author

kkafar commented Feb 13, 2024

Also: If the PR in its current shape works for you, you can install react-native-screens directly from the branch / commit hash, by putting following in your package.json:

"react-native-screens": "software-mansion/react-native-screens#@kkafar/another-rotation-issue"

(writing from memory, lookout for typos)

@kkafar kkafar changed the title fix(iOS): invalid initial orientation of screen underneath modal fix(iOS): invalid initial orientation of screen underneath full screen modal Feb 13, 2024
@kkafar kkafar requested a review from tboba February 13, 2024 13:18
@kkafar kkafar self-assigned this Feb 13, 2024
@vargajacint
Copy link

I tested it on a real device and found the exiting animation is wrong: It's going from left to right not top to bottom if you are in landscape mode.

@kkafar
Copy link
Member Author

kkafar commented Feb 13, 2024

I tested it on a real device and found the exiting animation is wrong: It's going from left to right not top to bottom if you are in landscape mode.

Would you mind providing screen recordings visualising what do you mean?

As I'm testing on iPhone 12 mini right now & I do not see difference between simulator and real device.

@vargajacint
Copy link

vargajacint commented Feb 13, 2024

I tested it on a real device and found the exiting animation is wrong: It's going from left to right not top to bottom if you are in landscape mode.

Would you mind providing screen recordings visualising what do you mean?

As I'm testing on iPhone 12 mini right now & I do not see difference between simulator and real device.

As you can see, if you keep your phone in landscape mode the screen (navigation.goBack) is closing from left to right (aka top->bottom in portrait mode)

https://github.com/software-mansion/react-native-screens/assets/42407460/cdfb58ca-f758-4b51-b280-61296ac96b25
iPhone 14 Pro

@kkafar
Copy link
Member Author

kkafar commented Feb 13, 2024

Yeah, for now it is expected (however I see why this might be a subject for change). IIRC the stack animation reflects the orientation of the screen underneath, so if it is in portrait mode, the dismissal will be top-down from the perspective of the screen underneath (looking like it's left to right from the perspective of screen in landscape mode)

@kkafar
Copy link
Member Author

kkafar commented Feb 13, 2024

I'm resentful to merge this PR as it introduces more bugs than it fixes, e.g.
image and various weird interface jumps.

I'll need to spend more time on it.

@aleqsio
Copy link
Contributor

aleqsio commented Mar 4, 2024

@Jpunt you can use this patch file to install the patch without needing to keep the branch updated. remember to rerun pod install :)

https://gist.github.com/aleqsio/b1cb8bd24f34ed697e21b0f28a1efe68 – put this file into the patches folder.

you can use patch-package for this.

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.

7 participants