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

deetergp commented 3 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

We have an issue where opening the app in native iOS on an iPad in Landscape orientation, the Chat Selector component is floating in the middle of the screen. We opened this issue to deal with it and had a workaround put forward by one of our contributors, but in the process of investigating, discovered that the real issue is with React Native core. They have an open issue and are working on a fix, but it's anyone's guess when it will be ready.

Let's keep an eye on the React Native team's solution and see if we can put it into place and undo the the temporary workaround we are using for now.

cc @marcaaron @mallenexpensify

Workaround:

https://github.com/Expensify/Expensify.cash/issues/2180#issuecomment-833695795

Original React Native Issue

https://github.com/facebook/react-native/issues/29290

Platform:

Where is this issue occurring?

[ ] Web
[x] iOS (iPad)
[ ] Android
[ ] Desktop App
[ ] Mobile Web

Version Number: 1.0.2-51 Notes/Photos/Videos: See the original issue

View all open jobs on Upwork

MelvinBot commented 2 years ago

This issue has not been updated in over 15 days. eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

mallenexpensify commented 2 years ago

Removed the monthly label so it won't auto-close. Also dropped a (likely fruitless) post in #expensify-open-source to see if anyone has ideas. https://expensify.slack.com/archives/C01GTK53T8Q/p1633992309160300

puneetlath commented 2 years ago

@deetergp do we know that they are actively working on a fix? If not, perhaps we can hire a contributor to submit a PR to fix it to RN directly now.

deetergp commented 2 years ago

@deetergp do we know that they are actively working on a fix? If not, perhaps we can hire a contributor to submit a PR to fix it to RN directly now.

Your guess is as good as mine… The last comment on that issue was from a year ago. There are references from other issues and PRs as recently as the end of October '21 so it is definitely still in people's minds. 🤷

MelvinBot commented 2 years ago

@deetergp, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

puneetlath commented 2 years ago

I still think we should hire a contributor to fix this PR directly in RN https://github.com/facebook/react-native/issues/29290. I'm going to add the external label for it.

MelvinBot commented 2 years ago

Triggered auto assignment to @NicMendonca (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

NicMendonca commented 2 years ago

Upwork job: https://www.upwork.com/jobs/~017b165dfa9821e51c

MelvinBot commented 2 years ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

MelvinBot commented 2 years ago

Triggered auto assignment to @marcaaron (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

marcaaron commented 2 years ago

Should we just quadruple this one now? Fixing this upstream is worth a bit more than $250.

puneetlath commented 2 years ago

Agreed.

NicMendonca commented 2 years ago

quadrupled the price -- https://www.upwork.com/jobs/~017b165dfa9821e51c

marcaaron commented 2 years ago

Waiting for proposals

marcaaron commented 2 years ago

Still waiting

NicMendonca commented 2 years ago

doubled: https://www.upwork.com/jobs/~017b165dfa9821e51c

NicMendonca commented 2 years ago

Doubled today ☝️

marcaaron commented 2 years ago

Still waiting for proposals

mallenexpensify commented 2 years ago

Doubled price to $4000 https://www.upwork.com/jobs/~017b165dfa9821e51c

marcaaron commented 2 years ago

Still waiting for proposals here.

mallenexpensify commented 2 years ago

Doubled price to $8000 https://www.upwork.com/jobs/~017b165dfa9821e51c

aneequeahmad commented 2 years ago

PROPOSAL

As the problem is already highlighted in the react-native community as this is a react-native bug.

Solution 01

using the useSafeAreaFrame() hook from react-native-safe-area-context. It works really well as a replacement if we make sure to only have one SafeAreaProvider at the top level of the app(which we already have). Many libraries like react-native-collapsible-tab-view have done this to address this useWIndowDimensions() issue.

Solution 02

Another way to achieve what we want would be to allow passing the width as an optional prop while still defaulting it internally to what useWindowDimensions returns. Pass our own width which you can get from useSafeAreaFrame. Additionally, it will also allow the library to work properly If you could look into this alternate implementation.

In my opinion, both of these solutions are bulletproof and as is not a workaround but a proper alternate solution.

Additional information:

Here is the merged commit https://github.com/PedroBern/react-native-collapsible-tab-view/commit/95bbce29e3af2d7657ee38b3ed5b67b80d96cfc5

cc: @marcaaron

marcaaron commented 2 years ago

works really well as a replacement

Sorry not looking for replacements (we already have a workaround deployed). Fix in react-native repro is a requirement for this issue.

NicMendonca commented 2 years ago

doubled: https://www.upwork.com/jobs/~017b165dfa9821e51c

lbaldy commented 2 years ago

PROPOSAL This has to be fixed inside the react native repository, through a standard MR process for Facebook (out of our control in terms of timing). Once merged, it would also be required to upgrade Expensify's react native version (this should probably be handled via another ticket that is well planned etc.).

Solution 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), 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.

IMHO solution should be applied to react native core modules, preventing it from publishing this event in case of app state change.

Please let me know if the proposal is fine from your perspective, and I will work on a react native MR.

I can provide a quickfix to react native code if you would like to test if it really fixes the issue.

Obviously before all the tests are going to happen the workaround implemented for this has to be removed.

AndrewGable commented 2 years ago

Have we confirmed this happens on a "basic" react native app to rule out any UI logic on our side? I know that it seems like they were "flipped", but it seems possible that the values were just "delayed" in reporting the real value?

lbaldy commented 2 years ago

@AndrewGable Please find my video showing the issue, and reproduction using the react native basic application.

https://youtu.be/nFDOml9M8w4

Here is a vide of my inline fix showing how it impacted the Expensify App:

https://www.youtube.com/watch?v=NMXtkll2sKM

For obvious reasons I blurred the XCODE part of the video.

AndrewGable commented 2 years ago

Thank you for the very informative video @lbaldy! This looks very promising for sure. @parasharrajat - Can you review the videos and verify this is the expected behavior then work with @lbaldy to flush out the proposal?

azimgd commented 2 years ago

I observed a bug with useWindowDimensions happening due to following observers returning swapped values:

UIApplicationDidBecomeActiveNotification posted when the app becomes active.
UIApplicationDidChangeStatusBarOrientationNotification Posted when the orientation of the app’s user interface changes.

Both events first return old, then new dimensions when app goes to background and then foreground after orientation is changed.

We should handle following scenarios properly when the app is in foreground, only allow to emit events when app is foreground will not work in split screen mode:

Rotating device should emit dimensions based with respect to orientation [implemented in RN]
Only unique dimensions should be emitted [implemented in RN]
App going background and then foreground should only emit foreground dimensions [bug in RN, to be fixed]
Split screen mode should emit correct dimensions [buggy, to be fixed]
Split screen resize should emit correct dimensions [implemented in RN]

Proposal is to store application's background/foreground state prior to orientation change observer and add extra checks which should prevent unnecessary event emission.

- (void)_interfaceFrameDidChange
{
  NSDictionary *nextInterfaceDimensions = RCTExportedDimensions(_moduleRegistry);
  UIApplicationState nextAppState = [RCTSharedApplication() applicationState];

  // See if app is in split-screen or a fullscreen mode
  BOOL isAppFullscreen = CGRectEqualToRect(
   [RCTSharedApplication() delegate].window.screen.bounds,
   [RCTSharedApplication() delegate].window.frame
  );

  // is app in background or foreground
  BOOL isAppActive = (
    (UIApplicationStateBackground == _currentAppState && UIApplicationStateActive == nextAppState) ||
    (UIApplicationStateActive == _currentAppState && UIApplicationStateActive == nextAppState)
  );

  // add extra check
  if (!([nextInterfaceDimensions isEqual:_currentInterfaceDimensions]) && (isAppActive || !isAppFullscreen)) {

Similar, but reduced check should be added into:

- (void)_interfaceOrientationDidChange

Same pattern is already applied to track prev / next values for orientation changes, see UIInterfaceOrientation.

Here is the draft PR fixing above on iOS https://github.com/Expensify/react-native/pull/9

Manual tests performed:

Open an app on iPad and watch for useWindowDimensions result
Rotating orientation should return actual dimensions
Rotating orientation in landscape should not first emit dimensions for portrait and then for landscape
Split screen should return actual dimensions
Resizing split screen should return actual dimensions

Issue wasn't reproducible on Android tablet and phone. Event emitters work correctly.

parasharrajat commented 2 years ago

Oh, I didn't notice that I am assigned to it. I will check them tomorrow morning IST.

lbaldy commented 2 years ago

@azimgd that sounds exactly like what I presented in my proposal and a follow up video to @AndrewGable. No? ;)

parasharrajat commented 2 years ago

Looking at the video @lbaldy I can say that it looks convincing. The next step would be to share a technical proposal here to explain the problem and lay out your fix. Your previous solution does not really talk about the solution but what is happening.

Also, the solution should be tested on the E/app without the workaround before being proposed here.

And, yes it is necessary to explain your technical changes here in the proposal.

azimgd commented 2 years ago

@marcaaron @parasharrajat could you have a look at the proposal above please ?

lbaldy commented 2 years ago

@parasharrajat totally makes sense, please find my proposal below. If anything is missing let me know.

Problem statement:

The orientation change notification is published by IOS when app goes to the background, due to some sort of a race condition/timing issue causes wrong dimensions being returned and transformation being queued in react native.

Solution description and assumptions:

We need to prevent publishing the event when app transforms from foreground to background, but at the same time we have to ensure:

App reacts to orientation changes when in foreground.
App reacts to orientation changes that happened when the app was in background.
App reacts to view changes when in multitasking (split) view.
App reacts to view changes when moving from slide over view to fullscreen or split.
App doesn't re-trigger the animation of the side bar when app simply goes to background and comes back to foreground in the same orientation.

Technical solution:

The fix has to be applied directly to react-native core module. The whole code responsible for this behavior lives in RCTDeviceInfo.mm.

There are a couple of things that have to be modified.

a) The body of the _interfaceOrientationDidChange:

Here we make sure we run this function when the app is in active and also it was switching to fullscreeen or is not running in fullscreen.

- (void)_interfaceOrientationDidChange
{
  UIInterfaceOrientation nextOrientation = [RCTSharedApplication() statusBarOrientation];
  UIApplicationState appState = [RCTSharedApplication() applicationState];
  BOOL isRunningInFullScreen = CGRectEqualToRect([UIApplication sharedApplication].delegate.window.frame, [UIApplication sharedApplication].delegate.window.screen.bounds);

  // Update when we go from portrait to landscape, or landscape to portrait
  if ((((UIInterfaceOrientationIsPortrait(_currentInterfaceOrientation) &&
       !UIInterfaceOrientationIsPortrait(nextOrientation)) ||
      (UIInterfaceOrientationIsLandscape(_currentInterfaceOrientation) &&
       !UIInterfaceOrientationIsLandscape(nextOrientation)) || (isRunningInFullScreen != _isFullscreen || !isRunningInFullScreen)))
      && appState == UIApplicationStateActive) {
      #pragma clang diagnostic push
      #pragma clang diagnostic ignored "-Wdeprecated-declarations"
          [[_moduleRegistry moduleForName:"EventDispatcher"] sendDeviceEventWithName:@"didUpdateDimensions"
                                                                                body:RCTExportedDimensions(_moduleRegistry)];
            _currentInterfaceOrientation = nextOrientation;
            _isFullscreen = isRunningInFullScreen;
      #pragma clang diagnostic pop
  }

}

b) _interfaceFrameDidChange

Here we make sure we run it when the app is in active state.

- (void)_interfaceFrameDidChange
{
  NSDictionary *nextInterfaceDimensions = RCTExportedDimensions(_moduleRegistry);
    UIApplicationState appState = [RCTSharedApplication() applicationState];

  if (!([nextInterfaceDimensions isEqual:_currentInterfaceDimensions]) && appState == UIApplicationStateActive) {
    #pragma clang diagnostic push
    #pragma clang diagnostic ignored "-Wdeprecated-declarations"
        [[_moduleRegistry moduleForName:"EventDispatcher"] sendDeviceEventWithName:@"didUpdateDimensions"
                                                                              body:nextInterfaceDimensions];
          _currentInterfaceDimensions = nextInterfaceDimensions;
    #pragma clang diagnostic pop
  }
}

c) Add the _isFullscreen variable to hold the previous state for multitasking view.

@implementation RCTDeviceInfo {
  UIInterfaceOrientation _currentInterfaceOrientation;
  NSDictionary *_currentInterfaceDimensions;
  BOOL _isFullscreen;
}

d) We need to make sure to run the interfaceOrientationDidChange instead of interfaceFrameDidChange when the UIApplicationDidBecomeActiveNotification occurs.

  [[NSNotificationCenter defaultCenter] addObserver:self
                                           selector:@selector(interfaceOrientationDidChange)
                                               name:UIApplicationDidBecomeActiveNotification
                                             object:nil];

Solution provided by azimgd doesn't work when the app changes the orientation while in background.

Please find my PR below: https://github.com/Expensify/react-native/pull/10

parasharrajat commented 2 years ago

App reacts to orientation changes that happened when the app was in the background.

How can an app change orientation while in the background? Could you please explain me? @lbaldy

@azimgd Why are we removing the _bridge parameter from function definitions?

lbaldy commented 2 years ago

@parasharrajat in a real world device (or emulator). You go to landscape. Move the app to background. Rotate the device back to portrait (while the app is in background), open the app - the app should transform itself from landscape (how we deactivated it) to portrait (which is the current orientation). While doing that it should preserve a correct behaviour of the side bar - hide or show it depending on the logic implemented in the JS.

azimgd commented 2 years ago

This commit affected the diff: https://github.com/facebook/react-native/commit/e500e89fd60d521b0a4e68273ae67d02d63a53cf, looks like the sync was applied after I pushed my changes.

parasharrajat commented 2 years ago

@azimgd Got it. Why are you only triggering the event when the app is active? https://github.com/Expensify/react-native/blob/f635dd7b74b9d5f2d3b71972bcc395eae780ed6d/React/CoreModules/RCTDeviceInfo.mm#L213

azimgd commented 2 years ago

When the app is not in split screen mode:

Trigger when the app is opened (goes from background → foreground)
Trigger when the device is rotated

We are filtering away wrong dimensions emits (e.g. goes from foreground → background) there. As when the app goes into background those are triggered in batch [wrong{x,y}, correct{x,y}].

Additionally !isAppFullscreen will:

trigger when the app is resized in the split screen mode

lbaldy commented 2 years ago

@parasharrajat I added a video testing all the cases as well as 'background orientation change' I mentioned earlier: https://youtu.be/Ytj0K4SwP4w please let me know if you have any questions related to my proposal.

parasharrajat commented 2 years ago

Thanks, I am testing something and soon update you all.

parasharrajat commented 2 years ago

Ok, based on my testing. There is no event fired when the app is in the background. background orientation change or when the app's orientation is changed while the app is in an inactive state, is detected when the app comes to the active state. As soon as the app comes to an active state, the app receives an orientation event with proper data.

That said, @lbaldy came up with the proposal first. We were mostly convinced by his explanation and videos. He was also thorough during the discussion. Although @azimgd's proposal is also correct it matches @lbaldy 's proposal. Given that @lbaldy is a new contributor and he mentioned hiding the code "For obvious reasons I blurred the XCODE part of the video.", I asked him to share technical details. I like @lbaldy 's proposal. @lbaldy Please consider sharing the technical details next time. It is a crucial part of the proposal review process.

It was very hard to pick one over another due to both proposals were great, I chose to go with the first proposal.

cc: @marcaaron

:ribbon: :eyes: :ribbon: C+ reviewed

azimgd commented 2 years ago

Based on the contributing.md:

We look for the earliest provided, best proposed solution that addresses the job.

That said:

previous proposals didn't have any technical explanation
missed a very important "Split Screen Mode", which is a key on why duplicate events happen
previous proposals mentioned a "one-liner fix" which was later transformed into very close copy of PR#9, see isFullScreen, event filtering

convinced by his explanation and videos For obvious reasons I blurred the XCODE part of the video It is a crucial part of the proposal review process.

contributing.md explicitly mentions: Your solution proposal should include a brief technical explanation of the changes you will make.

lbaldy commented 2 years ago

I think the solution proposed by @azimgd doesn't work or even breaks when the app when in example: switches from slide over mode to fullscreen. Thus this proposal isn't complete.

parasharrajat commented 2 years ago

Thanks to both of you for sharing your concerns. But my choice is never final. There would be someone from the Team to assign the job to one of you. We are currently discussing this internally.

azimgd commented 2 years ago

doesn't work or even breaks when the app when in example: switches from slide over mode to fullscreen.

Have you had a chance to test my PR? I'm pretty sure it handles that scenario correctly.

lbaldy commented 2 years ago

@parasharrajat I am going to wait for the final decision and assignment after the internal discussions. If any additional information regarding any of my comments or videos or code is needed please let me know.

marcaaron commented 2 years ago

Sorry guys, going OOO for a week and won't be able to help with this one. Sounds like it's moving in the right direction though!

melvin-bot[bot] commented 2 years ago

Current assignee @parasharrajat is eligible for the Exported assigner, not assigning anyone new.

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @iwiznia (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

Expensify / App