From 7d42106d4ce20c644bda4d928fb0abc163580cee Mon Sep 17 00:00:00 2001 From: lbaldy Date: Tue, 28 Jun 2022 08:56:25 -0700 Subject: [PATCH] prevent from publishing dimensions change event when app changes state (#34014) Summary: This fix solves a problem very well evaluated [here](https://github.com/Expensify/App/issues/2727) as well as this [one](https://github.com/facebook/react-native/issues/29290). The issue is that when the app goes to background, in landscape mode, the RCTDeviceInfo code triggers an orientation change event that did not physically happen. Due to that, we get swapped values returned when going back to the app. I debugged the react-native code, and to me it seems that react native publishes the orientation change event one extra time when switching the state of the app to 'inactive'. Here is what is happening: 1. iPad is in landscape. 2. We move the app to inactive state. 3. Native Code queues portrait orientation change (no such change happened physically), and immediately after it triggers landscape change (same as we had in point 1). 4. We restore the app to active state. 5. The app receives two queued orientation change events, one after another. 6. The quick transition between portrait and landscape happens even though it never went back to portrait. Fresh `react-native init` app repro case can be found here: https://github.com/lbaldy/issue-34014-repro Video presenting the issue (recorded while working on: https://github.com/Expensify/App/issues/2727 ): https://www.youtube.com/watch?v=nFDOml9M8w4 ## Changelog [iOS] [Fixed] - Fix the way the orientation events are published, to avoid false publish on orientation change when app changes state to inactive Pull Request resolved: https://github.com/facebook/react-native/pull/34014 Test Plan: ### Test Preparation 1. Make sure you have a working version of E/App. 2. Open App/src/components/withWindowDimensions.js and update the constructor by changing this line: `this.onDimensionChange = _.debounce(this.onDimensionChange.bind(this), 100);` to `this.onDimensionChange = this.onDimensionChange.bind(this);` 3. Open the NewExpensify.xcodeproj in xCode. 4. Open the RCTDeviceInfo.mm file and replace it's contents with the file from this PR. 5. Select your device of choice (I suggest starting with iPad mini) and run the app though xCode. 6. From this point you can move to the test scenarios described below. ### iPad Mini tests: Reproduction + Fix test video (Test 1): https://youtu.be/jyzoNHLYHPo Reproduction + Fix test video (Test 2): https://youtu.be/CLimE-Fba-g **Test 1:** 1. Launch app in portrait, open chat - no sidebar visible. 7. Switch to landscape - sidebar shows. 8. Put app to background. 9. Put app back to foreground - make sure the side menu doesn't flicker. **Test 2:** 1. Launch app in portrait, open chat - no sidebar visible. 2. Switch to landscape - sidebar shows. 3. Put app to background. Switch orientation back to portrait. 4. Put app back to foreground - make sure the side menu hides again as it should be in portrait. ### iPad Pro tests: Reproduction + Fix test video (Test 3, Test 4): https://youtu.be/EJkUUQCiLRg iPad mini test 1 applies. Scenario 2 does not as the screen is too wide in both orientations and iPad pro shows sidebar always. **Test 3:** 1. launch the app. 2. Make sure you're in landscape mode. 3. See split screen with some other app. Make sure the side bar is visible. 4. Play with the size of the view, resize it a bit. When the view shrinks it should hide the sidebar, when it grows it should show it. 10. Move the app to background and back to foreground, please observe there are no flickers. **Test 4:** 1. Launch the app. 2. Make sure you're in landscape mode. 3. Make the multitasking view and make Expensify app a slide over app. 4. Move back to fullscreen/split screen. Make sure the menu is shown accordingly 5. Move the app to background and back to foreground, please observe there are no flickers. ### iPhone: Non reg with and without the fix video: https://youtu.be/kuv9in8vtbk Please perform standard smoke tests on transformation changes. Reviewed By: cipolleschi Differential Revision: D37239891 Pulled By: jacdebug fbshipit-source-id: e6090153820e921dcfb0d823e0377abd25225bdf --- React/Base/RCTUtils.h | 2 ++ React/Base/RCTUtils.m | 5 ++++ React/CoreModules/RCTDeviceInfo.mm | 39 +++++++++++++++++++++--------- 3 files changed, 35 insertions(+), 11 deletions(-) diff --git a/React/Base/RCTUtils.h b/React/Base/RCTUtils.h index 135ff2f160da23..a9502dd215a93f 100644 --- a/React/Base/RCTUtils.h +++ b/React/Base/RCTUtils.h @@ -186,4 +186,6 @@ RCT_EXTERN BOOL RCTValidateTypeOfViewCommandArgument( NSString const *commandName, NSString const *argPos); +RCT_EXTERN BOOL RCTIsAppActive(void); + NS_ASSUME_NONNULL_END diff --git a/React/Base/RCTUtils.m b/React/Base/RCTUtils.m index 961cc36b24d0c8..4f141146ba9acb 100644 --- a/React/Base/RCTUtils.m +++ b/React/Base/RCTUtils.m @@ -1067,3 +1067,8 @@ RCT_EXTERN BOOL RCTValidateTypeOfViewCommandArgument( return true; } + +BOOL RCTIsAppActive(void) +{ + return [RCTSharedApplication() applicationState] == UIApplicationStateActive; +} diff --git a/React/CoreModules/RCTDeviceInfo.mm b/React/CoreModules/RCTDeviceInfo.mm index 46e313615a4afb..78e53899778bf6 100644 --- a/React/CoreModules/RCTDeviceInfo.mm +++ b/React/CoreModules/RCTDeviceInfo.mm @@ -26,6 +26,7 @@ @interface RCTDeviceInfo () @implementation RCTDeviceInfo { UIInterfaceOrientation _currentInterfaceOrientation; NSDictionary *_currentInterfaceDimensions; + BOOL _isFullscreen; } @synthesize bridge = _bridge; @@ -60,7 +61,7 @@ - (void)initialize _currentInterfaceDimensions = RCTExportedDimensions(_moduleRegistry, _bridge); [[NSNotificationCenter defaultCenter] addObserver:self - selector:@selector(interfaceFrameDidChange) + selector:@selector(interfaceOrientationDidChange) name:UIApplicationDidBecomeActiveNotification object:nil]; @@ -173,22 +174,36 @@ - (void)interfaceOrientationDidChange - (void)_interfaceOrientationDidChange { - UIInterfaceOrientation nextOrientation = [RCTSharedApplication() statusBarOrientation]; + UIApplication *application = RCTSharedApplication(); + UIInterfaceOrientation nextOrientation = [application statusBarOrientation]; + + BOOL isRunningInFullScreen = + CGRectEqualToRect(application.delegate.window.frame, application.delegate.window.screen.bounds); + // We are catching here two situations for multitasking view: + // a) The app is in Split View and the container gets resized -> !isRunningInFullScreen + // b) The app changes to/from fullscreen example: App runs in slide over mode and goes into fullscreen-> + // isRunningInFullScreen != _isFullscreen The above two cases a || b can be shortened to !isRunningInFullScreen || + // !_isFullscreen; + BOOL isResizingOrChangingToFullscreen = !isRunningInFullScreen || !_isFullscreen; + BOOL isOrientationChanging = (UIInterfaceOrientationIsPortrait(_currentInterfaceOrientation) && + !UIInterfaceOrientationIsPortrait(nextOrientation)) || + (UIInterfaceOrientationIsLandscape(_currentInterfaceOrientation) && + !UIInterfaceOrientationIsLandscape(nextOrientation)); // Update when we go from portrait to landscape, or landscape to portrait - if ((UIInterfaceOrientationIsPortrait(_currentInterfaceOrientation) && - !UIInterfaceOrientationIsPortrait(nextOrientation)) || - (UIInterfaceOrientationIsLandscape(_currentInterfaceOrientation) && - !UIInterfaceOrientationIsLandscape(nextOrientation))) { + // Also update when the fullscreen state changes (multitasking) and only when the app is in active state. + if ((isOrientationChanging || isResizingOrChangingToFullscreen) && RCTIsAppActive()) { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wdeprecated-declarations" [[_moduleRegistry moduleForName:"EventDispatcher"] sendDeviceEventWithName:@"didUpdateDimensions" body:RCTExportedDimensions(_moduleRegistry, _bridge)]; + // We only want to track the current _currentInterfaceOrientation and _isFullscreen only + // when it happens and only when it is published. + _currentInterfaceOrientation = nextOrientation; + _isFullscreen = isRunningInFullScreen; #pragma clang diagnostic pop } - - _currentInterfaceOrientation = nextOrientation; } - (void)interfaceFrameDidChange @@ -203,15 +218,17 @@ - (void)_interfaceFrameDidChange { NSDictionary *nextInterfaceDimensions = RCTExportedDimensions(_moduleRegistry, _bridge); - if (!([nextInterfaceDimensions isEqual:_currentInterfaceDimensions])) { + // update and publish the even only when the app is in active state + if (!([nextInterfaceDimensions isEqual:_currentInterfaceDimensions]) && RCTIsAppActive()) { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wdeprecated-declarations" [[_moduleRegistry moduleForName:"EventDispatcher"] sendDeviceEventWithName:@"didUpdateDimensions" body:nextInterfaceDimensions]; + // We only want to track the current _currentInterfaceOrientation only + // when it happens and only when it is published. + _currentInterfaceDimensions = nextInterfaceDimensions; #pragma clang diagnostic pop } - - _currentInterfaceDimensions = nextInterfaceDimensions; } - (std::shared_ptr)getTurboModule:(const ObjCTurboModule::InitParams &)params