react-native-screens icon indicating copy to clipboard operation
react-native-screens copied to clipboard

fix(iOS): invalid initial orientation of screen underneath full screen modal

Open kkafar opened this issue 1 year ago • 13 comments

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

https://github.com/software-mansion/react-native-screens/assets/50801299/7f30ca37-9e67-4812-982e-b6144cce827e

After

https://github.com/software-mansion/react-native-screens/assets/50801299/47f0f57b-46e6-4adc-9bd7-1f87613106d8

Test code and steps to reproduce

Test1775 in TE

Checklist

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

kkafar avatar Jul 28 '23 15:07 kkafar

this PR should be merged

LucasHimelfarb avatar Oct 31 '23 15:10 LucasHimelfarb

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

matthargett avatar Jan 10 '24 20:01 matthargett

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

Jpunt avatar Feb 02 '24 13:02 Jpunt

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)

kkafar avatar Feb 02 '24 16:02 kkafar

@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?

Jpunt avatar Feb 12 '24 11:02 Jpunt

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

kkafar avatar Feb 13 '24 12:02 kkafar

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 avatar Feb 13 '24 12:02 kkafar

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.

vargajacint avatar Feb 13 '24 13:02 vargajacint

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.

kkafar avatar Feb 13 '24 13:02 kkafar

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

vargajacint avatar Feb 13 '24 13:02 vargajacint

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 avatar Feb 13 '24 13:02 kkafar

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.

kkafar avatar Feb 13 '24 13:02 kkafar

@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.

aleqsio avatar Mar 04 '24 15:03 aleqsio