search

ampproject / amphtml

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

Iframe messaging doesn't work when ad fallback via Safeframe path #13326

Closed zhouyx closed 3 years ago

zhouyx commented 6 years ago

Please check the fallback example in https://github.com/ampproject/amphtml/blob/master/test/manual/amp-animation/scrollbound-embeds.amp.html#L62

Messaging between iframe and parent AMP document doesn't work because the sentinel is not matched.

We have our logic to check for sentinel in window.name, and if not found generate a random one. It seems that the safeframe script always remove window.name, and thus the script in iframe can not get correct sentinel

cc @lannka

jamesshannon commented 6 years ago

Shouldn't this be higher priority? It's breaking existing implementations of amp-ad that rely on fallback and collapsing?

@aghassemi

lannka commented 6 years ago

no creatives in production right now is using the messaging API, so it's not uncovered until now

jamesshannon commented 6 years ago

Oh? Maybe I'm seeing something else, then.

https://ampbyexample.com/components/amp-ad/#using-placeholders

Notice that the fallback doesn't work. I see that DFP is returning an effectively blank ad:

<html><body style="background-color:transparent"></body></html>

Which is then dutifully copied to the iframe.

In a test of collapsing (same ad without the <div fallback>) the iframe doesn't collapse. So i'm assuming that somewhere between DFP implementation, amp ad (and maybe the amp runtime) something has regressed recently. I came across this issue so figured that was it. Is it not related?

zhouyx commented 6 years ago

@jamesshannon This is not related to the bug here. This bug is about an AMP ad fallback to render in cross origin iframe case. Not the fallback element.

Whether to collapse the ad iframe or display the fallback element, depends on whether the ad is in viewport or not. We only try to collapse the iframe if it is not in viewport.

BTW https://ampbyexample.com/components/amp-ad/#using-placeholders The fallback works for me. Could you provide more information here? Thanks.

jamesshannon commented 6 years ago

It's not showing the fallback for me -- just an iframe body with a transparent background.

image

(This behavior was initially reported by Lonely Planet, so I don't think it's just me.)

I don't know enough about doubleclick impl or amp-ad to figure out which layer isn't working. While I would expect that the ad would collapse as long as it's not above the viewport ("AMP determines that this operation can be performed without affecting the user's scroll position"), but in any case the fact that <amp-ad> seems to be unaware to even show the fallback seems to me to suggest that that's not the issue.

ampprojectbot commented 6 years ago

This issue hasn't been updated in awhile. @bradfrizzell Do you have any updates?

ampprojectbot commented 5 years ago

This issue hasn't been updated in awhile. @bradfrizzell Do you have any updates?

calebcordry commented 5 years ago

@jamesshannon Is this still a problem for you?

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.