Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.48k stars 2.83k forks source link

[HOLD App/issues/16660] [$2000] Pinch to zoom on a Macbook browser dynamically changes the app's view and components #17246

Closed kavimuru closed 11 months ago

kavimuru commented 1 year 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!


Action Performed:

  1. When a chat is open, pinch to zoom on either the LHN or the details tab. 1 (alternate). On a fresh sign in page, zoom in on any part of the site with pinch to zoom.

    Expected Result

    The page doesn't reorganize or shfit as you zoom in this way, this should be equivalent to magnification and shouldn't change the content ordering or structure of the app.

    Actual Result

    App re-renders into different various forms as you zoom in breaking your ability to zoom into

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.2.98-1 Reproducible in staging?: needs reproduction Reproducible in production?: needs reproduction If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/43996225/231004704-3ffa4eb5-bcb9-456f-83c7-555e63e266b5.mov

Expensify/Expensify Issue URL: Issue reported by: @johnmlee101 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681152993219619

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01caf96ff9d8613c10
  • Upwork Job ID: 1646608088499531776
  • Last Price Increase: 2023-05-02
MelvinBot commented 1 year ago

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

alexpensify commented 1 year ago

I'll try to test soon.

alexpensify commented 1 year ago

I tested here is my page before zoom

image

Here is my page when zooming

image

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01caf96ff9d8613c10

MelvinBot commented 1 year ago

Current assignee @alexpensify is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

Armedin commented 1 year ago

This issue seems to happen whenever you zoom in on a web browser. The dimensionsEventListener is registering zoom events as a window dimension change. For instance, if I zoom 3 times and print the window dimension:

Screenshot at Apr 14 00-26-02
fedirjh commented 1 year ago

@Armedin I kindly request that you follow our proposal template when submitting proposals. The template can be found here : PROPOSAL TEMPLATE.

alitoshmatov commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Pinch to zoom on a Macbook browser dynamically changes the app's view and components

What is the root cause of that problem?

This is caused by how react-native-web handling dimensions. https://github.com/Expensify/react-native-web/blob/bb2f765614628eeccdd9d2850cb287557a92b6c9/packages/react-native-web/src/exports/Dimensions/index.js#L63-L71 Above you can see it is setting width and height based on window.visualViewport which changes when you zoom.

What changes do you think we should make in order to solve the problem?

There is an option to make an upstream fix in react-native-web to set window height and width to documentElement.clientHeight which won't change when you zoom and feels more natural to set this measures to window object in dimensions.

What alternative solutions did you explore? (Optional)

alexpensify commented 1 year ago

@fedirjh - early this week, can you share feedback on this new proposal? Thanks!

fedirjh commented 1 year ago

Thank you for sharing your proposal, @alitoshmatov. I agree with your RCA. However, I'd like to point out that the use of viewport dimensions was intentionally added in this commit https://github.com/Expensify/react-native-web/commit/ccfd936f274ca2105745f9edbbb4aad80725e181 (PR #14392) to address this issue #11463. It seems that you are suggesting reverting these changes, but I don't think that would be ideal unless you also provide a solution for #11463.

alitoshmatov commented 1 year ago

@fedirjh Is it possible that we return one more field from dimensions from react native web, possibly separate viewport and window objects and use them respectively

fedirjh commented 1 year ago

@alitoshmatov Can you please elaborate more ? How would that fix the issue ?

tgolen commented 1 year ago

Thanks for the research into this issue. One thing that isn't clear to me is why this is the expected behavior. Can anyone add some details about that? Is it because that's what happens on normal websites when you pinch to zoom?

fedirjh commented 1 year ago

The portion of the viewport that is currently visible is called the visual viewport. This can be smaller than the layout viewport, such as when the user has pinched-zoomed. The layout viewport remains the same, but the visual viewport became smaller.

@tgolen The MDN documentation on viewport states that the layout viewport remains the same during a pinched zoom gesture, while the visual viewport becomes smaller. However, this commit Expensify/react-native-web@ccfd936 breaks this behavior.

The reason why zooming affects the visualViewport is because the visualViewport represents the visible portion of the webpage as seen by the user in the viewport. When the user pinch to zoom, the visible portion of the webpage changes, and so the visualViewport needs to be updated to reflect the new dimensions and position of the visible viewport.

This behavior is expected because it reflects the user's interaction with the webpage. When the user pinch to zoom on a webpage, they are changing the scale of the content to better suit their needs, such as making the text easier to read or viewing an image in more detail. They do not expect the page to change its layout when they pinch to zoom, as the position of the content may change or even disappear, resulting in a bad UX.

that's what happens on normal websites when you pinch to zoom?

That's correct, even oldDot has this behavior where the website's layout is maintained when you pinch to zoom.

https://user-images.githubusercontent.com/36869046/232587056-7aeaba5c-1445-4f56-a674-807358c7e235.mov

tgolen commented 1 year ago

Great, thank you! That is an excellent explanation. Sounds like the next step for this is still waiting on an answer for https://github.com/Expensify/App/issues/17246#issuecomment-1510892091

alitoshmatov commented 1 year ago

@fedirjh I am suggestion to pass visualViewport width and height seperately along window, and screen values: https://github.com/Expensify/react-native-web/blob/ccfd936f274ca2105745f9edbbb4aad80725e181/packages/react-native-web/src/exports/Dimensions/index.js#L63-L86

And then we can modify here to use visualViewport width: https://github.com/rawalyogendra/App/blob/6a1ae162886703a772d27081eff4c99a49dbf971/src/components/ScreenWrapper/index.js#L92

But there is a concern which this solution might break philosophy of platform independent code. Since this code visualViewport will only be available in web platforms through react-native-web.

fedirjh commented 1 year ago

@alitoshmatov I see your point. If I understand correctly, you're referring to the dimensions of the documentElement, because we're currently passing the visualViewport dimensions as the window dimensions.

I am suggestion to pass visualViewport width and height seperately along window, and screen values:

What if we pass the height of the visualViewport and the width of the documentElement ? Would that address both issues ?

alitoshmatov commented 1 year ago

@fedirjh Yes in previous comment i meant window=documentElement, screen, and visualViewport 3 seperate fields.

What if we pass the height of the visualViewport and the width of the documentElement ? Would that address both issues ?

Well it seems like there are some components which is styled based on window.height and they are adopting to smaller screen when zoomed, which is not desirable.

fedirjh commented 1 year ago

Yes in previous comment i meant window=documentElement, screen, and visualViewport 3 seperate fields.

@alitoshmatov Thank you for the update. However, I believe this solution would contradict the philosophy of platform-independent code. In my opinion, any solution that deviates from reverting this commit https://github.com/Expensify/react-native-web/commit/ccfd936f274ca2105745f9edbbb4aad80725e181 would be a workaround.

alitoshmatov commented 1 year ago

You are right, but I think https://github.com/Expensify/react-native-web/commit/ccfd936f274ca2105745f9edbbb4aad80725e181 itself is a workaround solution in expense of this issue. We should really reconsider this solution and https://github.com/Expensify/App/pull/14392 .

tgolen commented 1 year ago

I agree with you both. For this issue, let's make sure we revert the previous workaround and have a solution which fixes both problems 👍

MelvinBot commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

alexpensify commented 1 year ago

@fedirjh - are we good with the proposal now and can we carry on to the next steps? Thanks for the update.

fedirjh commented 1 year ago

@alexpensify We are still waiting for proposal that fixes both problems, we agreed that we should revert the previous workaround and have a solution which fixes https://github.com/Expensify/App/pull/14392

alexpensify commented 1 year ago

Whoops, I jumped the start line here. Thank you for clarifying!

MelvinBot commented 1 year ago

@tgolen @alexpensify @fedirjh this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

tgolen commented 1 year ago

Still waiting for proposals

tgolen commented 1 year ago

No new proposals yet

tgolen commented 1 year ago

Still no new proposals.

tgolen commented 1 year ago

Let's just let the price double on this next week and hopefully we'll start getting some proposals then.

MelvinBot commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

tgolen commented 1 year ago

Funny you should mention that, Melvin. Looks like great minds think alike and I beat you to the punch.

tgolen commented 1 year ago

Another day another similar update. Have a good weekend! 👋

alexpensify commented 1 year ago

Sigh, Melvin is still going to bark at us over the weekend.

tgolen commented 1 year ago

Cool, is the price increasing on this one today?

fedirjh commented 1 year ago

@tgolen I guess so, this issue is already 3 weeks old.

tgolen commented 1 year ago

@alexpensify any idea why the price hasn't increased for this?

alexpensify commented 1 year ago

No, I'm not sure why.

fedirjh commented 1 year ago

I think price should be updated manually

MelvinBot commented 1 year ago

Upwork job price has been updated to $2000

alexpensify commented 1 year ago

The price has been updated and we are waiting for proposals here.

tgolen commented 1 year ago

Waiting for proposals

tgolen commented 1 year ago

Still waiting

alitoshmatov commented 1 year ago

@fedirjh I think I have found potential solution: So it looks like we have scale field in visualViewport and it represents pinch zoom.

I think we should multiply it with height and width here: https://github.com/Expensify/react-native-web/blob/ccfd936f274ca2105745f9edbbb4aad80725e181/packages/react-native-web/src/exports/Dimensions/index.js#L65-L66

And It results in correct and same window measures, even if we are pinch zoomed

alitoshmatov commented 1 year ago

It is working as expected:

https://user-images.githubusercontent.com/59907218/236642314-cd540f28-9147-45e4-a292-ff900b3a15bb.mov

fedirjh commented 1 year ago

@alitoshmatov That make sense to me, the changes will be in our App right ?

alitoshmatov commented 1 year ago

@fedirjh No, It will be in react-native-web.