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

Doubleclick served a4a ads do not work when loaded in a PWA with shadowdom #25007

Closed
indieisaconcept opened this issue Oct 10, 2019 · 19 comments · Fixed by #25321
Closed

Doubleclick served a4a ads do not work when loaded in a PWA with shadowdom #25007

indieisaconcept opened this issue Oct 10, 2019 · 19 comments · Fixed by #25321
Assignees

Comments

@indieisaconcept
Copy link

indieisaconcept commented Oct 10, 2019

What's the issue?

When loading an AMP document into a shadowdoc as part of PWA, should the document contain an amp-ad of type "doubleclick" which loads an amp4ads based ad, the ad fails to load due to numerous errors related to No ampdoc found for [object HTMLHtmlElement].

image

How do we reproduce the issue?

A demo is available here: https://d1kvlucx6m02nr.cloudfront.net/

Upon first ( uncached ) load the ad loads correctly as the document is yet to be loaded within the shell container.

image

However as soon as navigation occurs & the shell container is used the ad fail to load.

  1. Navigate to https://d1kvlucx6m02nr.cloudfront.net/
  2. Click on "Article 1"

What browsers are affected?

All browsers

Which AMP version is affected?

HTML shadows – Version 1910071803120

Based on previous issues related to PWA, shadowdoc & Doubleclick it looks like there may have a been a regression.

Additionally the a4a ad example loaded via projects example of a PWA is also broken. http://localhost:8000/pwa/

@indieisaconcept indieisaconcept changed the title Doubleclick served amp ads do not work with shadowdoc Doubleclick served a4a ads do not work when loaded in a PWA with shadowdom Oct 12, 2019
@indieisaconcept
Copy link
Author

Just wondering whether this issue is on anyones radar in @ampproject/wg-ads

@jeffjose
Copy link
Contributor

@calebcordry Would you mind taking a quick look?

@indieisaconcept
Copy link
Author

Thanks @jeffjose & @calebcordry. FYI I had to replace the ad as it is no-longer being trafficked. I've replaced it with the a4a example.

@mehigh
Copy link

mehigh commented Oct 24, 2019

I've looked at the demo page myself in the latest Chrome Canary - it behaves in the same way as the screenshot posted here. It's behaving in the same fashion in Safari on OSX, so I can rule this out as not being a browser-specific quirk.

@derekherman
Copy link

@calebcordry could you estimate when you would be able to put some headspace around this issue or find someone else on the AMP team that could?

@calebcordry
Copy link
Member

@indieisaconcept @mehigh @derekherman Sorry for the delay.

@indieisaconcept Thanks a lot for the example page.

Did some searching and I think this might be related to the work in #22734 @dvoytenko would you mind taking a look?

@indieisaconcept
Copy link
Author

@calebcordry thanks. Happy to provide further support/examples as required to assist with getting this resolved.

@dvoytenko
Copy link
Contributor

@calebcordry The #22734 is not launched yet. And based on error stacks I can confirm that this is not in the same codepath.

@calebcordry
Copy link
Member

@dvoytenko thanks for checking. I will dig more into this today.

@calebcordry
Copy link
Member

cc/ @ampproject/wg-ads

@indieisaconcept
Copy link
Author

@calebcordry I had chance to pull down your branch. It looks like the change your proposing in #25321 resolves the issue which is great news.

@indieisaconcept
Copy link
Author

@calebcordry great to see this merged. When do you think this could be feasibly released?

@calebcordry
Copy link
Member

calebcordry commented Nov 1, 2019

@indianburger @indieisaconcept While debugging this issue we also found a related issue fixed in #25357. We are currently discussing the risk/reward in trying a cherry pick. I will update here with the resolution. Worst case it would follow the normal AMP release schedule.

@dvoytenko
Copy link
Contributor

Cherrypick is in #25367, so hopefully it can be in PROD sooner.

@calebcordry
Copy link
Member

@indieisaconcept Change should now be live in canary. You can opt in at https://cdn.ampproject.org/experiments.html . I tested the new version on you example page, but feel free to test on your own, and let us know if you run into anything unexpected.

@indieisaconcept
Copy link
Author

@calebcordry thanks! I can confirm based on testing that I am seeing the same behaviour as yourself with the example page & when testing against our pre-prod environment I am seeing ads load correctly.

@indieisaconcept
Copy link
Author

@calebcordry, @dvoytenko just wanted to say thanks for getting this resolved so quickly. I verified again today following the prod release.

@dvoytenko
Copy link
Contributor

@indieisaconcept Thanks for reporting and helping us repro this!

@jeffjose
Copy link
Contributor

jeffjose commented Nov 7, 2019

Thanks for the quick turnaround on this, @calebcordry and @dvoytenko!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants