ampproject / amphtml

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

collapse ad space issues #9546

Closed coreybyrnes closed 5 years ago

coreybyrnes commented 7 years ago

Let me start by saying that I know if an ad is not in view, no ad loads, and there's no fallback that the ad space will collapse. But we have the need to run blank ads for reporting and for ad bundle purchasing and we need these blank ads to collapse the ad space. So then I recently learned of the context.noContentAvailable() function which collapses the ad space but kills the content of the iframe which doesn't allow the impression to load so there is no tracking. So how can I kill the ad space but still have an impression load. I'm trying to specifically solve for the "sticky" ad space, but my question applies to any ad space. I have also tried window.context.requestResize(0,0); but it wasn't executing (and strangely I wasn't seeing the onResizeSuccess or onResizeDenied listeners fire).

Here's a test url where currently I am using context.noContentAvailable(). You can see the view?xai=... in the network tab is cancelled - that's the impression failing to load. http://www.cbsnews.com/amp/news/james-comey-fired-fbi-director-in-2014-60-minutes/?adNetwork=7336&adtargeting_campaign=sticky_7336

taymonbeal commented 7 years ago

@lannka @ampproject/a4a

lannka commented 7 years ago

@coreybyrnes just try to understand your problem: so you want to count an "impression" (i actually would not call this an "impression") even when the ad slot does not get filled with an ad? Is it to calculate a fill rate? If not, what's your real use case?

/cc @zhouyx

coreybyrnes commented 7 years ago

@lannka But it is being filled with an ad... just a blank one. ;) We do this in almost all our ad slots across the entire CBSi network of sites - when no ad is running, we run a blank creative that collapses the space. There are many reasons/cases for it, but for example one case is when an advertiser purchases a complete buyout of all ad slots on the page but doesn't have a creative for a particular slot (to guarantee no other advertiser's ad runs in that slot).

keithwrightbos commented 7 years ago

Have you considered trying to resize the creative to 0x0 instead of noContent? I believe noContent causes the frame to be removed while resize should leave it in place but with no size allowing the delayed impression to potentially fire.

coreybyrnes commented 7 years ago

@keithwrightbos thanks for the reply, but as I stated in the original post the resizeRequest isn't working and the resize events aren't firing. So not sure what's causing that, but this specific case is slightly more complex because I'm trying to collapse the sticky component which is the parent to the ad slot so I'm unsure if a resize would reach that far up.

keithwrightbos commented 7 years ago

Apologies, I missed that! Sounds like investigating why resizeRequest is not working is the ideal solution. Definitely odd and needs to be run down.

coreybyrnes commented 7 years ago

@keithwrightbos no worries!

@lannka @zhouyx let me know what you think! I can update my test url (posted in the original post) to be using context.requestResize(0,0) instead of context.noContentAvailable if that seems to be a better option. Then we can attempt to diagnose why it's not executing and if it will collapse the sticky component at the same time?

zhouyx commented 7 years ago

@coreybyrnes FYI: we don't allow resizing for sticky ad component, I think that's why context.requestResize(0, 0) doesn't work.

I still trying to understand why you still want to keep the iframe after ad collapse.

for example one case is when an advertiser purchases a complete buyout of all ad slots on the page but doesn't have a creative for a particular slot (to guarantee no other advertiser's ad runs in that slot).

But it won't run other advertiser's ad either after we kill the iframe? Or do you need to keep the iframe to calculate total impression time or so?

taymonbeal commented 7 years ago

@zhouyx, if I understand this use case correctly, the iframe only needs to be kept alive long enough to ensure that the delayed impression beacon has a chance to fire, for accounting purposes. In the above link, it was showing up as "(canceled)" in the Chrome dev tools.

zhouyx commented 7 years ago

Thanks @taymonbeal @coreybyrnes If that is the case, iframe has full control on when to send no-content. would sending no-content after impression tracking finish help?

coreybyrnes commented 7 years ago

@taymonbeal Thanks for chiming in and clarifying.

@zhouyx I don't have control over when the impression fires - it's controlled by DFP. I will investigate if I can override that and manually fire it for this sticky slot only, but I know that won't be a universal option for all our slots so the original question/issue still stands.

coreybyrnes commented 7 years ago

@zhouyx @lannka I tried manually firing an impression before calling noContentAvailable but it doesn't work. The ad network inserts the scripts that handle the impression after the creative code, so as soon as you call noContentAvailable everything is wiped. I tried to set up a timer to call noContentAvailable after 10s but it never fired.

Ideally there would be a new function, something like collapseSlot, that would simply set the ad call to display:none and also see if the ad call was nested inside a component like the sticky or flying-carpet and collapse that as well. Thoughts?

taymonbeal commented 7 years ago

@coreybyrnes, what happens if you try putting the call to noContentAvailable inside an onload event listener?

coreybyrnes commented 7 years ago

@taymonbeal Thanks for the suggestion, but it unfortunately didn't fire. I'm sure there's a reason, but it seems like noContentAvailable won't execute if there's any delay either through a timer or waiting for the onload event. Here's what I tried (and I do see the log in the console):

window.addEventListener('load', function(){ console.log('window loaded'); window.parent.context.noContentAvailable(); }, false);

You can see this on the test url from the original comment: http://www.cbsnews.com/amp/news/james-comey-fired-fbi-director-in-2014-60-minutes/?adNetwork=7336&adtargeting_campaign=sticky_7336

taymonbeal commented 7 years ago

Ah, I think I know what's wrong here! The relevant postMessage listener listens for only one message, which can be either render-start or no-content. Once it's received one, it stops listening and any further messages are ignored.

Fellow maintainers: would you be in favor of changing this so that noContentAvailable() can be called at any time?

coreybyrnes commented 7 years ago

@taymonbeal thanks for making that discovery!

zhouyx commented 7 years ago

I am not sure about handling no-content after render-start. The point of render-start is to inform AMP runtime when the ad starts rendering and set the ad visible to user. So I think it's quite reasonable for AMP runtime to assume only one message will be received. In the special case for sticky ad. Once AMP runtime receive render-start it will unhide the ad. Upon receiving another no-content, runtime will hide the ad again. In this case user will notice a blank ad blink, which is exactly what we want to avoid.

coreybyrnes commented 7 years ago

@zhouyx @taymonbeal But I am able to run a creative that calls noContentAvailable() and it does indeed kill the iframe and in the case of the sticky it doesn't ever appear - no "blink". It's only if I try to call noContentAvailable after a delay via setTimeout or waiting for the window.onload event that it won't execute. But we need to delay it until the impression has successfully rendered.

zhouyx commented 7 years ago

@coreybyrnes delaying the call to noContentAvailable should work. I used to delay the signal by few seconds during manual testing and it works as expected. When you delay the call to noContentAvailable, do you have control of renderStart? I assume it's possible that renderStart is called by nested window since impression has successfully rendered.

coreybyrnes commented 7 years ago

@zhouyx I don't believe I have control over renderStart, but I'm really not sure how/when that is fired. Perhaps it is tied to the impression? Really not sure, but it seems like if the impression successfully fires then noContentAvailable cannot execute.

taymonbeal commented 7 years ago

Since you're using useSameDomainRenderingUntilDeprecated, renderStart is triggered by the GPT event slotRenderEnded. There's no synchronization between this and the delayed impression beacon, so in principle they could occur in either order.

coreybyrnes commented 7 years ago

@taymonbeal thanks for the clarification. So can we test out your idea to allow a noContentAvailable call even if a renderStart has fired? It seems worth testing to see if an ad blink would occur if the noContentAvailable fired on window.onLoad.

zhouyx commented 7 years ago

@coreybyrnes Sorry I am super confused again. 😢

First I want to make sure this discussion is all about 3P ads render in a cross origin iframe.

Then I have few questions: How would you tell if an ad is blank ad that should be collapsed? When you make the change to delay noContentAvailable where are you making the change? Since slotRenderEnded event is triggered, does it mean that the ad slot load a rendered ad? Why still want to call noContentAvailable?

coreybyrnes commented 7 years ago

@zhouyx Sorry for the delayed response. To recap, this is about a DFP creative that is intended to collapse the ad space by setting display:none. We need to serve these "blank" creatives that collapse the ad space for tracking purposes (hence the need to preserve the impression) and more importantly when an advertiser buys all ad spaces on a page but doesn't have a creative for a specific ad space and needs to guarantee no other advertiser runs in that slot.

How would you tell if an ad is blank ad that should be collapsed?

A blank creative is built in DFP and scheduled to a campaign line.

When you make the change to delay noContentAvailable where are you making the change?

In the DFP creative itself, where I was trying to call it in order to collapse the ad space.

Since slotRenderEnded event is triggered, does it mean that the ad slot load a rendered ad? Why still want to call noContentAvailable?

Yes from the sounds of it, the slotRenderEnded event executes when a creative loads which would be the case with these "blank" creatives. We need to collapse the ad space, not kill the iframe as noContentAvailable does, for the reasons listed above.

From the feedback I've received, it doesn't sound like there's a way to collapse the ad space from with a creative. The only available function kills the iframe which doesn't allow the impression to fire so it isn't a viable solution. It seems like a new function might need to be added that the collapse the ad space with a display:none even if slotRenderEnded has executed. Thoughts?

coreybyrnes commented 7 years ago

@zhouyx @lannka @taymonbeal Any further thoughts on how I can accomplish this?

taymonbeal commented 7 years ago

I think that I or one of my colleagues could implement the suggestion that I made upthread, if we wanted to.

@zhouyx, obviously the usual AMP prohibition on user-visible reflow would still apply. If noContentAvailable were called while the ad was in the viewport, it wouldn't disappear, it'd just be replaced with a fallback or blank rectangle. Within the confines of their rectangle, we already can't prevent ads from blinking or whatever, because they can contain arbitrary JavaScript. In light of this, do you still think that adding such support would be a bad idea?

zhouyx commented 7 years ago

@taymonbeal Ads blinking is only an issue with sticky-ad, as we always collapse sticky-ad even it is in viewport. I would still argue calling noContentAvaiable after renderstart... If we have to, I would rather have a new API for this.

When you make the change to delay noContentAvailable where are you making the change? In the DFP creative itself, where I was trying to call it in order to collapse the ad space.

Since we are making change in DFP creative, Is it possible to add logic in creative to prevent calling of renderStart?

coreybyrnes commented 7 years ago

@zhouyx I don't believe you can prevent the calling of renderStart from within DFP creative code. If a new API function is going to be built for this, can it be to simply collapse the element via display:none instead of killing the content as noContentAvailable does? With that I wouldn't have to delay calling it based upon if/when the impression has fired.

zhouyx commented 7 years ago

@coreybyrnes I wonder we would allow an iframe with display:none and not killing it.

To @lannka @jasti

Requests here: Creatives know they have nothing to serve. They want to put display:none and call noContentAvailable. In the meantime the creative still want the impression tracking to take place. Ad server however has no knowledge of this and will call renderStart()

Possible solution:

  1. Creative call noContentAvailable() after impression tracking is done. This is not applicable because ad server will call renderStart() before it.
  2. Make our code listen to noContentAvailable() or a new API emptyAd() after render-start is received. But I feel render-start message means nothing to us. This could lead to sticky-ad blink and other ad remove placeholder too early. Ideas?

BTW I'll be on vacation starting tmr😄

taymonbeal commented 7 years ago

What if we changed doubleclick.js so that renderStart was triggered by SlotOnload instead of SlotRenderEnded? @keithwrightbos, would this work and what other effects would it have?

michaelkleber commented 7 years ago

Calling renderStart from SlotOnload seems appealing. But are there ads that we make invisible until the renderStart call? If so, then a slow but visually irrelevant tracking pixel could delay iframe onload and be a visually-bad side-effect. So we'd need to be careful with such a change.

keithwrightbos commented 7 years ago

We are in the process of launching amp-ad-network-doubleclick-impl Fast Fetch and I suggest we focus on a fix there as it should make things less complex. @coreybyrnes are you using remote HTML or useSameDomainUntilDeprecated? @zhouyx are you free to meet and discuss? Thanks

coreybyrnes commented 7 years ago

@keithwrightbos I'm not sure how remote HTML or useSameDomainUntilDeprecated relates? I want to run a simple DFP creative that hides the element.

taymonbeal commented 7 years ago

@keithwrightbos: Yes, they're using both of those.

zhouyx commented 7 years ago

Sorry for the late reply, just return from my vacation.

But are there ads that we make invisible until the renderStart call? If so, then a slow but visually irrelevant tracking pixel could delay iframe onload and be a visually-bad side-effect. So we'd need to be careful with such a change.

Yes. For normal ads in cors iframe. We only toggle off placeholder (set to visible in case of sticky ad) after receiving renderStart call.

@keithwrightbos Yes. I am happy to discuss more on this use case : )

coreybyrnes commented 7 years ago

Any further thoughts on a solution for manually collapsing the amp-ad container from with ad code? :)

taymonbeal commented 7 years ago

We're currently working on an extension to <amp-analytics> that will allow publishers to listen for ad-related events, such as no-fills. Does this suffice for your use case? If not (like if you need information from inside the creative) then we have a few other ideas for how this might be made to work but they're still rather preliminary.

coreybyrnes commented 7 years ago

@taymonbeal It doesn't sound like that will work because my scenario is a creative that manually collapses the space. From my original post:

we have the need to run blank ads for reporting and for ad bundle purchasing and we need these blank ads to collapse the ad space

taymonbeal commented 7 years ago

If you're not actually going to display an ad in a slot, then it's better if we don't create an iframe for it. Clearly you still need things to happen when the ad serves, but we're trying to figure out if there's some other way to meet that need. This wouldn't let you run arbitrary JavaScript, but maybe you don't need arbitrary JavaScript. For now you just want to make sure that the standard DoubleClick delayed impression beacon fires, right?

coreybyrnes commented 7 years ago

The primary purposes of our "blank" creative is to (1) collapse the ad space and (2) ensure no other ad can run in the space if an advertiser wants to own all ads on the page but doesn't have an ad for a specific slot size. So yes we want to make sure the DoubleClick impression executes but we also want to have the ability to make sure no other ad loads. There is other functionality as well, but those are the primary reasons. So again if there was another context function available that would simply hide the space instead of killing the iframe like context.noContentAvailable() does, that would be 100% ideal.

robhazan commented 7 years ago

@coreybyrnes - this seems like it may not be a public discussion for the benefit of all consumers of the AMP codebase, nor a bug, but rather an implementation question specific to to CBS. Will you please reach out to your DFP account team, ask them to add me into the email, and we'll help you get this issue squared away? We have some ideas, but we'll need to ask you a couple more questions.

Thanks!

jasti commented 7 years ago

Closing for now. Please reopen if the DFP path doesn't work out.

jasti commented 6 years ago

@coreybyrnes think we can build something generic in AMP that kills the iframe when context.blankContent(), so that the delayed impression is fired. @zhouyx @keithwrightbos do you have any objections to this?

coreybyrnes commented 6 years ago

@jasti sounds great... how about collapsing an amp component? that's originally what started this inquiry from me so we're really hoping that can get resolved first as we can't run the flying-carpet or sticky without the ability to collapse the component container element. thanks so much for reopening this!!!

jasti commented 6 years ago

SGTM. @keithwrightbos and @zhouyx, do you see any technical issues with this?

coreybyrnes commented 6 years ago

@jasti any update here? thanks!

jasti commented 6 years ago

Sorry for the delay, @coreybyrnes. @lannka do you see any technical issue with suggested approach in https://github.com/ampproject/amphtml/issues/9546#issuecomment-345709338?

coreybyrnes commented 6 years ago

@jasti @lannka sorry to be a pest but my product managers are asking daily... any update on manually collapsing the component wrapper?

lannka commented 6 years ago

@jasti @keithwrightbos given the delayed fetch (together with those renderStart/noContent APIs) is going to be deprecated in doubleclick (#11834), should this be directly handled in the doubleclick fast fetch extension?

jasti commented 6 years ago

Yes, IMO. @keithwrightbos ?