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): Fix updating bounds while changing interface orientation #1970

Merged
merged 10 commits into from
May 8, 2024

Conversation

tboba
Copy link
Member

@tboba tboba commented Nov 15, 2023

Description

Currently while trying to change the interface orientation there's a bug that doesn't layout the next screen, causing the old frame to remain (which means that the next screen will be rotated regarding to the previous orientation.
This PR fixes that problem by calling [_controller.view setNeedsLayout] on the navigationController, where the view is being rotated.

When I was testing this PR I also came to another solution of setting the frame in viewWillTransitionToSize but I didn't spot any differences between one solution and this one.

- (void)viewWillTransitionToSize:(CGSize)size
       withTransitionCoordinator:(id<UIViewControllerTransitionCoordinator>)coordinator
{
  [super viewWillTransitionToSize:size withTransitionCoordinator:coordinator];
#ifdef RCT_NEW_ARCH_ENABLED
  [coordinator
      animateAlongsideTransition:^(id<UIViewControllerTransitionCoordinatorContext> coordinator) {
        self.screenView.frame = CGRectMake(0, 0, size.width, size.height);
      }
      completion:^(id<UIViewControllerTransitionCoordinatorContext> coordinator) {
        [self.screenView setNeedsLayout];
      }];
#endif
}

I've also tried to make a workaround of a reaaaally old bug in React Native: facebook/react-native#16060, but unfortunately I failed with that 😕

Closes #1738.

Changes

  • Adds call to setNeedsLayout during the interface orientation

Screenshots / GIFs

Before

Screen.Recording.2023-11-15.at.14.36.19.mov

After

Screen.Recording.2023-11-15.at.14.33.58.mov

Test code and steps to reproduce

You can find in this PR Test1970.tsx that contains test test I've tested while making this change.

Checklist

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

@tboba tboba changed the title fix(iOS): Fix updating bounds on new architecture fix(iOS): Fix updating bounds while changing interface orientation Nov 15, 2023
@tboba tboba requested a review from kkafar November 16, 2023 09:49
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.

As we discussed: the iOS change is ok, but we still need to work on the view clipping (or at least determine why the view is clipped) & I would suggest that we remove Test1970 & reuse some other test example with orientation change (if there is one).

@tboba tboba force-pushed the @tboba/fix-updating-bounds branch from 9b95c1d to 42d9c36 Compare November 16, 2023 14:06
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.

One more note

ios/RNSScreenStack.mm Show resolved Hide resolved
@dani-mp
Copy link

dani-mp commented Apr 9, 2024

Are you folks planning on getting this merged? There are a bunch of related issues reported in different repositories about this: expo/expo#27228 (comment).

@Will94o
Copy link

Will94o commented Apr 17, 2024

Hello ! thanks for the fix but when will it be merged ?

@dani-mp
Copy link

dani-mp commented Apr 17, 2024

In case folks are having problems with this too, I could have a non-buggy and consistent behavior using stacks with different orientations (some landscape, some portrait) using React Navigation's orientation screen prop. I'm developing a game and some stacks must be always in landscape, and the rest (login, onboarding, settings, and so on) are always portrait. When I tried implementing this with expo-screen-orientation, it was buggy.

@tboba
Copy link
Member Author

tboba commented May 6, 2024

Hi @dani-mp! Could you suggest such patch for changing the orientation? I wonder if we could avoid calling setNeedsLayout on iOS.

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.

I think we can process with this PR

@@ -923,6 +923,7 @@ - (void)navigationController:(UINavigationController *)navigationController
{
[self emitOnFinishTransitioningEvent];
[RNSScreenWindowTraits updateWindowTraits];
[_controller.view setNeedsLayout];
Copy link
Member

Choose a reason for hiding this comment

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

One single note, let's leave a comment above this line why do we trigger layout here (e.g. link to this PR).

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.

Yeah, thanks for that comment.
I think we are good to go.

@tboba tboba merged commit 1e75e0c into main May 8, 2024
7 checks passed
@tboba tboba deleted the @tboba/fix-updating-bounds branch May 8, 2024 11:52
@sanghavip16
Copy link

@tboba can you please release new React Native npm package of "react-native-screens" with above changes?

@kkafar
Copy link
Member

kkafar commented Jun 13, 2024

We're in release process currently, however still waiting for few PRs to land on main. I won't promise anything, cause last date I promised was that it will be ready for week ago, however it should be soon

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

## Description

Currently while trying to change the interface orientation there's a bug
that doesn't layout the next screen, causing the old frame to remain
(which means that the next screen will be rotated regarding to the
previous orientation.
This PR fixes that problem by calling `[_controller.view
setNeedsLayout]` on the `navigationController`, where the view is being
rotated.

When I was testing this PR I also came to another solution of setting
the frame in `viewWillTransitionToSize` but I didn't spot any
differences between one solution and this one.

```objc
- (void)viewWillTransitionToSize:(CGSize)size
       withTransitionCoordinator:(id<UIViewControllerTransitionCoordinator>)coordinator
{
  [super viewWillTransitionToSize:size withTransitionCoordinator:coordinator];
#ifdef RCT_NEW_ARCH_ENABLED
  [coordinator
      animateAlongsideTransition:^(id<UIViewControllerTransitionCoordinatorContext> coordinator) {
        self.screenView.frame = CGRectMake(0, 0, size.width, size.height);
      }
      completion:^(id<UIViewControllerTransitionCoordinatorContext> coordinator) {
        [self.screenView setNeedsLayout];
      }];
#endif
}
```

I've also tried to make a workaround of a reaaaally old bug in React
Native: facebook/react-native#16060, but
unfortunately I failed with that 😕

Closes software-mansion#1738.

## Changes

- Adds call to `setNeedsLayout` during the interface orientation

## Screenshots / GIFs

### Before


https://github.com/software-mansion/react-native-screens/assets/23281839/bcc366ab-9757-4a31-89e0-241c22cdd5c6

### After


https://github.com/software-mansion/react-native-screens/assets/23281839/6634379a-2697-46a3-aed6-3cb156616817

## Test code and steps to reproduce

You can find in this PR `Test1970.tsx` that contains test test I've
tested while making this change.

## 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect layout when navigating between screens of different orientation on iOS
5 participants