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.53k stars 2.88k forks source link

[DISTANCE] [$1000] LOW: mWeb / Safari- i icon and mapbox logo not displayed #27800

Closed lanitochka17 closed 10 months ago

lanitochka17 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!


Issue found when executing PR https://github.com/Expensify/App/pull/27491

Action Performed:

  1. Launch the app on mWeb safari
  2. Open the global create menu and select Request money
  3. Select the Distance tab if it is not already active

Expected Result:

Toggle attribution (i) icon at the bottom-right of the map and Mapbox logo at the bottom-left of the map displayed to select

Actual Result:

Icon and mapbox icon not displayed. (No issue in chrome mWeb)

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.71-7

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Bug6206495_IMG_7377

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015fbbf81efc9a06b4
  • Upwork Job ID: 1706697994798551040
  • Last Price Increase: 2023-10-19
  • Automatic offers:
    • abdulrahuman5196 | Reviewer | 27464745
    • aswin-s | Contributor | 27464746
Issue OwnerCurrent Issue Owner: @abdulrahuman5196
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

johncschuster commented 1 year ago

@lanitochka17 for those of us less familiar with this new feature, can you please provide a video example of the incorrect behavior, as well as a video example of the correct behavior? I'd like to see the difference between what you spotted on Safari vs Chrome.

Thanks!

m-natarajan commented 1 year ago

@johncschuster I am attaching the video for reference.

https://github.com/Expensify/App/assets/38435837/507fc7c7-e9e8-4ede-a5e7-1cbc0b42ad53

akinwale commented 1 year ago

This seems to be caused by the Smart App banner pushing down the elements, which can only be seen on the staging or production URLs. Still trying to figure out why.

There's an open PR that fixes a bug that occurs when the map container resizes. I am not sure if that would fix this, but it may be worth a shot to check it out.

EDIT: The PR is in staging now, but no dice.

akinwale commented 1 year ago

After doing some more digging, it turns out that when the Smart App Banner is displayed, it somehow messes up the z-indexes of the map and its controls, since it triggers a re-render of the map canvas, and the map ends up being rendered over the Mapbox logo and the toggle attribution icon, resulting in the controls being hidden.

Setting a z-index of -1 for the .mapboxgl-canvas CSS class seems to solve the rendering problem. However, this prevents the map from being interacted with (tap / touch / pan / zoom operations stop working).

melvin-bot[bot] commented 1 year ago

@johncschuster, @lanitochka17 Huh... This is 4 days overdue. Who can take care of this?

johncschuster commented 1 year ago

Thanks for posting that video, @m-natarajan! That was very helpful!

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~015fbbf81efc9a06b4

melvin-bot[bot] commented 1 year ago

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

johncschuster commented 1 year ago

@abdulrahuman5196 just a heads up, @akinwale has already shared what they believe is the cause in this comment.

kameshwarnayak commented 1 year ago

Unable to reproduce in the current main.

simulator_screenshot_1F037C7D-9F48-4D75-A0B9-DFCDA22098B1

johncschuster commented 1 year ago

@lanitochka17 are you able to reproduce this on main?

lanitochka17 commented 1 year ago

Issue is reproducible on Build 1.3.75-3

IMG_7523

abdulrahuman5196 commented 1 year ago

@johncschuster Not sure of the issue status here. Do let me know when this issue comes into proposal review phase. Would be happy to help review on proposals or any other help required

kameshwarnayak commented 1 year ago

@lanitochka17 Can you check on main. Cant reproduce. It might have been already fixed.

melvin-bot[bot] commented 1 year ago

@johncschuster @abdulrahuman5196 @lanitochka17 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!

melvin-bot[bot] 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? πŸ’Έ

melvin-bot[bot] commented 1 year ago

@johncschuster, @abdulrahuman5196, @lanitochka17 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

johncschuster commented 1 year ago

@lanitochka17 bump! Can you check on main to see if this is still reproducible?

lanitochka17 commented 1 year ago

@johncschuster Hello, issue is reproducible on Build 1.3.77-5

https://github.com/Expensify/App/assets/78819774/5751aad1-a4f8-412e-a988-8525d0f21f83

melvin-bot[bot] commented 1 year ago

@johncschuster @abdulrahuman5196 @lanitochka17 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 1 year ago

@johncschuster, @abdulrahuman5196, @lanitochka17 Eep! 4 days overdue now. Issues have feelings too...

abdulrahuman5196 commented 1 year ago

I think this issue is still waiting on @johncschuster Traige. Do let me know if anything is required from my end - https://github.com/Expensify/App/issues/27800#issuecomment-1742024216

melvin-bot[bot] 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? πŸ’Έ

mvtglobally commented 1 year ago

Issue not reproducible during KI retests. (First week)

neil-marcellini commented 1 year ago

That's not what I see. The issue is still reproducible. We should see the mapbox logo and info icon on the map here.

neil-marcellini commented 1 year ago

@mvtglobally please check your test steps.

b4s36t4 commented 1 year ago

Is this only happening on physical devices? I tried on simulator not able to repro.

neil-marcellini commented 1 year ago

Interesting, I guess so.

johncschuster commented 1 year ago

@abdulrahuman5196

I think this issue is still waiting on @johncschuster Traige. Do let me know if anything is required from my end - https://github.com/Expensify/App/issues/27800#issuecomment-1742024216

Thanks for checking! This issue has already been triaged. We're just waiting on proposals.

melvin-bot[bot] commented 1 year ago

@johncschuster, @abdulrahuman5196, @lanitochka17 Whoops! This issue is 2 days overdue. Let's get this updated quick!

abdulrahuman5196 commented 1 year ago

@johncschuster There hasn't been any proposals yet. Can we re-test the issue and raise the bounty?

melvin-bot[bot] commented 1 year ago

@johncschuster @abdulrahuman5196 @lanitochka17 this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 1 year ago

Current assignee @abdulrahuman5196 is eligible for the Internal assigner, not assigning anyone new.

mvtglobally commented 1 year ago

Issue not reproducible during KI retests. (Second week)

melvin-bot[bot] commented 1 year ago

Upwork job price has been updated to $1000

graylewis commented 1 year ago

@johncschuster @mvtglobally

It seems like this issue can't be reproduced on an iPhone simulator. Could someone with a physical iPhone try to reproduce this on their phone and on a simulator and see if it's different?

johncschuster commented 1 year ago

@neil-marcellini has already reproduced it, here

abdulrahuman5196 commented 1 year ago

Yup. I was also able to reproduce this issue. I don't think this is fixed. We are still waiting on proposals.

melvin-bot[bot] commented 1 year ago

πŸ“£ @aswin-s-kumar! πŸ“£ Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
melvin-bot[bot] commented 1 year ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

melvin-bot[bot] commented 1 year ago

⚠️ Invalid email. Please make sure to create an Expensify account with this email first here.

aswin-s commented 1 year ago

Proposal

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

MapBox attribution controls are not visible on iOS Safari

What is the root cause of that problem?

The problem arises due to the SmartApp banner in PROD, which pushes content down and reduces the screen height. This forces the scroll view to render distance request components within a scrollable container, leading to the Canvas element (responsible for rendering the map view) being painted above the attribution controls, thereby hiding them. This issue is caused by a Webkit bug that affects the stacking order of the canvas element when it's rendered inside a scrolling parent container.

This issue occurs on small-screen devices or when there is smart banner at the top which forces the content to scroll inside the scroll view. This can be replicated on DEV and simulators by adding an additional view of 50px in height above the screen contents. It's important to note that this problem is specific to Mac/iOS Safari, which uses the Webkit rendering engine. Chrome on desktop and Android uses the Blynk rendering engine and does not experience this issue.

https://github.com/Expensify/App/assets/145648771/07d98f63-1004-4d36-a706-d68dcda3f6f1

Let me explain this behaviour further.

Here's how the components are organised in the React component tree. Less important components have been omitted.

MoneyRequestSelectorPage
β”œβ”€β”€ HeaderWithBackButton
└── OnyxTabNavigator
    └── NewDistanceRequestPage
        └── DistanceRequest
            └── DraggableList (ScrollView)
                β”œβ”€β”€ DistanceRequestRenderItem
                └── DistanceRequestFooter
                    └── DistanceMapView
                        └── MapView (Uses MapBoxGL component)

The entire Distance Request controls are rendered within a ScrollView component to ensure accessibility on small-screen devices. The map is rendered using the mapbox-gl library, and this is how it gets rendered in the DOM:

scroll-view (Parent scrollable div element with overflow-y auto)       
└── mapbox-map
    β”œβ”€β”€ mapboxgl-canvas-container
    β”‚   └── mapboxgl-canvas (canvas element, relatively positioned)
    └── mapboxgl-control-container (attribution controls)
        β”œβ”€β”€ mapboxgl-ctrl-top-left
        β”œβ”€β”€ mapboxgl-ctrl-top-right
        β”œβ”€β”€ mapboxgl-ctrl-bottom-left (logo, absolutely positioned)
        └── mapboxgl-ctrl-bottom-right (info icon, absolutely positioned)

Based on the DOM structure above, the absolutely positioned attribution controls should be rendered above the canvas element. However, when the screen height is insufficient, the parent container with overflow-y:auto becomes a scrollable container. This triggers a rendering bug in Webkit browsers that causes the relatively positioned canvas element to be painted over the absolutely positioned controls. This can be verified in PROD by reducing the min-height of mapViewContainer from 300px to 250px. This reverts the scroll container back to normal, and the controls become visible again.

https://github.com/Expensify/App/assets/145648771/e224a1e3-99eb-4fae-b6cd-c54c6909e373

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

As the issue is related to nodes directly under the MapBox container, we cannot apply styles to the component nodes or change the stacking order. There are no props for the Map component that allow us to inject custom CSS. To target the nodes, we need to explicitly reference them by their class name and apply a CSS fix to the Web platform.

To address the stacking order bug related to absolutely positioned elements within a scrolling parent, we can resolve it by compelling the browser to paint the layer separately using a CSS transform. The transform style prompts the browser to repaint the controls above the canvas element.

  1. Add a css file with the transform style applied to attribution controls
/* attribution-fix.css */
.mapboxgl-ctrl-bottom-left {
  -webkit-transform: translate3d(0, 0, 1px);
  transform: translate3d(0, 0, 1px);
}

.mapboxgl-ctrl-bottom-right {
  -webkit-transform: translate3d(0, 0, 1px);
  transform: translate3d(0, 0, 1px);
}
  1. Import the css in MapView.web.tsx

https://github.com/Expensify/App/blob/3656ae16d2105920659c9105cb865ebe9d52a04d/src/components/MapView/MapView.web.tsx#L20

import Direction from './Direction';
import {MapViewHandle, MapViewProps} from './MapViewTypes';

import 'mapbox-gl/dist/mapbox-gl.css';

// Add this line
import './attribution-fix.css';

Here is how it looks after the fix

https://github.com/Expensify/App/assets/145648771/ca77510a-33fc-4e55-bccb-521d6cdee20a

What alternative solutions did you explore? (Optional)

None

aswin-s-kumar commented 1 year ago

Sorry guys with the mixup above with my contributor account. I was trying to use an agency upwork profile link. Was not sure how it works. Sorry for the spam emails πŸ‘Ό

abdulrahuman5196 commented 1 year ago

We got a proposal to review in the weekend. Need to investigate the proposal much further.

Before that,

This issue occurs only in PROD and on small-screen devices like the iPhone SE.

@aswin-s-kumar / @aswin-s (not sure which is correct GH). I was able to repro this issue in iphone 14 simulator on adhoc builds generated.

aswin-s commented 1 year ago

@abdulrahuman5196 You should be able to replicate this even on Dev on small screen devices. The only precondition is that the vertical screen height should be less to cause a scroll in content region. I could reproduce this even on Mac Safari with window resized to phone resolution.

johncschuster commented 1 year ago

@abdulrahuman5196 did you see the above comment?

abdulrahuman5196 commented 1 year ago

@aswin-s Sorry missed the reply. I meant a different question.

This issue occurs only in PROD and on small-screen devices like the iPhone SE

I was pointing out that this is wrong actually. I am able to reproduce in iphone 14 size of screens as well. Let me know if i missed something.

aswin-s commented 1 year ago

@abdulrahuman5196 Ok. I'll remove above line to avoid confusion. Did you get a chance to look at evaluating the root cause and proposal?