ampproject / amphtml

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

Unable to access `window.context` when ads delivered via Glade #2236

Closed mjrusso closed 7 years ago

mjrusso commented 8 years ago

Part of #1937 (cc @gduchene) included the following change:

...an experiment on Glade, a new lightweight tag, on 1% of the AMP DoubleClick requests.

This introduces a behaviour change that is causing Polar ads to not render when delivered via DFP (note: this is not related to #2220). It is possible that other types of creatives that are delivered via DFP are also broken.

By default, amp-ad renders an iframe with the origin https://3p.ampproject.net.

GPT, subsequently, renders a number of iframes inside of this iframe. The iframe that the actual creative payload is delivered in to does not have a src attribute, and it can access its parent's window.context.

However, Glade renders the creative into an iframe with origin https://securepubads.g.doubleclick.net, and thus cannot access the parent (origin https://3p.ampproject.net) due to the Same-Origin Policy. This means that it isn't possible to access the data that amp-ad provides to its parent's window.context.

In the short-term, is it possible to disable using Glade for any request (unless google_glade=1), until a better longer-term solution is figured out?

clundie commented 8 years ago

It is possible to work around this and render the ad, but without context the ad cannot perform its own analytics or iframe resizing. Possibly other issues too.

kashyapnitin commented 8 years ago

Michael could you clarify - which specific window.context parameters do the polar creatives rely on being able to access - for them to work correctly? (and preferably what purposes do you use them for)? We would like to enable the Glade tag to be able to allow the right things to be exposed without having to compromise its design of loading on a cross domain iFrame.

Many thanks

clundie commented 8 years ago

It uses quite a few, such as:

master data.type, data.slot (to determine the doubleclick ad unit) referrer requestResize (native ads may have variable height & need to expand based on the content size which is only known after rendering) location canonicalUrl mode (determine if development mode is on, so we can deliver a larger development version of our script that does more logging etc.)

Some of this is needed for our optimization & reporting system to work properly; others affect native ad rendering (e.g. the location and/or canonicalUrlof the main page would be necessary to determine how to render the native ad, which is delivered to different sites of the same customer)

mjrusso commented 8 years ago

Some more colour re: resizing —

There's a bit of choreography that needs to happen to make the resizing work properly. requestResize (and onResizeSuccess/onResizeDenied) are part of the story, but in addition to the iframe that requestResize resizes, we also need to manually resize an additional iframe. This additional iframe is a child of the iframe that's resized via requestResize, and its inside here where the creative is actually rendered — for both GPT and Glade.

In some cases it's just a matter of adjusting the height. However there are other cases we need to handle, particularly those when the height and width are being overridden (via data-override-height, data-override-width). In these cases, the size of the iframe where the creative is rendered to is set (by GPT or Glade) to the overridden values, while the higher-level amp-ad iframe is a completely different size.

Currently, we do something like this (simplified):

  width = "100%"
  height = #...snip...
  window.parent.context.requestResize? width, height
  window.frameElement.width = width
  window.frameElement.height = height

These adjustments to window.frameElement.width and window.frameElement.height are not broken under Glade, but I thought a little extra context (pardon the pun) would be helpful.

nekodo commented 8 years ago

We're considering the following:

The two combined would mean that reaching out of the iframe would not be necessary and any changes using the resize API would just work.

mjrusso commented 8 years ago

@nekodo that sounds like a very reasonable approach.

Some comments:

It would be up to the creative to include it if desired.

The creative would need to know (or have the ability to find out) if it is running in an AMP environment. Preferably something reliable/supported/not a hack. Perhaps Glade can set an AMP-specific global in the ad iframe? (Sidenote: Is Glade is going to make its way to the non-AMP parts of the web at some point?)

A totally different option (but not withouts its own downsides) is to move the signalling up to the amp-ad element itself, so that you can indicate at the tag level if you want this JS file included (and, if so Glade would automatically include it).

(a subset of what is available to amp-ad implementations)

What subset did you have in mind?

nekodo commented 8 years ago

Pull request to enable the fill behaviour for Glade is merged now.

I'm looking into the creativeContext API. The only major thing that I do not think will be supported is the master iframe functionality, but lets wait until we actually have a patch that works end to end:)

mjrusso commented 8 years ago

Thanks @nekodo — #2375 looks great, and the (current) plan to support everything except computeInMasterFrame and supporting plumbing makes sense.

coreybyrnes commented 8 years ago

I'm unable to access window.context even when the origin is "https://3p.ampproject.net". I'm simply trying to spit out the location.href as a starting point (I eventually want to implement a resize). In my DFP creative I simply have the following code:

<script>
console.log('window.context.location.href = '+window.context.location.href);
<script>

Do I need anything else? Do I need to implement a listener to wait for the context object to become available? I tried searching all documentation and demos but wasn't able to find anything.

mjrusso commented 8 years ago

@coreybyrnes: you'll need to access context via the iframe's parent (i.e., window.parent.context).

This will change with Glade, which is what we're discussing in this issue thread.

coreybyrnes commented 8 years ago

@mjrusso Thanks so much. I had tried that in the past without success, but there must have been something else at play.

kashyapnitin commented 8 years ago

The Glade fill feature is now in prod. Michael could you reconfirm the behaviour you see with polar ads now? Are they still broken with the Glade tag? Could you kindly point us to a sample test page?

mjrusso commented 8 years ago

Sorry about the delayed response (vacation).

@rudygalfi: could you kindly re-open the ticket? There's a few parts here, and we still need the new context API.

@kashyapnitin: looks like the new fill code is working well. There's still going to be some sizing issues until we're able to call requestResize using the new context API, however thumbs up on #2375. (Also, here's a sample page: https://amp.polarmobile.com/dfptest.html?google_glade=1#development=1)

jasti commented 7 years ago

Please follow future updates for the new context API in this issue: https://github.com/ampproject/amphtml/issues/6829