Modify amp sticky ad to only wait on render-start if render-start implemented. #21157

bradfrizzell · 2019-02-27T19:50:11Z

If render start is supported for a given network, then amp sticky ad should wait only on render-start, not render-start OR load_end.

Also, enable render-start for adsense.

This fixes a bug where Adsense sticky ads on canonical amp running delayed fetch could show a sticky ad for a no-fill.

…lemented.

keithwrightbos · 2019-03-01T17:39:13Z

extensions/amp-sticky-ad/1.0/amp-sticky-ad.js

-    ]).then(() => {
+    return this.ad_.getImpl().then(impl => {
+      const promises = [signals.whenSignal(CommonSignals.RENDER_START)];
+      if (!(impl.config || {}).renderStartImplemented) {


Seems odd that this ever used load end as a signal. I would instead of expected render start signal to be sufficient and that the AMP runtime just controls when the signal fires based on the impl config. In other words, for DF config w/o renderStartImplemented, render start is fired when the iframe is attached. While for configs w/ renderStartImplemented, it would be fired based on received postMessage triggered by context.renderStarted(). Are we positive that's no the case?

Was a goal here that the sticky shouldn't be made visible until the creative had completely loaded in order to reduce blank rectangle time?

keithwrightbos

Please get review/approval from Yuxuan (she can merge) and ensure related changes with adsbygoogle are in production.

zhouyx · 2019-03-27T01:51:48Z

extensions/amp-sticky-ad/1.0/amp-sticky-ad.js

+	  (!config && (
+	      !impl.ignoreLoadEndForAmpStickyAd ||
+		!impl.ignoreLoadEndForAmpStickyAd()))) {
+        promises.push(signals.whenSignal(CommonSignals.LOAD_END));


Supposedly LOAD_END event should come after RENDER_START. Is it not the case?

load end was firing from the remote.html frame even though the ad inside wasn't actually there yet, so we need to block on render start instead, to verify the ad actually exists before we show the sticky container.

zhouyx · 2019-03-27T01:53:54Z

extensions/amp-a4a/0.1/amp-a4a.js

+   * to trigger showing the sticky-ad container.
+   * @return {boolean}
+   */
+  ignoreLoadEndSignalForAmpStickyAd() {


I didn't see the function being used? Am I missing anything?

whoops sorry removed.

zhouyx · 2019-03-27T02:03:21Z

This fixes a bug where Adsense sticky ads on canonical amp running delayed fetch could show a sticky ad for a no-fill.

I felt sending 'no-content' signal is the right answer here. Are we having a race condition where iframe onload event fired before 'no-content'.

In any case, I think we should fix the issue on the ad level, not at the sticky-ad level.

bradfrizzell · 2019-03-27T15:32:56Z

I felt sending 'no-content' signal is the right answer here. Are we having a race condition where iframe onload event fired before 'no-content'.

My description wasn't thorough enough. We are not just concerned with a no-fill case, but also the fact that the sticky ad container shows before the ad is rendered when there is a filled ad. So the only way to handle this is to modify amp sticky ad to wait to show until it's received a message that the ad is rendered. If we instead wait for a no-content message, then we will show an empty sticky ad container which will then suddenly disappear.

zhouyx · 2019-03-28T01:48:07Z

@bradfrizzell Thank you for the explanation.

I found this line of code to be related

amphtml/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

Line 271 in 3477d89

return Promise.race([iframeLoadPromise, noContentPromise]);

I think the race condition here is really the render-start/no-content message arrives after the iframe 'load' event? How often do you think that will happen. Do you know how often that the render-start arrives after load event? Thanks.

I still don't like the idea of having amp-sticky-ad change its behavior based on its child. What about we have ads send a RENDER_START signal and have amp-sticky-ad always wait for that signal?

bradfrizzell · 2019-03-29T18:56:35Z

I am fine with getting rid of blocking on the load-end message entirely, but just want to raise the following concern. If an ad network wants to work with amp sticky ad, is it ok to require that implement the render-start message? Because as it is right now, my understanding is that the sticky ad will work with any ad tag loaded inside it, as the sticky container shows based on the browser load event firing on the ad frame, whereas render-start is a new api the network would have to add. What do you think?

bradfrizzell · 2019-04-04T15:31:37Z

Ping :)

zhouyx · 2019-04-04T17:32:52Z

Looking at the code again, here's one thing that I think it's unclear to me.

amphtml/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

Lines 260 to 268 in 9b8a19b

    
           Promise.race([ 
        
             renderStartPromise, 
        
             iframeLoadPromise, 
        
             timer.promise(VISIBILITY_TIMEOUT), 
        
           ]).then(() => { 
        
             if (this.iframe) { 
        
               setStyle(this.iframe, 'visibility', ''); 
        
             } 
        
           });

@dvoytenko Do you recall that why we want to toggle visibility at iframe onload? I vaguely remember that you ran some experiment and iframe onload always arrives after render-start?

zhouyx · 2019-04-04T17:45:03Z

I am fine with getting rid of blocking on the load-end message entirely, but just want to raise the following concern. If an ad network wants to work with amp sticky ad, is it ok to require that implement the render-start message? Because as it is right now, my understanding is that the sticky ad will work with any ad tag loaded inside it, as the sticky container shows based on the browser load event firing on the ad frame, whereas render-start is a new api the network would have to add. What do you think?

That's a good point. We definitely want to support all ad networks, even if they don't implement renderStart API. Currently we have a few signals. 1. render-start 2. iframe on load 3. visibility timeout 4.no-content (mutual exclusive of render-start)
we
display ad at: Promise.race([1, 2, 3]);
LOAD_END at: Promise.race([2, 4]);

At this change we want to display sticky-ad at 1 (if render-start is supported), 2 (if render-start not supported).

I think that makes perfect sense. But it leads to different behavior between sticky-ad and regular ad. I am leaning towards changing the behavior or regular ad. Can we apply the same behavior to regular ad as well?

bradfrizzell · 2019-04-08T18:20:58Z

is that change something that we'd need to block this PR on? It seems like that would be a larger scope than what I'm doing here. The change I made to the promise race is contained within amp-sticky-ad at this time.

bradfrizzell · 2019-04-23T11:11:40Z

@zhouyx ping on above

zhouyx · 2019-05-08T22:49:54Z

Synced with the team on this. + @lannka

RENDER_START signal is a component opt-in common signal that informs that the content is ready to be displayed. Currently only amp-ad and shadow doc is using it.
LOAD_END signal is a runtime common signal that fires at element's layoutCallback complete.
We decide that as a container element, ad-sticky-ad should not need to understand the implementation of the child amp-ad element. It should also waits for the RENDER_START/NO_CONTENT signal from the child amp-ad and toggle display based on that.

Today the RENDER_START COMMON SIGNAL from ad is implemented as an optional signal which equals to the render-start message. For cleaner code, we want to have the ad always fire a RENDER_START whenever we decide to toggle visibility. (This may require a small refactor to the amp-ad code)

@bradfrizzell In this way, we don't need to call ignoreLoadEndForAmpStickyAd(), instead we can have the sticky-ad waits for signals.whenSignal(CommonSignals.RENDER_START) only. Let me know if this sounds good to you? Thank you.

TODO to @zhouyx: Add more detailed comments to common signals, so that we don't need to review them from start again next time.

bradfrizzell · 2019-05-14T14:26:39Z

@zhouyx done, please take another look

zhouyx

Because we remove LOAD_END here, we want to make sure that all visible ad has signaled RENDER_START. Can we ensure that renderViaNameAttrOfXOriginIframe_, renderViaIframeGet_ and renderAmpCreative_, the three rendering methods have all signaled RENDER_START?

I took a quick look at the amp-a4a.js code. Here's what I found, please confirm
#renderAmpCreative_callsinstallFriendlyIframeEmbedwhere FIE always signalsRENDER_START renderViaNameAttrOfXOriginIframe_andrenderViaIframeGet_both use theAmpAdXOriginIframeHandlerwhich always signalsRENDER_START` after changes from #22305

extensions/amp-sticky-ad/1.0/amp-sticky-ad.js

bradfrizzell · 2019-05-15T17:25:34Z

Agree that renderViaNameAttr calls renderStart, as it uses iframeRenderHelper which then uses XOriginIframeHandler
RenderViaIFrameGet also uses this same call chain, so it uses renderStart
renderAmpCreative utilizes friendly-iframe-embed which calls startRender_ on itself, which triggers render-start

So yes I agree your assessment looks correct to me, render-start will be called in all cases.

zhouyx

One more request. Please help me understand why we need both the config change for delayed fetch and also adding support to fast fetch.
I think the PR is good to go after #22305 is merged.

ads/_config.js

zhouyx · 2019-05-20T20:12:50Z

FYI: #22305 is merged. Please let me know when the PR is ready. Thank you

zhouyx · 2019-06-18T22:02:48Z

@bradfrizzell Any update on the PR. Thanks

bradfrizzell · 2019-06-19T14:11:47Z

@zhouyx hey sorry this fell off my plate with some higher priority stuff. going to try to finish this up today / tomorrow

zhouyx · 2019-06-19T23:22:12Z

examples/adsense_sticky.amp.html

+  </amp-sticky-ad>
+
+
+  <!--


nit: Please remove.

zhouyx · 2019-06-19T23:22:21Z

examples/adsense_sticky.amp.html

+  <h2>AdSense</h2>
+
+
+ <!-- <amp-sticky-ad layout="nodisplay">


nit: remove

zhouyx · 2019-06-19T23:22:36Z

examples/adsense_sticky.amp.html

+</head>
+<body>
+
+  <amp-user-notification


nit: no need

zhouyx · 2019-06-19T23:23:34Z

examples/adsense_sticky.amp.html

@@ -0,0 +1,118 @@
+<!doctype html>


Do we need this new example file. Is it possible to move it to adsense.amp.html instead? Thank you

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

zhouyx · 2019-06-20T00:35:47Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+          event['source'] == this.iframe.contentWindow
+        ) {
+          this.renderStarted();
+          this.element.parentElement.setAttribute('visible', '');


I don't think you need to toggle visibility here. AMP sticky ad when it received render-start signal will toggle visibility. this.renderStarted() should have done the work.

zhouyx · 2019-06-20T00:41:13Z

extensions/amp-a4a/0.1/amp-a4a.js

   * @return {!Promise} awaiting load event for ad frame
   * @private
   */
-  iframeRenderHelper_(attributes) {
+  iframeRenderHelper_(attributes, letCreativeTriggerRenderStart) {


Do we need to pass in letCreativeTriggerRenderStart? I think we can call this.letCreativeTriggerRenderStart() instead.

good call, fixed

zhouyx · 2019-06-20T00:42:33Z

extensions/amp-a4a/0.1/amp-a4a.js

-      return this.iframeRenderHelper_(dict({'src': srcPath, 'name': name}));
+      return this.iframeRenderHelper_(
+        dict({'src': srcPath, 'name': name}),
+        false


I'm a bit confused why we need to set the letCreativeTriggerRenderStart value specifically to false here?

zhouyx · 2019-06-21T17:10:55Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+          event['source'] == this.iframe.contentWindow
+        ) {
+          this.renderStarted();
+          this.element.setAttribute('visible', '');


One last request: we set iframe visibility to hidden here
https://github.com/ampproject/amphtml/blob/master/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js#L278
Should also set iframe visibility instead of element visibility.

zhouyx

LGTM

…lemented. (ampproject#21157) * Modify amp sticky ad to only wait on render-start if render-start implemented. * Remove console log * Cleanup * Make it work for iframe get rendering canonical amp adsense * Respond to feedback * Remove all reliance on LOAD_END event * Update test * Fix syntax * Respond to feedback * respond to feedback * Fix getData * Add tests, respond to comment

Modify amp sticky ad to only wait on render-start if render-start imp…

09440b6

…lemented.

googlebot added the cla: yes label Feb 27, 2019

bradfrizzell added 2 commits February 27, 2019 14:50

Remove console log

377a30f

Cleanup

1018d99

keithwrightbos reviewed Mar 1, 2019

View reviewed changes

bradfrizzell requested a review from zhouyx March 13, 2019 18:47

keithwrightbos approved these changes Mar 13, 2019

View reviewed changes

Make it work for iframe get rendering canonical amp adsense

fa13913

bradfrizzell assigned zhouyx Mar 22, 2019

zhouyx suggested changes Mar 27, 2019

View reviewed changes

Respond to feedback

23de1f8

Remove all reliance on LOAD_END event

9250f09

Update test

7361b3e

zhouyx reviewed May 15, 2019

View reviewed changes

extensions/amp-sticky-ad/1.0/amp-sticky-ad.js Show resolved Hide resolved

zhouyx reviewed May 16, 2019

View reviewed changes

ads/_config.js Show resolved Hide resolved

bradfrizzell and others added 2 commits May 21, 2019 10:58

Merge branch 'master' into frizz-adsense

d6a232f

Fix syntax

a23d1b7

Respond to feedback

6d211da

zhouyx reviewed Jun 20, 2019

View reviewed changes

bradfrizzell added 2 commits June 20, 2019 13:47

respond to feedback

7f058fd

Fix getData

947c6aa

zhouyx reviewed Jun 21, 2019

View reviewed changes

Add tests, respond to comment

669a8f3

zhouyx approved these changes Jun 24, 2019

View reviewed changes

bradfrizzell merged commit c5a1acc into ampproject:master Jun 25, 2019

Modify amp sticky ad to only wait on render-start if render-start implemented. #21157

Modify amp sticky ad to only wait on render-start if render-start implemented. #21157

Conversation

bradfrizzell commented Feb 27, 2019

Choose a reason for hiding this comment

keithwrightbos 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

zhouyx commented Mar 27, 2019

bradfrizzell commented Mar 27, 2019

zhouyx commented Mar 28, 2019

bradfrizzell commented Mar 29, 2019

bradfrizzell commented Apr 4, 2019

zhouyx commented Apr 4, 2019

zhouyx commented Apr 4, 2019

bradfrizzell commented Apr 8, 2019

bradfrizzell commented Apr 23, 2019

zhouyx commented May 8, 2019

bradfrizzell commented May 14, 2019

zhouyx left a comment

Choose a reason for hiding this comment

bradfrizzell commented May 15, 2019

zhouyx left a comment • edited

Choose a reason for hiding this comment

zhouyx commented May 20, 2019

zhouyx commented Jun 18, 2019

bradfrizzell commented Jun 19, 2019

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

zhouyx left a comment

Choose a reason for hiding this comment

zhouyx left a comment •

edited