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

iOS- 1.0.2-51- Left menu shown in the center and need to reload App to fix it #2180

Closed
mallenexpensify opened this issue Mar 31, 2021 · 69 comments · Fixed by #2799 or #3078
Closed

iOS- 1.0.2-51- Left menu shown in the center and need to reload App to fix it #2180

mallenexpensify opened this issue Mar 31, 2021 · 69 comments · Fixed by #2799 or #3078
Assignees
Labels
Reviewing Has a PR in review

Comments

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Mar 31, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Job on Upwork https://www.upwork.com/jobs/~01acd19801f956e8ed


Expected Result:

App should be open and show chat as usually.

Actual Result:

App is opened but left menu is shown in the middle. Need to toggle device orientation again to fix it.

Action Performed:

  1. Launch App
  2. Open any chat
  3. Switch orientation of iPad from landscape to portrait
  4. Switch orientation back to landscape
  5. Tap on home button on iPAD to put app into the background
  6. Open App from background

Platform:

iOS - iPad

Version Number: 1.0.2-51

Notes/Photos/Videos:
image

Expensify/Expensify Issue URL:
https://github.com/Expensify/Expensify/issues/157516

@Amazing1974
Copy link

can you share the source code? then i can fix it.

@mallenexpensify
Copy link
Contributor Author

mallenexpensify commented Mar 31, 2021

@mallenexpensify
Copy link
Contributor Author

@marcaaron assigned to you for incoming proposal reviews since you added the External label to the E/E issue

@marcaaron
Copy link
Contributor

Gonna unassign myself from this one as the process changed on that.
Adding the Exported label should auto-assign someone now.

@marcaaron marcaaron removed their assignment Apr 5, 2021
@MelvinBot
Copy link

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

@mallenexpensify
Copy link
Contributor Author

on hold to see if #2051 magically fixes this too.

@deetergp
Copy link
Contributor

🤞

@tugbadogan
Copy link
Contributor

I debugged this issue and found that react native's Dimensions module is behaving weirdly on older iPad devices. When the app is sent to background, onDimensionChange callback function is called with wrong window dimensions before it's called with the right dimensions.

While using iPad in portrait mode (width: 768, height: 1024), when I send the app to the background this function is first called with (width: 1024, height: 768) and then with (width: 768, height: 1024). Components are rendered based on the first call and it's causing elements to be positioned badly on the screen.

I'm still trying to find out why it behaves like this on older iPad versions while it's working as expected on iPhone and iPad Pro.

@marcaaron
Copy link
Contributor

Could be related to this ? facebook/react-native#29290

@tugbadogan
Copy link
Contributor

@marcaaron

Could be related to this ? facebook/react-native#29290

Yes, it looks similar. I couldn't find any workaround or temporary solution yet.

@deetergp
Copy link
Contributor

Any word on your findings, @tugbadogan?

@tugbadogan
Copy link
Contributor

Yes, I have a proposal for this issue now.

Since this problem is caused by Dimensions native class is calling 'change' callback too many times with wrong dimensions, we can use debounce to slow down listening these changes, so we can avoid breaking call sites by triggering too many window dimension updates.

Screenshot 2021-04-22 at 02 25 51

Making these changes resolve this issue.

@marcaaron
Copy link
Contributor

Thanks, but maybe there's more to it? I applied this and was able to break it quickly by just changing the orientation a few times.

@tugbadogan
Copy link
Contributor

Which device/emulator are you using to test this? I couldn't reproduce the issue after this fix even though I changed the orientation many times.

Screen.Recording.2021-04-22.at.18.41.09.480p.mov

@marcaaron
Copy link
Contributor

I was testing on on simulator iPad Pro (9.7-inch). Sorry, I think maybe I had an old bundle cached when adding in the suggested change? Tested again and it does seem to fix it. Probably, it's fine to make this change since debouncing this event is a fine idea anyway. But we are not quite addressing the root problem.

@tugbadogan
Copy link
Contributor

Hmm, I was able to reproduce broken view on orientation change on an iPhone (I couldn't find a reliable way to reproduce though). This looks like a separate issue then iPad having broken view when the app is sent to background. It looks like a bug on Drawer.Navigator.

@marcaaron
Copy link
Contributor

Hmm, I was able to reproduce broken view on orientation change on an iPhone

I'm confused as the current discussion is not about iPhone.

This looks like a separate issue then iPad having broken view when the app is sent to background. It looks like a bug on Drawer.Navigator.

Not too sure what you are referring to exactly, but if you have discovered another issue please create a new issue or add context to an existing one.

@mallenexpensify
Copy link
Contributor Author

I doubled price to $500 in Upwork https://www.upwork.com/jobs/~01e2db72cd3240af67

@mallenexpensify
Copy link
Contributor Author

The link above wasn't getting any 👀 so I closed it and created another with the $500 price.
https://www.upwork.com/jobs/~01acd19801f956e8ed

@ViniSchonardie
Copy link

ViniSchonardie commented May 4, 2021

Im having some difficulties to reproduce the error on Ipad Pro 9-7 emulator;
But a good option would be to transform the hook to functional component and use this react native hook
import { useWindowDimensions } from 'react-native';
https://reactnative.dev/docs/usewindowdimensions
Its more reliable;

From React Native Docs:

useWindowDimensions is the preferred API for React components. Unlike Dimensions, it updates as the window's dimensions update. This works nicely with the React paradigm.

@ViniSchonardie
Copy link

ViniSchonardie commented May 4, 2021

Also, i am watching the React Native changelog, and aparently they made some changes about this on 0.63.4, and the Expensify.cash its on 0.63.3...

https://github.com/react-native-community/releases/blob/master/CHANGELOG.md
Dimension update events are now properly sent following orientation change (0e9296b95d by @ajpaulingalls)
facebook/react-native@0e9296b

It says android versions, but its enough to give it a try

@marcaaron
Copy link
Contributor

Its more reliable;

What makes it more reliable?

Dimension update events are now properly sent following orientation change (0e9296b95d by @ajpaulingalls)

Thanks, but that commit looks like it changed Java code and we are experiencing an issue on iOS.

@mallenexpensify
Copy link
Contributor Author

@Jag96 Should I plan to pay @tugbadogan on June 15th for this?

@Jag96 Jag96 changed the title iOS- 1.0.2-51- Left menu shown in the center and need to reload App to fix it [HOLD for payment June 15th] iOS- 1.0.2-51- Left menu shown in the center and need to reload App to fix it Jun 10, 2021
@Jag96 Jag96 added the Reviewing Has a PR in review label Jun 10, 2021
@Jag96
Copy link
Contributor

Jag96 commented Jun 10, 2021

@mallenexpensify yes, updated the title 👍

@tugbadogan
Copy link
Contributor

@mallenexpensify It looks like Upwork approved the milestone and paid me automatically after some time passed.

@mallenexpensify
Copy link
Contributor Author

Hi @tugbadogan is this issue the 'exact' same as this one? #2447

The combo of these two has been confusing me for WEEKS

@tugbadogan
Copy link
Contributor

@mallenexpensify This issue and #2447 are both caused by some animation bugs in react-navigation library. These issues are separate symptoms of similar bugs in react-navigation.

  • In this issue, react navigation fails to update the position of the left menu when orientation is changed to landscape, so it's displayed in the center and UI breaks.
  • In iPad - Chat - Chats are not accessible on iPad in Portrait mode (Pay on June 15th) #2447, when device orientation is changed to portrait, react-navigation fails to move the left menu out of screen, left menu is displayed correctly on the screen, but chat buttons are not clickable.

By upgrading react-navigation to 6.x, we started using React Animated 2 which I expect to solve positioning issues, so both of these issues can be closed.

@parasharrajat
Copy link
Member

parasharrajat commented Jun 15, 2021

By upgrading react-navigation to 6.x, we started using React Animated 2

@tugbadogan I believe this perception is wrong here as per the docs https://reactnavigation.org/docs/6.x/drawer-navigator#uselegacyimplementation. And you have not set the flag explicitly.

@tugbadogan
Copy link
Contributor

The documentation says

This defaults to true in following cases:
* Reanimated 2 is not configured
* App is connected to Chrome debugger (Reanimated 2 cannot be used with Chrome debugger)
* App is running on Web

Otherwise, it defaults to false

That prop defaults to false when Reanimated 2 is configured. This issue and other issue come back when I set useLegacyImplementation prop to true.

@parasharrajat
Copy link
Member

parasharrajat commented Jun 15, 2021

I meant that we should set useLegacyImplementation as false explicitly as I think it says for any of those cases it's true. But its fine if issue is fixed.

@tugbadogan
Copy link
Contributor

I meant that we should set useLegacyImplementation as false explicitly as I think it says for any of those cases it's true. But its fine if issue is fixed.

Yeah, it'd be better to set it false to enable Reanimated 2 on web and have consistency. That's need to be tested thoroughly on web though.

@tugbadogan
Copy link
Contributor

I tried and it throws some exceptions, it may not be ready for react-native-web yet or we need to fix a couple of more things to enable it on web.

Screenshot 2021-06-16 at 00 32 27

@parasharrajat
Copy link
Member

@tugbadogan
I think Reanimated 2 is not configured on Android as well. I see a warning
image

@parasharrajat
Copy link
Member

@mallenexpensify mallenexpensify changed the title [HOLD for payment June 15th] iOS- 1.0.2-51- Left menu shown in the center and need to reload App to fix it iOS- 1.0.2-51- Left menu shown in the center and need to reload App to fix it Jun 23, 2021
@mallenexpensify
Copy link
Contributor Author

This should have been paid via Upwork

@tugbadogan
Copy link
Contributor

Hi @mallenexpensify,

Other issue (#2447) is closed now. I think we can close this issue as well.

@mallenexpensify
Copy link
Contributor Author

Let's do it @tugbadogan !!!

@dklymenk
Copy link
Contributor

Hello, I think the job posting for this issue is no longer relevant, right? https://www.upwork.com/jobs/~012e79714137f7faa6

@mallenexpensify
Copy link
Contributor Author

Closed, thanks @dklymenk . This issue was a nightmare for me, think it got double posted months ago.

AndrewGable added a commit that referenced this issue Oct 7, 2022
remove workaround introduced in #2180 after the proper fix from #2727
smrutiparida pushed a commit to autosave-app/App that referenced this issue Oct 13, 2022
remove workaround introduced in Expensify#2180 after the proper fix from Expensify#2727
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewing Has a PR in review
Projects
None yet
Development

Successfully merging a pull request may close this issue.