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

:bug IFrame placeholder assertion fix #19786

Closed
wants to merge 1 commit into from

Conversation

acsant
Copy link

@acsant acsant commented Dec 11, 2018

Instructions:

  • Placeholder is null on layoutCallback
    • Call to updateLayoutBox in custom-element causes the layoutCallback on the amp-iframe, however this.placeholder_ is null - the context here seems to be different compared to when the IFrame was first rendered and the first layoutCallback was executed.

screen shot 2018-12-11 at 11 01 12 am

this.assertPosition above fails because this.placeholder_ is null

After adding the calls to this.getPlaceholder() on layoutCallback:

screen shot 2018-12-11 at 11 32 09 am

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@aghassemi
Copy link
Contributor

Thanks @acsant, really appreciate the initiative, we love contributions in AMP.

I assume this is for #19705 (comment) ?

I like to dig a bit deeper on the root cause of this. The properties you are setting in layoutCallback are already set in buildCallback and we should never get to a case where buildCallback is not called before layoutCallback. We need to fix the root cause of this.

Are you by any chance using amp-lightbox-gallery in this page? There was an issue where including that extensions was causing some race conditions in our component life-cycle which was fixed by #19778 couple of days ago ( not in production yet, but should be in Dev Channel - you can enable dev channel by visiting https://cdn.ampproject.org/experiments.html )

It would be great if you can test your page with Dev Channel enabled and let me know if issue is still there.

@eveyiyuan
Copy link

@aghassemi We are using amp-lightbox-gallery on this page. Thanks for the advice! Enabling Dev Channel seems to have fixed the issue! At least I haven't been able to reproduce it since enabling it.

@aghassemi
Copy link
Contributor

@eveyiyuan Awesome! Thanks for confirming. That fix will be in production next Tuesday Dec 18.

@aghassemi
Copy link
Contributor

@acsant @eveyiyuan Unrelated to this thread but since you are heavily using amp-lightbox-gallery, I wanted to mention that we have been working on making "swipe to dismiss" interaction much more smooth and native-looking recently. It will launch soon but if you like to take a look and give us feedback, you can see it here: https://amp-polish.firebaseapp.com/examples/article-with-lightbox-2.0-gallery.amp.html (on mobile, use swipe up/down gesture on lightboxed images to close the lightbox)

@aghassemi
Copy link
Contributor

Closing per discussions above.

@aghassemi aghassemi closed this Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants