AMP fixed header on Chrome iOS has incorrect position when in an iFrame

SGudbrandsson commented 7 years ago

Short description of the issue

When viewing Google AMP results on Chrome iOS, there is a bug where the header disappears when I click on an AMP result with a header that has position:fixed; top:0; CSS.

How do we reproduce the issue?

Create an AMP page with a header with position:fixed; top:0; CSS.
Get the result indexed
Search for the page on Chrome in iOS
Make sure the location bar is not displayed (only scroll down)
Click on the search result
You'll see that the header is "calculated" to be below the google bar, but is hidden by the height of the location bar.

Example:

Example of how it should look:

iphone-amp-correct

Public URL (works fine if you access it directly):

https://guidetoiceland.is/connect-with-locals/regina/the-magical-snaefellsnes-peninsula/amp

Search result that will cause the issue:

https://www.google.is/search?sourceid=chrome-psyapi2&ion=1&espv=2&ie=UTF-8&q=sn%C3%A6fellsnes&oq=sn%C3%A6&aqs=chrome.0.69i59j69i61j69i57j0j69i61l2.1266j0j7

Here is a screen recording of where I reproduce the bug:

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

I'm not sure if this is a bug with the AMP recalc JS or Chrome on iOS, so I created a bug report on crbug.com as well:

https://bugs.chromium.org/p/chromium/issues/detail?id=687317

What browsers are affected?

Chrome 55 on iOS.

Which AMP version is affected?

1485231782273

jridgewell commented 7 years ago

/to @muxin /cc @RJSumi

muxin commented 7 years ago

This should be a amp viewer bug. Filed b/34890671

adelinamart commented 7 years ago

Closing this one for one and will follow up internally. Thanks.

SGudbrandsson commented 7 years ago

@adelinamart how can I follow the progress on this? Is there an issue that I can follow?

adelinamart commented 7 years ago

Hi @sigginet , unfortunately as for all other internal bugs, there's no good way to track the progress. Will be asking the Viewer Team to provide updates, and when we get them will try to leave updates on this closed issues. Hope that works for you. Thanks.

SGudbrandsson commented 7 years ago

Thanks @adelina

On Thu, Feb 9, 2017 at 6:37 PM, Adelina notifications@github.com wrote:

Hi @sigginet https://github.com/sigginet , unfortunately as for all other internal bugs, there's no good way to track the progress. Will be asking the Viewer Team to provide updates, and when we get them will try to leave updates on this closed issues. Hope that works for you. Thanks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ampproject/amphtml/issues/7283#issuecomment-278732084, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiLGo8_9OH9Brd_X6_AvJjp3CyJjw7Lks5ra1zPgaJpZM4Lzotq .

jrf0110 commented 7 years ago

I think this might actually be runtime-related. I'm seeing the same behavior on Cloudflare's AMPViewer on desktop chrome only if I set my User Agent to Chrome on iOS. It's also worth noting that Cloudflare's viewer uses a completely different technique for showing/hiding the viewer header (it doesn't rely on position: fixed + communicating a paddingTop to the runtime). See the following two videos:

Position fixed broken Position fixed works

The video that shows the broken behavior is using the following user agent:

Mozilla/5.0 (iPhone; CPU iPhone OS 9_0_1 like Mac OS X) AppleWebKit/601.1 (KHTML, like Gecko) CriOS/47.0.2526.107 Mobile/13A405 Safari/601.1.46 (000204)

jrf0110 commented 7 years ago

Though, I can't reproduce on Chrome desktop with Google Search's Viewer. That seems a little odd, but it would be even more odd if these things weren't related :)

adelinamart commented 7 years ago

@jrf0110 thanks so much for the extra troubleshooting. Some people in our team will look into this more closely.

dvoytenko commented 7 years ago

All! So we went for quite a loop trying to debug it. I wonder if anyone has any recommendation on how to debug Chrome iOS on devices?

jrf0110 commented 7 years ago

Only way I know is Browserstack + firebug:

<script src="https://getfirebug.com/firebug-lite.js#startOpened"></script>

Debugging Chrome on iOS is not fun :/

dvoytenko commented 7 years ago

@jrf0110 Thanks for the advise! I believe we identified the main culprit here and working on the fix. @muxin can add details.

jrf0110 commented 7 years ago

@dvoytenko @muxin any chance we could get some details on what's going on here? I'm wondering if there's anything I can do viewer side

dvoytenko commented 7 years ago

@jrf0110 From the initial analysis it looks like the content is auto-scrolled up by the amount of pixels that corresponds to the Chrome's address bar. It looks like a browser bug, but we haven't yet confirmed it with certainty.

dvoytenko commented 7 years ago

@muxin is this fixed now?

muxin commented 7 years ago

no, the position is sometimes correct, sometimes not.

dagurk commented 7 years ago

We at guidetoiceland.is have been trying to find a fix for this on our end but have been unsuccessful, perhaps some of our findings will help solve this

We originally thought this bug could be negated by using different CSS. So we tried many different methods to achieve our design, but the bug was always present.

We also tried finding a website that was not affected by this bug, and made some interesting discoveries.

We found no sites that where not affected by the bug
However, the rate of the bug appearing varies wildly, some sites only show the bug in 1 out of 20 page loads, others show it as much as 8 out of 10 times
After some analysing we found that it is most likely the content, not the CSS that causes this variance

Example page: amp.cnn.com/cnn/2017/05/30/politics/donald-trump-change-the-subject/index.html

This page is almost always displayed correctly, but if you remove the media carousel from the html it fails most times

media carousel code:

`<amp-iframe width="768" height="432" frameborder="0" layout="responsive" sandbox="allow-scripts allow-same-origin allow-popups" src="https://fave.api.cnn.io/v1/amp/?video=politics/2017/05/29/jared-kushner-proposed-secret-line-to-kremlin-sciutto-pkg-lead.cnn&canonical_url=&ssid=cnn.com_mobile_mobileweb_politics&seconds=151&videoId=politics%2F2017%2F05%2F29%2Fjared-kushner-proposed-secret-line-to-kremlin-sciutto-pkg-lead.cnn&imageUrl=https://ssl.cdn.turner.com/cnnnext/dam/assets/170516144834-02-jared-kushner-file-super-169.jpg&autoplay=&headline=White House defends Kushner's Russia contact&section=politics&source=cnn&videoCollection=true&id=h_8883b2800364f1d9a9b12925ce521ec1&customer=cnn&edition=domestic&env=prod&adServerRootUrl=dev&path=2017/05/30/politics/donald-trump-change-the-subject/index.html">

`

mauricionr commented 7 years ago

I have the same problem with amp fixed header using chrome on ios but when using amp-lightbox

the amp fixed header hide the content

SGudbrandsson commented 7 years ago

@dvoytenko do you have any updates or links to external issues so I can follow the progress of this? This is still broken on the stable iOS Chrome

dvoytenko commented 7 years ago

@muxin Do you have a response on the progress here? Did we ever file a bug against iOS Chrome?

dvoytenko commented 7 years ago

Chrome bug: https://bugs.chromium.org/p/chromium/issues/detail?id=687317

jridgewell commented 7 years ago

I looked into this today. This is both a iOS Chrome bug, and a Google viewer bug. Basically, resize and orientationchange events fire inconsistently, and very frequently they fire before the window size actually changes.

If iOS Chrome fired them consistently, the bug would be fixed. But Google viewer can be better about waiting for the window size to change before firing handlers.

See https://output.jsbin.com/pofoxuk to test.

jridgewell commented 7 years ago

Actually, we're not even listening for height resize events, only width changes. So when the browser header pops up (due to navigation), we don't even realize it.

dvoytenko commented 7 years ago

@jridgewell Interesting... Does it mean we can actually fix this on our side? It seemed to me that even the most basic statically positioned elements we affected, as if the edge of the document root was falling under the browser's address bar regardless of our paddings.

jridgewell commented 7 years ago

Yah, it's just a style that's using an incorrect height value (calculated before the browser header reappears). Once the header shows it'll trigger a resize, and we get the right height. We just need to listen for it.

jridgewell commented 7 years ago

This should be fixed internally (pending rollout). b/34890671.

dvoytenko commented 7 years ago

This should be deployed everywhere now.

SGudbrandsson commented 7 years ago

Awesome! Thanks Dima.

We'll test it and report back

On Wed, Aug 9, 2017 at 6:28 PM, Dima Voytenko notifications@github.com wrote:

This should be deployed everywhere now.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ampproject/amphtml/issues/7283#issuecomment-321341576, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiLGpPTf4mPh82RXn5WjJb3wUHG935Xks5sWfpAgaJpZM4Lzotq .

SGudbrandsson commented 7 years ago

It's a lot better now, but it still happens occasionally. To test, try opening the amp page a couple of times in Google, clicking the X button in between.

We reproduced this here: https://youtu.be/W-WikgxwxeI

noahnava commented 6 years ago

someone fix this?

ampproject / amphtml