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

[screen-orientation][ios] Screen Orientation module audit #22152

Merged
merged 38 commits into from
Jun 13, 2023

Conversation

behenate
Copy link
Member

@behenate behenate commented Apr 17, 2023

Why

Screen Orientation module is written in old modules API.
Screen orientation doesn't work correctly on iOS 16+ #21974, #21044
Various minor bugs (see below).

ENG-8363

How

Migration: Followed the usual migration steps.

Bugfixes:

  • Moved to new screen orientation API on iOS 16
  • Most of the issues below were fixed by using supportedInterfaceOrientationsFor in AppDelegates
  • Fixed the bug on iOS <= 15 where safe area view would fail when locked in orientaion X, device in orientation Y and pulled down the quick settings menu
  • Now when locked to portrait/landscape the status and navigation bars won't rotate freely.
  • In general the orientation behaviour is now basically the same as in native apps.
  • Due to recent changes dev-menu causes the app to crash on assert in requestOverlayMetricsIfNeeded function. When in landscape mode the notch height would be returned as negative, which causes the assertion to fail. I corrected it to always return values greater or equal than zero.

Here are some videos comparing old and new behaviours:

Old version is on the left, post-audit version is on the right. If the videos aren't loading try using Google Chrome.

iOS <=15 iOS <= 15
2023-04-26.18-17-21.mov
2023-04-26.18-18-25.mov
iOS <=15 iOS >= 16
2023-04-26.18-23-44.mov
2023-04-26.18-31-20.mov

Test Plan

Tested on simulator and physical devices (iOS 15 and 16).

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Apr 17, 2023
@expo-bot expo-bot added bot: needs changes ExpoBot found things that don't meet our guidelines and removed bot: suggestions ExpoBot has some suggestions labels Apr 24, 2023
@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions and removed bot: needs changes ExpoBot found things that don't meet our guidelines labels Apr 24, 2023
…and fixed SafeAreaView failing on rotation when no lock has been set
…ed SafeAreaView failing on rotation when no lock has been set
…it/migrate-to-swift' into @behenate/screen-orientation-audit/migrate-to-swift
…it/migrate-to-swift' into @behenate/screen-orientation-audit/migrate-to-swift

# Conflicts:
#	packages/expo-screen-orientation/ios/EXScreenOrientation/ScreenOrientationAppDelegate.swift
@expo expo deleted a comment from expo-bot Apr 26, 2023
@behenate behenate changed the title [screen-orientation][ios] Migrate screen-orientation to new Modules API [screen-orientation][ios] Migrate to new the Modules API. Performed a general module audit. Apr 28, 2023
@behenate behenate changed the title [screen-orientation][ios] Migrate to new the Modules API. Performed a general module audit. [screen-orientation][ios] Migrate to new the Modules API. Perform an audit of the module. Apr 28, 2023
@behenate behenate changed the title [screen-orientation][ios] Migrate to new the Modules API. Perform an audit of the module. [screen-orientation][ios] Module audit May 4, 2023
@behenate behenate changed the title [screen-orientation][ios] Module audit [screen-orientation][ios] Screen Orientation module audit May 4, 2023
behenate and others added 3 commits May 4, 2023 11:40
Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
@tsapeta tsapeta merged commit 0b761b6 into main Jun 13, 2023
@tsapeta tsapeta deleted the @behenate/screen-orientation-audit/migrate-to-swift branch June 13, 2023 08:26
@watadarkstar
Copy link

watadarkstar commented Jun 13, 2023

@tsapeta @behenate Can I try this out in a nightly build? Does Expo build nightly?

For example, can I do this:

yarn add expo-screen-orientation@nightly

@tsapeta
Copy link
Member

tsapeta commented Jun 13, 2023

@watadarkstar we don't build nightlies yet, but I'm going to publish some packages today, including expo-screen-orientation 😉 I'll let you know here!

@tsapeta tsapeta added the published Changes from the PR have been published to npm label Jun 13, 2023
@tsapeta
Copy link
Member

tsapeta commented Jun 13, 2023

@watadarkstar
It's out in 6.0.0-beta.1, you can use yarn add expo-screen-orientation@next to install it 🙂
Let us know your feedback 😊

@liquidvisual
Copy link

liquidvisual commented Jun 14, 2023

I couldn't seem to get this working when installing the pods, with SDK44 (bare workflow) - can't update at the moment unfortunately:

Command `pod install` failed.
└─ Cause: Failed to load 'ExpoScreenOrientation' podspec: 
[!] Invalid `ExpoScreenOrientation.podspec` file: undefined method `install_modules_dependencies' for Pod:Module.

@tsapeta
Copy link
Member

tsapeta commented Jun 14, 2023

@liquidvisual
I'm sure it will not work with that old SDK version. The install_modules_dependencies method comes from React Native and was added probably in 0.71.
I would recommend upgrading to this version and Expo SDK 48 (49 will be out in a few weeks though).

@singh-sukhmanjit
Copy link

Am able to rotate orientation using ScreenOrientation.lockAsync(orientationLock), but it doesnt lock it. If I rotate my phone then orientation changes accordingly.

@behenate
Copy link
Member Author

@singh-sukhmanjit What iOS version are you using, also are you using expo go or bare workflow? Code which you are using to lock the screen would be helpful too.

@watadarkstar
Copy link

watadarkstar commented Jun 15, 2023

@behenate @tsapeta I tested this on an iPhone 12 mini on iOS 16.2 using the package 6.0.0-beta.1 and its working great. Thank you! 🙏

I did yarn add expo-screen-orientation@next to install it.

@behenate @tsapeta Any idea when this will go into an actual release?

@tsapeta
Copy link
Member

tsapeta commented Jun 15, 2023

@watadarkstar
Thanks for the feedback! If it works for you, I think you can already use it 😉 We'll probably release the exact same version as stable v6 next week and it will be included in SDK 49 (at the end of the month).

@singh-sukhmanjit
Copy link

@behenate Am trying it on iPad (8th generation) iPadOS 16.1 with Expo SDK 48 bare workflow. Am using ScreenOrientation.lockAsync(ScreenOrientation.OrientationLock.LANDSCAPE_LEFT); to lock the screen. Info.plist includes:

<key>UISupportedInterfaceOrientations</key>
<array>
    <string>UIInterfaceOrientationPortrait</string>
    <string>UIInterfaceOrientationLandscapeLeft</string>
    <string>UIInterfaceOrientationLandscapeRight</string>
</array>

@behenate
Copy link
Member Author

behenate commented Jun 15, 2023

@singh-sukhmanjit Have you set the "requireFullScreen" option to true in app.json?
You can also try setting it to true directly in Info.plist.

@singh-sukhmanjit
Copy link

@behenate yes, it is set but still the behavior is same.

@jasonz1987
Copy link

still not work no ios 16.2 iphone simulator (bare workflow)

xcode log

2023-06-17 12:16:14.903751+0800 app[32163:174205] [Orientation] BUG IN CLIENT OF UIKIT: Setting UIDevice.orientation is not supported. Please use UIWindowScene.requestGeometryUpdate(_:)

my env:

system:
    OS: macOS 12.6.3
    CPU: (16) x64 Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
    Memory: 27.57 MB / 32.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.10.0 - ~/.nvm/versions/node/v16.10.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v16.10.0/bin/yarn
    npm: 7.24.0 - ~/.nvm/versions/node/v16.10.0/bin/npm
    Watchman: 2023.05.22.00 - /usr/local/bin/watchman
  Managers:
    CocoaPods: 1.12.1 - /Users/jasonz/.rbenv/shims/pod
  SDKs:
    iOS SDK:
      Platforms: DriverKit 22.2, iOS 16.2, macOS 13.1, tvOS 16.1, watchOS 9.1
    Android SDK:
      Android NDK: 23.0.7344513-beta4
  IDEs:
    Android Studio: 2021.2 AI-212.5712.43.2112.8609683
    Xcode: 14.2/14C18 - /usr/bin/xcodebuild
  Languages:
    Java: 11.0.18 - /usr/bin/javac
  npmPackages:
    @react-native-community/cli: Not Found
    react: 18.2.0 => 18.2.0
    react-native: 0.71.8 => 0.71.8
    react-native-macos: Not Found
  npmGlobalPackages:
    *react-native*: Not Found

package.json

   "expo-screen-orientation": "^6.0.0-beta.1",
   "expo": "~48.0.18",

@behenate
Copy link
Member Author

behenate commented Jun 18, 2023

@jasonz1987
I managed to replicate your issue. The problem is that the expo-modules-core part of the audit hasn't been published yet and in some cases its crucial for the screen-orientation module to work correctly. The necessary code change is small and has already been merged, but you will have to wait a bit until we publish it #22599.
For now you can try to paste the code from the PR into ExpoAppDelegate.swift rebuild the project and see if it works correctly for you.

It is also possible that your app.json is not configured correctly. If your app.json contains the following line: "orientation": "portrait" you should remove it and then add configuration line for the config plugin with your desired initial orientation lock example. Then run npx expo prebuild and rebuild your project.

As for the error it is most likely caused by react-native-screens (they are used by react-navigation if you are using it) which is still trying to use the old orientation API on newer iOS versions, this shouldn't have any impact on expo-screen-orientation. If you install 3.21.0 version of react-native-screens the error shouldn't appear anymore.

@jasonz1987
Copy link

@behenate the pr #22599 works, thanks a a lot.

@ycldz
Copy link

ycldz commented Aug 7, 2023

Expo: "~48.0.15"
expo-screen-orientation: "~5.1.1"
IOS Simulator: Iphone 14 Pro Max / v16.4
Real Device: Iphone 11 / v16.6

app.json in orientation deleted

app.json plugins in added
[
"expo-screen-orientation",
{
"initialOrientation": "DEFAULT"
}
],

await ScreenOrientation.lockAsync(ScreenOrientation.OrientationLock.LANDSCAPE);

It works fine on the simulator but not on the real device, has anyone solved this problem?

@behenate
Copy link
Member Author

behenate commented Aug 7, 2023

@ycldz
Hi, please upgrade to SDK 49 and screen orientation 6.0.5 and try again. (Don't upgrade only ScreenOrientation as it relies on some changes made in SDK 49)

Also please don't leave three identical comments in three different places

@ycldz
Copy link

ycldz commented Aug 7, 2023

@behenate unfortunately it doesn't work on expo 49 either.

@ycldz
Copy link

ycldz commented Aug 7, 2023

@behenate I save the code in the editor, the screen does not refresh on the device and the console.log code does not work thank you :D

@behenate
Copy link
Member Author

behenate commented Aug 8, 2023

@ycldz Are you using managed or bare workflow? I tested in a bare workflow with this app https://github.com/behenate/screen-orientation-demo and it seems to be working fine

@Darkhorse-Fraternity
Copy link

image
it doesn't work on expo 49 either.

@Darkhorse-Fraternity
Copy link

@ycldz Are you using managed or bare workflow? I tested in a bare workflow with this app https://github.com/behenate/screen-orientation-demo and it seems to be working fine

The reproduction of this issue requires the simultaneous use of screen-orientation and react-navigation.

@Darkhorse-Fraternity
Copy link

export const useLandscape = () => {
  useEffect(() => {
      void InteractionManager.runAfterInteractions(() => {
        void ScreenOrientation.lockAsync(
          ScreenOrientation.OrientationLock.LANDSCAPE_RIGHT,
        );
        // ...long-running synchronous task...
      });

    return () => {
      void InteractionManager.runAfterInteractions(() => {
        void ScreenOrientation.lockAsync(
          ScreenOrientation.OrientationLock.PORTRAIT_UP,
        );
        // ...long-running synchronous task...
      });
    };
  }, []);
};

Putting this feature on the page after the push will have a certain probability of recurring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.