ampproject / amphtml

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

ATF amp-iframe inside Shadow DOM not working on Safari #25755

Open gilbertococchi opened 4 years ago

gilbertococchi commented 4 years ago

ATF Amp-Iframe with a Placeholder inside a Sticky Element is not working and triggers error

dom.js:645 amp-iframe is not used for displaying fixed ad. Please use amp-sticky-ad and amp-ad instead.

Steps to Reproduce:

Screenshot with error below:

Screenshot 2019-11-25 at 11 18 06

Screenshot 2019-11-25 at 11 19 19 (2)

It works fine on Chrome Mobile and other browsers, it seems a iOS specific issue.

Previously reported in #18563

@iwoak @nainar

wassgha commented 4 years ago

I can not reproduce this, I can see the sticky audio when clicking the sound button

gilbertococchi commented 4 years ago

Hi Wassim, the issue is reproducible on large iOS devices such as "iPhone 6/7/8 Plus" Chrome Mobile UA emulation, or the Real Device but it has to be a Plus Screen iOS device and/or the Simulator.

The issue seems to be related with some viewport detection we apply to fixed Iframes here on large devices that triggers the error on iOS Plus devices only: https://github.com/ampproject/amphtml/issues/18563

Do you think we can change this logic to allow a non Ad usage of an ATF Amp-Iframe provided with a Placeholder to accomodate this?

wassgha commented 4 years ago

Investigated this and as per #3863 amp-iframe is essentially matching the size of the iframe to traditional ad sizes to detect whether the iframe is an ad or not (not sure if that's the optimal thing to do but given that we have no idea what content of an iframe would be considered an ad...) so a quick fix would be to play around with sizes different than a regular ad to circumvent this check. Not sure if we can do better than that, cc @jridgewell .

https://github.com/ampproject/amphtml/blob/ad497ca6f4bfb6b3ef0de2afa1bc7c10da0f502a/src/iframe-helper.js#L545-L563

wassgha commented 4 years ago

(to clarify, the issue is not Safari, it's the device's screen size)

wassgha commented 4 years ago

/cc @jridgewell on this

jridgewell commented 4 years ago

We can remove the restriction, it just needs an I2I and approval.

omerr132 commented 3 years ago

@jridgewell Any update on this? As of now we still get iframes blocked with this error message.

I'm not sure why - but this happens only to iframes that are inside lightboxes. The same iframe on the page itself doesn't get blocked .

stale[bot] commented 1 year 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.