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

Intent to implement: amp-ad-banner #8334

Closed
alanorozco opened this issue Mar 22, 2017 · 5 comments
Closed

Intent to implement: amp-ad-banner #8334

alanorozco opened this issue Mar 22, 2017 · 5 comments
Assignees
Labels
INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code P2: Soon Type: Discussion/Question WG: monetization
Milestone

Comments

@alanorozco
Copy link
Member

/cc @lannka

Background

We want to support lightboxes in AMP ads (#7015). The lightbox element must be sandboxed as part of the ad frame, but should visually cover the entire viewport when opened. In order to do this, the iframe containing the ad must be set to position: absolute and cover the entire viewport before the lightbox is opened. If we resize the frame as-is, the following problems occur:

  • The body tag could be styled as part of the creative. This means that if we change the size, background, padding or other properties, the creative will look broken.

  • The content of the creative will shift in position in the page.

The lightbox component has an opacity transition, which causes the intermediate state to be visible.

Illustration of problem

screen shot 2017-03-22 at 4 16 53 pm

Proposal

An intermediate element that contains the visual components of the creative banner must be inserted in the ad's root. This element will be repositioned when in lightbox mode so that the creative won't shift visually.

Desired result

screen shot 2017-03-22 at 4 18 42 pm

The iframe manager could potentially create the intermediate element when entering lightbox mode, but this is finicky since it would have to copy a certain set of <body> children and could have unintended side effects for the author of the creative due to conflicts in style properties.

A better solution is to introduce a <amp-ad-banner> element which would be responsible for delimiting the visual area of the banner.

This element would be sized and positioned by the iframe manager when a lightbox is opened. The author would then freely style the amp-ad-banner element as the visual root of the ad, potentially applying the styling that would otherwise be added to the body tag.

Details

  • amp-ad-banner must be a dummy extension without much (if any) component logic.

    It's not necessary for the component to have logic besides resizing.

  • Layout must be set to fill

    This is so that it's clear that sibling elements won't be visible in the ad.

  • Validation rules must specify that only one amp-ad-banner element can appear in the document.

    Same as above.

  • Validation rules for amp-ad-banner must specify that only amp-lightbox elements are valid siblings.

    Same as above.

  • Validation rules for amp-lightbox must specify that an amp-ad-banner element must exist as its sibling.

    Otherwise the runtime would not be able to reposition the creative properly.

Open questions

  • What happens with with more complicated ad types (e.g. flying carpet)?
@alanorozco alanorozco added WG: monetization INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code P2: Soon Type: Discussion/Question labels Mar 22, 2017
@alanorozco alanorozco added this to the New FRs milestone Mar 22, 2017
@jasti
Copy link
Contributor

jasti commented Mar 23, 2017

This looks great!

  • Validation rules for amp-ad-banner must specify that only amp-lightbox elements are valid siblings. --> parent, right?
  • I'd suggest changing the name to something more obvious like amp-lightbox-detail.
  • Also, can't this be extended beyond just ads?

@lannka
Copy link
Contributor

lannka commented Mar 23, 2017

Instead of introducing a new element, I tried to leverage the body element:
http://output.jsbin.com/kagidel

@jasti
Copy link
Contributor

jasti commented Mar 23, 2017

Just a note that this should also be tested when delivered from the DFP server. Sometimes, I've seen that the ad gets served into multiple cross domain iframes. Not sure if it matters.

@alanorozco
Copy link
Member Author

alanorozco commented Mar 23, 2017

@lannka

That example doesn't seem to work? The banner expands to the full-size of the frame which is exactly what we don't want.

@jasti

I think I didn't explain myself properly in the description. The banner does not specify the lightbox content. The markup would look like:

<body>
  <amp-ad-banner>
     This is what is displayed when the lightbox is closed.
  </amp-ad-banner>
  <amp-lightbox>
     This is the lightbox content
  </amp-lightbox>
</body>

Just a note that this should also be tested when delivered from the DFP server. Sometimes, I've seen that the ad gets served into multiple cross domain iframes. Not sure if it matters.

Would the friends not be "friendly frames" if it's A4A inside an AMP doc? @lannka?

For the inabox use case we would need to put the iframe resizing logic somewhere, but that's unrelated to this FR.

Also, can't this be extended beyond just ads?

The use case is for ads. Not sure in which other contexts we would like to anchor the position of an element in an iframe that expands.

@lannka
Copy link
Contributor

lannka commented Sep 8, 2017

Obsolete. We went with the body element solution: #10699

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code P2: Soon Type: Discussion/Question WG: monetization
Projects
None yet
Development

No branches or pull requests

3 participants