ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

Android : Styling issue in amp-sidebar #22860

Closed josselinDecathlon closed 5 years ago

josselinDecathlon commented 5 years ago

What's the issue?

When going to an our amp page with an Android phone. When opening the menu, the element with the close button (X) get 32px added on the cdn version.

How do we reproduce the issue?

Search "decathlon velo" from an Android device or using the chrome dev tool emulation of Android devices.

Load the page in the amp viewer by clicking the amp result on search, open the menu, you will notice some space above the cross button. Scrolling down on the amp-sidebar will make the close button overlay over the text with a gap at the top. This does not happen on the amp hosted version, only in the cdn and amp-viewer version.

What browsers are affected?

Browsers on Android.

Which AMP version is affected?

The current version.

Video of the bug available at this link. https://drive.google.com/open?id=1wDdfbjh-XaAbfPoJNZLo_9bhhP1iI-pD

Also try this cdn url : https://www-decathlon-fr.cdn.ampproject.org/v/s/www.decathlon.fr/fr/amp?categoryUri=/C-819830-velos-ville-classiques&usqp=mq331AQCKAE%3D&amp_js_v=0.1#origin=https://www.google.com&cid=1&prerenderSize=1&visibilityState=visible&paddingTop=32&history=1&p2r=0&horizontalScrolling=0&csi=1&storage=1&viewerUrl=https://www.google.com/amp/s/www.decathlon.fr/fr/amp%253FcategoryUri%3D/C-819830-velos-ville-classiques&cap=navigateTo,fragment,handshakepoll,cid,replaceUrl,fullReplaceHistory

And amp hosted url (without the bug) : https://www.decathlon.fr/fr/amp?categoryUri=/C-819830

aghassemi commented 5 years ago

to @sparhami to investigate.

sparhami commented 5 years ago

This looks to be a fixed layer bug. The sticky header is getting the top overwritten with calc(32px), coming from somewhere in this code block:

https://github.com/ampproject/amphtml/blob/364df8b91f621e9ec2bd696914451c07aa887ea5/src/service/fixed-layer.js#L779-L801

I verified this with a breakpoint.

jridgewell commented 5 years ago

Correct. This is because there's a sticky element inside fixed element. The top offsetting that we're doing is to account for the Viewer's header bar (the decathlon.fr on the top). We apply that to all fixed/sticky-positioned elements.

But, since this X button is sticky inside a fixed, we apply that offsetting twice.

sparhami commented 5 years ago

Should the contents of a sidebar be treated as if they were lightboxed to avoid this behavior?

jridgewell commented 5 years ago

I'll add an exception to FixedLayer that ignores children of already tracked elements.

jridgewell commented 5 years ago

I'll add an exception to FixedLayer that ignores children of already tracked elements.

Nope, can't do that, because we have to track lightboxes.