Yahoo amp-ad plugin #5122

src-code · 2016-09-20T17:39:36Z

Initial support for Yahoo display and Gemini ads via amp-ad plugin.

googlebot · 2016-09-20T17:39:38Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-09-20T18:22:27Z

CLAs look good, thanks!

lannka

we recently added new API: renderStart
Could you take a look and implement the APIs (renderStart + noContentAvailable)?
It helps AMP to provide a better ad loading experience.
Let me know if you have any questions regarding that.

lannka · 2016-09-20T19:37:44Z

ads/yahoo.js

+}
+
+export function yahoo (global, data) {
+    var cb = done.bind(this, global, data);


we use 2-space indentation

lannka · 2016-09-20T19:38:21Z

ads/_config.js

+  yahoo: {
+    prefetch: [
+      'https://s.yimg.com/os/ampad/display.js',
+      'https://s.yimg.com/os/ampad/gemini.js'


please append trailing ,

lannka · 2016-09-20T19:38:27Z

ads/_config.js

+      'https://s.yimg.com/os/ampad/display.js',
+      'https://s.yimg.com/os/ampad/gemini.js'
+    ],
+    preconnect: 'https://us.adserver.yahoo.com'


lannka · 2016-09-20T19:41:07Z

examples/ads.amp.html

+      data-sid="954014446"
+      data-site="news"
+      data-sa='{"LREC":"300x250","secure":"true","content":"no_expandable;"}'>
+  </amp-ad>


Could you also add an example for adtype=gemini

lannka · 2016-09-20T19:41:41Z

ads/yahoo.js

+/**
+* @param {!Window} global
+* @param {!Object} data
+*/


pls move the JS doc to the right place

Went ahead and restructured this code a bit.

src-code · 2016-09-20T22:20:52Z

@lannka Since our ad implementation will live externally from this code, I believe we'll have to adopt the renderStart/noContentAvailable APIs in those scripts. That said,
It's unclear to me that we can make use of those APIs for our initial display ad implementation, since we'll be nesting an iframe inside amp-ad's iframe, and won't necessarily have access to the relevant state.

By the way, the documentation for renderStart references waitForRenderStart list in _config.js which no longer exists - I assume that whitelist is no longer necessary?

src-code · 2016-10-04T21:22:20Z

Hi @lannka - Checking in, anything further need to be done for this one?

jridgewell · 2016-10-05T23:00:49Z

ads/yahoo.js

-export function yahoo (global, data) {
-  var cb = (function (global, data) {
+export function yahoo(global, data) {
+  const cb = (function(global, data) {


This bound function isn't necessary:

function cb() { window.ampAdExecute(global, data); } // or const cb = () => { window.ampAdExecute(global, data); };

lannka

Apologize for the delay.

I cannot get both examples working locally (both are blank).

Could you follow the guidelines and get it work locally?

lannka · 2016-10-10T16:34:02Z

examples/ads.amp.html

+      data-sa='{"LREC":"300x250","secure":"true","content":"no_expandable;"}'>
+  </amp-ad>
+
+  <h2 id="yahoo-gemini">Yahoo Gemini</h2>


id is not needed here. Each ad network will have one ID for navigation.

lannka · 2016-10-10T16:40:18Z

examples/ads.amp.html

+      type="yahoo"
+      data-sid="954014446"
+      data-site="news"
+      data-sa='{"LREC":"300x250","secure":"true","content":"no_expandable;"}'>


please also add the following tags inside amp-ad

<div placeholder></div> <div fallback></div>

lannka · 2016-10-10T16:46:01Z

@src-code render-start is not available in nested iframe yet. Will let you know once its ready.
The waitForRenderStart list is replaced by the renderStartImplemented: true in _config.js. Documentation was updated.

Thanks for your patience. Please address the comments and I will take another look.

src-code · 2016-10-17T23:38:12Z

@lannka Thanks, I'll update with fixed examples.

Though for the Gemini case, I likely will just pull that out. Gemini is a little different in that it's a native ad which we're rendering inside content populated with amp-list. amp-ad feels like the wrong approach for these ads, since we've already got the ad rendering available, so there's no reason to inject an iframe there (and we want to share styling with the content.) Really the only reason I can find to use amp-ad is for the ad viewability tracking, which doesn't seem to be solvable with the recently-added viewability triggers supported by amp-analytics. I'd hate to use amp-ad as just a tracking pixel in this case, or sticking the whole ad into the placeholder/fallback nodes and letting amp-ad fail silently (if that'd even work?) Curious to know if you've seen other use cases such as this, and have a recommendation for how to best handle this sort of native advertising?

lannka · 2016-10-17T23:48:44Z

Using amp-list to display ads might be a mis-use of the widget. @jasti @cramforce, is A4A our final answer to these kind of requirements?

src-code · 2016-10-17T23:55:51Z

@lannka In our case, the ads are co-mingled in the feed that returns article data, which is why they end up rendered by amp-list. In a sense they're just sponsored articles, but with the additional requirement of viewability tracking. There's no need for an ad call since we already have all the data available to us, and we want the rendering to mostly match the other non-ad content. I'm not sure A4A helps for those reasons?

src-code · 2016-10-18T00:17:58Z

In a sense, I think what I want is the ability to pass in the rendered ad via a child element such as what's done for placeholder or fallback. The only problem then would be viewability tracking, since I wouldn't have access to the observeIntersection APIs to help me in that case. And the amp-analytics viewability tracking is very limited (only amp- elements, selectors must be tag names or ids, and only fire once per selector, etc), so doesn't seem to be helpful here.

jasti · 2016-10-18T20:39:31Z

@src-code @lannka why not submit an embed similar to what Taboola does?
http://output.jsbin.com/lenubepone Is it possible to get just the sponsored articles part of a separate call? This way, you'll get observerIntesection API too.
If you just want to use amp-list, that's fine but we'll consider it to be just part of the content and there will no special treatment.

src-code · 2016-10-18T23:38:39Z

@jasti We could certainly go the embed route - I'll explore that next, and if it seems promising, open a separate PR. Meanwhile, I'm going to clean this one up a bit.

src-code · 2016-10-19T07:44:46Z

@laanka @jasti Actually, now that I think of it, going the embed route would not really help us much - we want to track the visibility of individual stories, not the aggregate. The sponsored stories are intermixed with the non-sponsored stories, so we want to track each sponsored story independently... So it seems like the only way to do that is to make each sponsored story its own amp-ad/amp-embed, but then we're back to the problem of having to feed pre-rendered content to the amp-ad via properties, or using amp-ad only as a tracking pixel - both of which seem wrong.

It'd be great if amp-ad would support a rendering option that didn't require an iframe, so we could just wrap existing, simple ad markup that doesn't require its own javascript, only than the amp-ad plugin code to track and beacon visibility. Does that make sense, and would it seem possible? Or is there a better way?

jasti · 2016-10-19T14:29:43Z

@src-code Not convinced using amp-ad/embed for each sponsored story is a bad idea. I don't think we could render a non A4A ad without an iframe.

src-code · 2016-10-19T16:31:16Z

@jasti Does A4A work outside of iframes? I was under the impression that it must be a fully-formed AMP document, which seems to imply iframes? There's no reason these ads couldn't be A4A, since they're just plain markup without javascript, but if they have to be iframed then it doesn't seem to help me much. My primary goal is to avoid iframes so I can take advantage of existing page styling, but I still need viewability tracking.

Maybe what's needed then is an enhancement to amp-analytics visibility tracking? Having the ability to track these elements in a more fine-grained fashion (eg by class rather than id or tag), and firing a separate beacon for each matching element, even for content injected late via amp-list, would be sufficient for what I'm trying to do...

lannka · 2016-10-19T16:50:35Z

@src-code A4A is in a same-origin iframe.

If you put amp-ad as an item of amp-list, you should be able to track its visibility using amp-analytics, no matter A4A or not.

For CSS sharing, @dvoytenko do we currently have a way to pass CSS to A4A?

src-code · 2016-10-19T17:05:40Z

@lannka

If you put amp-ad as an item of amp-list, you should be able to track its visibility using amp-analytics, no matter A4A or not.

As I understand it, amp-analytics will only match on id or tag name, so there's no good way to match a set of ads of a particular type, as I'd need here (I'd want to write a selector like amp-ad.gemini). Also, it'll only fire 1 beacon for all matching elements, rather than 1 beacon per element, I believe? And I can't select by id since I don't know what ids will be present on the page until after the amp-list renders.

But it's a moot issue, because if I'm going to use an amp-ad plugin anyway, the viewability tracking becomes easy since I can use the observeIntersection API. But then, styling becomes more difficult, since I'm stuck in an iframe.

dvoytenko · 2016-10-19T17:10:57Z

@lannka

For CSS sharing, @dvoytenko do we currently have a way to pass CSS to A4A?

Sure. The HTML is allowed to contain <style amp-custom>...</style> just like a normal AMP document.

src-code · 2016-10-19T17:23:27Z

In some ways, it seems like #5541 is similar to what I'd want (no iframe on amp-ad) but I'd want to avoid the network request (I already have the markup) and I'd want to track the visibility somehow (enhancements to amp-analytics to allow selectors to match multiple nodes, including nodes rendered via amp-list, and beacon as each becomes visible.)

lannka · 2016-10-20T17:24:31Z

@src-code #5541 will only work on pure image ad.

I see you want to do two things:

CSS sharing: I would go for A4A
an enhanced amp-analytics selector: ask @avimehta :-)

src-code · 2016-10-20T17:30:12Z

@lannka My ads are almost pure image ads - they just have a couple lines of text in addition to a single image. So the need is quite similar, I think.

Actually I have another proposal I am working on for a change to amp-ad that'll solve this, and I will open an issue to discuss. I hacked around in amp-ad yesterday and got it working pretty easily, and seems to solve my issues without having go overcomplicate things with A4A and amp-analytics. Will post more soon...

src-code · 2016-10-20T20:51:44Z

This PR diverged a bit with all the discussion about native ads, so I'm going to close it out in favor of a fresh PR dedicated solely to Yahoo display ads.

cramforce added NEEDS REVIEW WG: monetization labels Sep 20, 2016

cramforce assigned lannka Sep 20, 2016

lannka added the 3P label Sep 20, 2016

lannka requested changes Sep 20, 2016

View reviewed changes

jridgewell reviewed Oct 5, 2016

View reviewed changes

lannka requested changes Oct 10, 2016

View reviewed changes

src-code added 2 commits October 17, 2016 15:10

Adding Yahoo amp-ad plugin

7e1fa05

Removing Gemini

1c34e33

src-code mentioned this pull request Oct 20, 2016

I2I: Rendering native/simple ads outside amp-ad iframe #5721

Closed

src-code closed this Oct 20, 2016

src-code mentioned this pull request Oct 20, 2016

Yahoo amp-ad plugin #5727

Merged

Yahoo amp-ad plugin #5122

Yahoo amp-ad plugin #5122

Conversation

src-code commented Sep 20, 2016

googlebot commented Sep 20, 2016

googlebot commented Sep 20, 2016

lannka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

src-code commented Sep 20, 2016 • edited

src-code commented Oct 4, 2016

Choose a reason for hiding this comment

lannka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka commented Oct 10, 2016

src-code commented Oct 17, 2016

lannka commented Oct 17, 2016

src-code commented Oct 17, 2016

src-code commented Oct 18, 2016

jasti commented Oct 18, 2016

src-code commented Oct 18, 2016

src-code commented Oct 19, 2016

jasti commented Oct 19, 2016

src-code commented Oct 19, 2016 • edited

lannka commented Oct 19, 2016

src-code commented Oct 19, 2016

dvoytenko commented Oct 19, 2016

src-code commented Oct 19, 2016 • edited

lannka commented Oct 20, 2016

src-code commented Oct 20, 2016 • edited

src-code commented Oct 20, 2016

src-code commented Sep 20, 2016 •

edited

src-code commented Oct 19, 2016 •

edited

src-code commented Oct 19, 2016 •

edited

src-code commented Oct 20, 2016 •

edited