prevent from publishing dimensions change event when app changes state #34014

lbaldy · 2022-06-15T15:58:48Z

Summary

This fix solves a problem very well evaluated here as well as this one.

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:

iPad is in landscape.
We move the app to inactive state.
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).
We restore the app to active state.
The app receives two queued orientation change events, one after another.
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: Expensify/App#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

Test Plan

Test Preparation

Make sure you have a working version of E/App.
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);

Open the NewExpensify.xcodeproj in xCode.
Open the RCTDeviceInfo.mm file and replace it's contents with the file from this PR.
Select your device of choice (I suggest starting with iPad mini) and run the app though xCode.
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:

Launch app in portrait, open chat - no sidebar visible.
Switch to landscape - sidebar shows.
Put app to background.
Put app back to foreground - make sure the side menu doesn't flicker.

Test 2:

Launch app in portrait, open chat - no sidebar visible.
Switch to landscape - sidebar shows.
Put app to background. Switch orientation back to portrait.
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:

launch the app.
Make sure you're in landscape mode.
See split screen with some other app. Make sure the side bar is visible.
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.
Move the app to background and back to foreground, please observe there are no flickers.

Test 4:

Launch the app.
Make sure you're in landscape mode.
Make the multitasking view and make Expensify app a slide over app.
Move back to fullscreen/split screen. Make sure the menu is shown accordingly
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.

…ent when it doesn't apply

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

… name, add more comments describing it

prevent from publishing dimensions change event when app changes state

analysis-bot · 2022-06-15T16:28:20Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,788,854	-40,728
android	hermes	armeabi-v7a	7,177,896	-38,390
android	hermes	x86	8,098,714	-40,731
android	hermes	x86_64	8,076,695	-43,682
android	jsc	arm64-v8a	9,654,628	-40,770
android	jsc	armeabi-v7a	8,412,418	-38,433
android	jsc	x86	9,605,227	-40,790
android	jsc	x86_64	10,200,101	-43,723

Base commit: a5a956b
Branch: main

analysis-bot · 2022-06-15T18:38:18Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 7fb0bb4
Branch: main

facebook-github-bot · 2022-06-17T13:51:06Z

@jacdebug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cipolleschi

Hi @lbaldy, thanks for this long awaited fix. Unfortunately, we are unable to merge it right away because it is breaking our internal CI. I left some comments to help you fixing it, along with some style suggestions (feel free to discard them, if you don't like them).

Let me know if you have any questions!

React/CoreModules/RCTDeviceInfo.mm

Co-authored-by: Riccardo <riccardo.cipolleschi@gmail.com>

…ebook#34014 (comment)

facebook-github-bot · 2022-06-27T17:02:40Z

@jacdebug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cipolleschi · 2022-06-27T17:14:14Z

React/CoreModules/RCTDeviceInfo.mm

@@ -192,15 +195,15 @@ - (void)_interfaceOrientationDidChange
 // Update when we go from portrait to landscape, or landscape to portrait
 // Also update when the fullscreen state changes (multitasking) and only when the app is in active state.
 if ((isOrientationChanging || isResizingOrChangingToFullscreen) && isActive) {


isActive is used only once, here. You can use the RCTIsAppActive() function directly instead of saving its value in a variable.

React/CoreModules/RCTDeviceInfo.mm

cipolleschi

Thanks for the changes, I added a couple more stylistic improvements. Let's see if the internal CI is now happy!

lbaldy · 2022-06-28T05:47:27Z

@cipolleschi thanks for all your feedback. It seems that we're all green now ;)

facebook-github-bot · 2022-06-28T09:05:38Z

@jacdebug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cipolleschi · 2022-06-28T09:18:24Z

React/Base/RCTUtils.m

+BOOL RCTIsAppActive(void)
+{
+ return [RCTSharedApplication() applicationState] == UIApplicationStateActive;
+}


Sorry to bother, but our linter is asking a new line at the end of the file. Could you please add it?

done, and pushed

cipolleschi

Sorry, other changes from the linter... 😭

React/CoreModules/RCTDeviceInfo.mm

cipolleschi · 2022-06-28T09:55:56Z

React/CoreModules/RCTDeviceInfo.mm

@@ -201,17 +214,20 @@ - (void)interfaceFrameDidChange

 - (void)_interfaceFrameDidChange
 {
+ UIApplication *application = RCTSharedApplication();


could you also remove this variable that is unused? It's make our sanity checks exploding!

React/CoreModules/RCTDeviceInfo.mm

Co-authored-by: Riccardo <riccardo.cipolleschi@gmail.com>

facebook-github-bot · 2022-06-28T10:12:37Z

@jacdebug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-06-28T10:17:48Z

@jacdebug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

react-native-bot · 2022-06-28T17:15:04Z

This pull request was successfully merged by @lbaldy in 7d42106.

^{When will my fix make it into a release? | Upcoming Releases}

#34014) Summary: This fix solves a problem very well evaluated [here](Expensify/App#2727) as well as this [one](#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: Expensify/App#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: #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

#34014) Summary: This fix solves a problem very well evaluated [here](Expensify/App#2727) as well as this [one](#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: Expensify/App#2727 ): https://www.youtube.com/watch?v=nFDOml9M8w4  [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: #34014 Test Plan: 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. 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. 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. 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

facebook#34014) Summary: This fix solves a problem very well evaluated [here](Expensify/App#2727) as well as this [one](facebook#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: Expensify/App#2727 ): https://www.youtube.com/watch?v=nFDOml9M8w4  [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: facebook#34014 Test Plan: 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. 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. 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. 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

lbaldy and others added 8 commits May 24, 2022 08:59

fix useWindowDimensions, prevent from publishing dimensions change ev…

8653851

…ent when it doesn't apply

update code structure and add comments

b0c3bc7

improve comments

e21080d

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

simplify comments

b4b38d2

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

update comments

818a073

shorten the condition, rename the variable to give it more meaningful…

27e65a5

… name, add more comments describing it

Merge pull request Expensify#11 from lbaldy/bugfix/issue-2727

f690dc1

prevent from publishing dimensions change event when app changes state

Merge branch 'facebook:main' into master

0bee080

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jun 15, 2022

lbaldy mentioned this pull request Jun 15, 2022

[$16,000] Let's fix a React Native issue -- useWindowDimensions() returns swapped height and width in iOS Expensify/App#2727

Closed

5 tasks

react-native-bot added Bug Platform: iOS iOS applications. labels Jun 15, 2022

cipolleschi requested changes Jun 27, 2022

View reviewed changes

React/CoreModules/RCTDeviceInfo.mm Outdated Show resolved Hide resolved

React/CoreModules/RCTDeviceInfo.mm Outdated Show resolved Hide resolved

React/CoreModules/RCTDeviceInfo.mm Outdated Show resolved Hide resolved

React/CoreModules/RCTDeviceInfo.mm Outdated Show resolved Hide resolved

lbaldy and others added 3 commits June 27, 2022 17:06

improve code style

e5856b0

Co-authored-by: Riccardo <riccardo.cipolleschi@gmail.com>

improve code style

320cb11

Co-authored-by: Riccardo <riccardo.cipolleschi@gmail.com>

refactor based on PR review comments facebook#34014 (comment) and fac…

9d2be30

…ebook#34014 (comment)

cipolleschi reviewed Jun 27, 2022

View reviewed changes

React/CoreModules/RCTDeviceInfo.mm Outdated Show resolved Hide resolved

cipolleschi requested changes Jun 27, 2022

View reviewed changes

code style fixes

cabdc03

cipolleschi reviewed Jun 28, 2022

View reviewed changes

add new line at the end of file for the linter

394fd10

cipolleschi reviewed Jun 28, 2022

View reviewed changes

lbaldy and others added 2 commits June 28, 2022 12:09

formatting

23f6564

Co-authored-by: Riccardo <riccardo.cipolleschi@gmail.com>

formatting

ff62965

Co-authored-by: Riccardo <riccardo.cipolleschi@gmail.com>

remove ununsed variable

575f054

facebook-github-bot closed this in 7d42106 Jun 28, 2022

react-native-bot added the Merged This PR has been merged. label Jun 28, 2022

This was referenced Jun 29, 2022

[iOS] Dimensions API does not update values correctly when playing video in fullscreen #31692

Closed

iOS fullscreen Video - Breaks Dimensions API react-native-webview/react-native-webview#1872

Closed

kirgy mentioned this pull request Jan 10, 2023

useWindowDimensions reported incorrect values after device orientation change #35800

Closed

parasharrajat mentioned this pull request Jul 5, 2023

Update App to React Native 0.72 version Expensify/App#18507

Merged

57 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prevent from publishing dimensions change event when app changes state #34014

prevent from publishing dimensions change event when app changes state #34014

lbaldy commented Jun 15, 2022 •

edited

Loading

analysis-bot commented Jun 15, 2022 •

edited

Loading

analysis-bot commented Jun 15, 2022 •

edited

Loading

facebook-github-bot commented Jun 17, 2022

cipolleschi left a comment

facebook-github-bot commented Jun 27, 2022

cipolleschi Jun 27, 2022

cipolleschi left a comment

lbaldy commented Jun 28, 2022

facebook-github-bot commented Jun 28, 2022

cipolleschi Jun 28, 2022

lbaldy Jun 28, 2022

cipolleschi left a comment

cipolleschi Jun 28, 2022

lbaldy Jun 28, 2022

facebook-github-bot commented Jun 28, 2022

facebook-github-bot commented Jun 28, 2022

react-native-bot commented Jun 28, 2022

prevent from publishing dimensions change event when app changes state #34014

prevent from publishing dimensions change event when app changes state #34014

Conversation

lbaldy commented Jun 15, 2022 • edited Loading

Summary

Changelog

Test Plan

Test Preparation

iPad Mini tests:

iPad Pro tests:

iPhone:

analysis-bot commented Jun 15, 2022 • edited Loading

analysis-bot commented Jun 15, 2022 • edited Loading

facebook-github-bot commented Jun 17, 2022

cipolleschi left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 27, 2022

cipolleschi Jun 27, 2022

Choose a reason for hiding this comment

cipolleschi left a comment

Choose a reason for hiding this comment

lbaldy commented Jun 28, 2022

facebook-github-bot commented Jun 28, 2022

cipolleschi Jun 28, 2022

Choose a reason for hiding this comment

lbaldy Jun 28, 2022

Choose a reason for hiding this comment

cipolleschi left a comment

Choose a reason for hiding this comment

cipolleschi Jun 28, 2022

Choose a reason for hiding this comment

lbaldy Jun 28, 2022

Choose a reason for hiding this comment

facebook-github-bot commented Jun 28, 2022

facebook-github-bot commented Jun 28, 2022

react-native-bot commented Jun 28, 2022

lbaldy commented Jun 15, 2022 •

edited

Loading

analysis-bot commented Jun 15, 2022 •

edited

Loading

analysis-bot commented Jun 15, 2022 •

edited

Loading