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

Whitelist amp-lightbox as part of an AMP ad #7015

Closed
jasti opened this issue Jan 12, 2017 · 5 comments
Closed

Whitelist amp-lightbox as part of an AMP ad #7015

jasti opened this issue Jan 12, 2017 · 5 comments

Comments

@jasti
Copy link
Contributor

jasti commented Jan 12, 2017

amp-lightbox is currently not allowed in an AMP ad. Here is a legitimate use case.
https://joystick.app.box.com/s/oo42mzgbx8t6leohqfl4se98pvtcxzyp
CC @ampproject/a4a

Can we whitelist it?

@keithwrightbos
Copy link
Contributor

@dvoytenko / @tdrl do you recall if there was a reason we did not allow it to start? Do we expect it to behave properly when within a friendly frame?

@dvoytenko
Copy link
Contributor

amp-lightbox (and amp-image-lightbox) take over the whole viewport. This is what we want to do here as well?

In A4A we can do this. I'm pretty sure it doesn't currently fully work.

In 3p case, however, we need to establish some form of control. This would have to be done via messaging from the 3p iframe. We can't blankly allow all such requests from 3p. We have to ensure that request is sent on user action. This is similar how requestFullscreen API works. That being said, I think we can find a way to do this, we just need some research.

Btw, this is something we've been talking to browsers about. See whatwg/html#1983 and https://bugs.chromium.org/p/chromium/issues/detail?id=621631

@adelinamart adelinamart added this to the Pending Triage milestone Jan 18, 2017
@jasti jasti modified the milestones: Prioritized FRs, Pending Triage Feb 10, 2017
@jasti jasti self-assigned this Feb 10, 2017
@Gregable Gregable removed their assignment Feb 10, 2017
@jasti
Copy link
Contributor Author

jasti commented Feb 21, 2017

@dvoytenko, I do mean just whitelisting it for A4A. Any more detail on what's blocking there?

@dvoytenko
Copy link
Contributor

The full-screen overlay cannot escape the boundary of an iframe. So to make this work correctly in AMP, the whole iframe should be temporarily resized to take over the whole viewport. That by itself is ok, but we need to make sure the transition itself is not overly disruptive.

@jasti
Copy link
Contributor Author

jasti commented Mar 30, 2017

Closing this in preference of #2736.

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

No branches or pull requests

8 participants