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

amp-hidden not getting added to iframe placeholder after using Android native back #19705

Closed
eveyiyuan opened this issue Dec 7, 2018 · 9 comments

Comments

@eveyiyuan
Copy link

eveyiyuan commented Dec 7, 2018

What's the issue?

We are seeing placeholders not being hidden for amp-iframes after the iframes load when we navigate to the Google AMP viewer via the native / browser back on Android phones. This same issue does not appear to happen on iOS. The iframe content is fully loaded and responsive underneath the placeholder, and appears to work fine if we delete the placeholder element or change its z index. The amp-hidden class isn't added to the placeholder. We see this when we use a div or an image as a placeholder.

How do we reproduce the issue?

  1. Visit https://www.google.com/amp/s/www.redfin.com/UT/Park-City/3720-N-Sundial-Ct-84098/unit-B314/home/91868182/amp via the search results page on an Android device

  2. Click the search icon (the magnifying glass) in the header
    screen shot 2018-12-06 at 4 40 13 pm

  3. Use the native back button on the device to go back to the amp viewer

  4. Inspect any of the iframes (such as the fixed footer on the bottom of the viewport)
    screen shot 2018-12-06 at 4 39 34 pm

What browsers are affected?

Chrome on Android

Which AMP version is affected?

amp-version="1811272154520"
amp-iframe version 0.1

@aghassemi
Copy link
Contributor

Thanks for the bug report @eveyiyuan!

Likely a race between iframe's onload firing before we listen to onload. I've seen "Back" button do weird stuff for iframes in the past, like loading instantly from cache and firing events way too early.

@eveyiyuan
Copy link
Author

This there anything we can do to help this situation? Would firing a delayed embed-ready message from the iframe help? Thanks!

@acsant
Copy link

acsant commented Dec 10, 2018

I would also like to know if there is currently a workaround for this while it gets fixed?

Thanks,

@aghassemi
Copy link
Contributor

@eveyiyuan @acsant Sorry for the delay. I can't think of a workaround, we will be working on a fix in the next couple of weeks which should make it to production in early January.

@eveyiyuan
Copy link
Author

eveyiyuan commented Dec 11, 2018

@aghassemi No worries!
It seems like we are also experiencing a form of this bug after refreshing the page as well (not just after using the browser back button).
We are seeing the placeholder not render correctly when we open an amp-lightbox with an amp-iframe inside of it. This causes amp to complain about the lack of a placeholder and not render the iframe contents.

Error case:
screen shot 2018-12-10 at 4 11 06 pm

Expected case:
screen shot 2018-12-10 at 4 13 45 pm

To Repro:

  • Refresh the page linked to this bug
  • Click the mini google map (it doesn't happen consistently, but it seems to happen pretty often)

Once again it seems like we are only seeing this on Chrome and not on Safari.

Thanks for your help with this.

@aghassemi
Copy link
Contributor

@eveyiyuan thanks for the detailed report. Will look into that issue as well at the same time.

@acsant
Copy link

acsant commented Dec 11, 2018

@aghassemi For Eve's second issue where the content of the IFrame isn't being loaded, I noticed that it was failing the assertion check for the iframe having a placeholder when layoutCallback was called. So I traced through the code and noticed that when the resource manager invokes the layoutCallback on the iframe element, the context seems to differ and this.placeholder_ is null. Doing this quick change seems to fix the issue but I'm not sure whether this is just patching the underlying problem or if it's an actual fix: #19786

@ampprojectbot ampprojectbot added this to the Backlog Bugs milestone Dec 12, 2018
@ampprojectbot
Copy link
Member

This issue doesn't have a category which makes it harder for us to keep track of it. @aghassemi Please add an appropriate category.

@aghassemi
Copy link
Contributor

Per #19786 (comment), #19778 has also fixed the root cause of this issue.

Closing the issue for now. Please feel free to reopen in you noice the issue is not fully fixed. #19778 will rollout to production on Tuesday Dec 18th.

@aghassemi aghassemi assigned sparhami and unassigned aghassemi Dec 13, 2018
@aghassemi aghassemi moved this from To do to Done in FixIt - UI - December 2018 Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

5 participants