ads: Fix how safeframe measures after expand (ampproject#14048)

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

@@ -1046,7 +1046,7 @@ export class AmpA4A extends AMP.BaseElement {
     // unlayoutCallback so that it is reverted to original size in case
     // of resumeCallback.
     this.originalSlotSize_ = this.originalSlotSize_ || this.getLayoutBox();
-    return super.attemptChangeSize(newHeight, newWidth).catch(() => {});
+    return super.attemptChangeSize(newHeight, newWidth);
   }
 
   /** @override  */

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

@@ -228,7 +228,7 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A {
       return this.attemptChangeSize(
           AmpAdNetworkAdsenseImpl.getResponsiveHeightForContext_(
               viewportSize),
-          viewportSize.width);
+          viewportSize.width).catch(() => {});
     }
   }
 

extensions/amp-ad-network-doubleclick-impl/0.1/safeframe-host.js

@@ -153,7 +153,7 @@ export class SafeframeHostApi {
     this.isFluid_ = isFluid;
 
     /** @private {?({width: number, height: number}|../../../src/layout-rect.LayoutRectDef)} */
-    this.initialSize_ = initialSize;
+    this.slotSize_ = initialSize;
 
     /** @private {?({width, height}|../../../src/layout-rect.LayoutRectDef)} */
     this.creativeSize_ = creativeSize;
@@ -489,23 +489,35 @@ export class SafeframeHostApi {
     if (this.isCollapsed_ || !this.isRegistered_) {
       return;
     }
-    this.handleSizeChange(this.initialSize_.height,
-        this.initialSize_.width,
+    this.handleSizeChange(this.creativeSize_.height,
+        this.creativeSize_.width,
         SERVICE.COLLAPSE_RESPONSE,
         /** isCollapse */ true);
   }
 
   /**
    * @param {number} height
    * @param {number} width
+   * @param {boolean} isCollapsed
+   * @param {string} messageType
    */
-  resizeIframe(height, width) {
-    if (this.iframe_) {
-      setStyles(this.iframe_, {
-        'height': height + 'px',
-        'width': width + 'px',
-      });
-    }
+  resizeSafeframe(height, width, isCollapsed, messageType) {
+    this.isCollapsed_ = isCollapsed;
+    this.baseInstance_.measureMutateElement(
+        /** MEASURER */ () => {
+          this.baseInstance_.getResource().measure();
+        },
+        /** MUTATOR */ () => {
+          if (this.iframe_) {
+            setStyles(this.iframe_, {
+              'height': height + 'px',
+              'width': width + 'px',
+            });
+          }
+          this.sendResizeResponse(/** SUCCESS */ true, messageType);
+        },
+        this.iframe_
+    );
   }
 
   /**
@@ -527,11 +539,9 @@ export class SafeframeHostApi {
    */
   handleSizeChange(height, width, messageType, optIsCollapse) {
     if (!optIsCollapse &&
-        width <= this.initialSize_.width &&
-        height <= this.initialSize_.height) {
-      this.resizeIframe(height, width);
-      this.isCollapsed_ = !!optIsCollapse;
-      this.sendResizeResponse(/** SUCCESS */ true, messageType);
+        width <= this.slotSize_.width &&
+        height <= this.slotSize_.height) {
+      this.resizeSafeframe(height, width, !!optIsCollapse, messageType);
     } else {
       this.resizeAmpAdAndSafeframe(
           height, width, messageType, optIsCollapse);
@@ -562,45 +572,48 @@ export class SafeframeHostApi {
   }
 
   /**
-   *
+   * Attempts to resize both the amp-ad and the Safeframe.
+   * If the amp-ad can not be resized, then if it was a collapse request,
+   * we will still collapse just the safeframe.
    * @param {number} height
    * @param {number} width
    * @param {string} messageType
    * @param {boolean=} optIsCollapse
    */
   resizeAmpAdAndSafeframe(height, width, messageType, optIsCollapse) {
+    // First, attempt to resize the Amp-Ad that is the parent of the
+    // safeframe
     this.baseInstance_.attemptChangeSize(height, width).then(() => {
-      const success = !!this.baseInstance_.element.style.height.match(height)
-              && !!this.baseInstance_.element.style.width.match(width);
-      // If the amp-ad element was successfully resized, always update
-      // the size of the safeframe as well. If the amp-ad element could not
-      // be resized, but this is a collapse request, then only collapse
-      // the safeframe.
-      if (success || optIsCollapse) {
-        this.resizeIframe(height, width);
-        this.isCollapsed_ = !!optIsCollapse;
-        this.baseInstance_.element.getResources().resources_.forEach(
-            resource => {
-              if (resource.element == this.baseInstance_.element) {
-                // Need to force a measure event, as measure won't happen immediately
-                // if the element was above the viewport when resize occured, and
-                // without a measure, we'll send the wrong size for the creative
-                // on the geometry update message.
-                resource.measure();
-              }
-            });
+      // If this resize succeeded, we always resize the safeframe.
+      // resizeSafeframe also sends the resize response.
+      this.resizeSafeframe(height, width, !!optIsCollapse, messageType);
+      // Update our stored record of what the amp-ad's size is. This
+      // is just for caching. Setting it here doesn't actually change
+      // the size of the amp-ad, the attempt change size above did that.
+      this.slotSize_.height = height;
+      this.slotSize_.width = width;
+    }, /** REJECT CALLBACK */ () => {
+      // If the resize initially failed, it may have been queued
+      // as a pendingChangeSize, which will cause the size change
+      // to execute upon the next user interaction. We don't want
+      // that for safeframe, so we reset it here.
+      this.baseInstance_.getResource().resetPendingChangeSize();
+      if (optIsCollapse) {
+        // If this is a collapse request, then even if resizing
+        // the amp-ad failed, still resize the iframe.
+        // resizeSafeframe also sends the resize response.
+        this.resizeSafeframe(height, width, !!optIsCollapse, messageType);
       } else {
-        // attemptChangeSize automatically registers a pendingChangeSize if
-        // the initial attempt failed. We do not want to do that, so clear it.
-        this.baseInstance_.element.getResources().resources_.forEach(
-            resource => {
-              if (resource.element == this.baseInstance_.element) {
-                resource.pendingChangeSize_ = undefined;
-              }
-            });
+        // If this is not a collapse request, then we were attempting to
+        // expand past the bounds of the amp-ad, and it failed. Thus,
+        // we need to send a failure message, and the safeframe is
+        // not resized.
+        this.sendResizeResponse(false, messageType);
       }
-      this.sendResizeResponse(success || !!optIsCollapse, messageType);
-    }).catch(() => {});
+    }).catch(err => {
+      dev().error(TAG, `Resizing failed: ${err}`);
+      this.sendResizeResponse(false, messageType);
+    });
   }
 
   /**

extensions/amp-ad-network-doubleclick-impl/0.1/test/test-doubleclick-safeframe.js

@@ -447,7 +447,7 @@ describes.realWin('DoubleClick Fast Fetch - Safeframe', realWinConfig, env => {
 
   describe('Resizing', () => {
     let safeframeMock;
-    let resizeIframeSpy;
+    let resizeSafeframeSpy;
     let sendResizeResponseSpy;
     let resizeAmpAdAndSafeframeSpy;
     let attemptChangeSizeStub;
@@ -465,8 +465,8 @@ describes.realWin('DoubleClick Fast Fetch - Safeframe', realWinConfig, env => {
       });
       ampAd.appendChild(safeframeMock);
       doubleclickImpl.iframe = safeframeMock;
-      resizeIframeSpy = sandbox.spy(
-          safeframeHost, 'resizeIframe');
+      resizeSafeframeSpy = sandbox.spy(
+          safeframeHost, 'resizeSafeframe');
       sendResizeResponseSpy = sandbox.spy(
           safeframeHost, 'sendResizeResponse');
       resizeAmpAdAndSafeframeSpy = sandbox.spy(
@@ -512,17 +512,19 @@ describes.realWin('DoubleClick Fast Fetch - Safeframe', realWinConfig, env => {
       sendExpandMessage(50, 50);
       // Verify that we can immediately resize the safeframe, and don't
       // need to call any of the fancy AMP element resize things.
-      expect(resizeIframeSpy).to.be.calledOnce;
-      expect(resizeIframeSpy).to.be.calledWith(100, 100);
-      expect(safeframeMock.style.height).to.equal('100px');
-      expect(safeframeMock.style.width).to.equal('100px');
-      expect(sendResizeResponseSpy).to.be.calledWith(
-          true, SERVICE.EXPAND_RESPONSE);
-      expect(resizeAmpAdAndSafeframeSpy).to.not.be.called;
+      return Services.timerFor(env.win).promise(100).then(() => {
+        expect(resizeSafeframeSpy).to.be.calledOnce;
+        expect(resizeSafeframeSpy).to.be.calledWith(100, 100);
+        expect(safeframeMock.style.height).to.equal('100px');
+        expect(safeframeMock.style.width).to.equal('100px');
+        expect(sendResizeResponseSpy).to.be.calledWith(
+            true, SERVICE.EXPAND_RESPONSE);
+        expect(resizeAmpAdAndSafeframeSpy).to.not.be.called;
+      });
     });
 
     /**
-     * If the safeframed creative asks to resize within the bounds of
+     * If the safeframed creative asks to resize outside the bounds of
      * the amp-ad element, first we try to resize the amp-ad element by
      * using element.attemptChangeSize. If that succeeds, then we also
      * resize the safeframe.
@@ -535,18 +537,35 @@ describes.realWin('DoubleClick Fast Fetch - Safeframe', realWinConfig, env => {
         ampAd.style.height = '600px';
         ampAd.style.width = '600px';
         f();
-        return {'catch': f => f()};
+        return {'catch': () => {}};
       };
       attemptChangeSizeStub.returns({then});
       sendExpandMessage(550,550);
 
-      expect(resizeIframeSpy).to.be.calledOnce;
-      expect(resizeIframeSpy).to.be.calledWith(600, 600);
-      expect(safeframeMock.style.height).to.equal('600px');
-      expect(safeframeMock.style.width).to.equal('600px');
-      expect(sendResizeResponseSpy).to.be.calledWith(
-          true, SERVICE.EXPAND_RESPONSE);
-      expect(resizeAmpAdAndSafeframeSpy).to.be.calledOnce;
+      return Services.timerFor(env.win).promise(100).then(() => {
+        expect(resizeSafeframeSpy).to.be.calledOnce;
+        expect(resizeSafeframeSpy).to.be.calledWith(600, 600);
+        expect(safeframeMock.style.height).to.equal('600px');
+        expect(safeframeMock.style.width).to.equal('600px');
+        expect(sendResizeResponseSpy).to.be.calledWith(
+            true, SERVICE.EXPAND_RESPONSE);
+        expect(resizeAmpAdAndSafeframeSpy).to.be.calledOnce;
+      });
+    });
+
+    /**
+     * If the safeframed creative asks to resize outside the bounds of
+     * the amp-ad element, first we try to resize the amp-ad element by
+     * using element.attemptChangeSize. If that rejects, we should send
+     * a failure message.
+     */
+    it('resizeAmpAdAndSafeframe should send error on rejection', () => {
+      attemptChangeSizeStub.rejects();
+      safeframeHost.resizeAmpAdAndSafeframe(550, 550, SERVICE.EXPAND_RESPONSE);
+      return Services.timerFor(env.win).promise(100).then(() => {
+        expect(sendResizeResponseSpy).to.be.calledWith(
+            false, SERVICE.EXPAND_RESPONSE);
+      });
     });
 
     /**
@@ -556,23 +575,18 @@ describes.realWin('DoubleClick Fast Fetch - Safeframe', realWinConfig, env => {
      */
     it('expand_request should fail if expanding past amp-ad bounds and would ' +
        'create reflow', () => {
-      // Sneaky hack to do a synchronous mock of attemptChangeSize
-      // In our mock, we don't do anything to the iframe that would be seen as a
-      // success, thus we are failing the resizing.
-      const then = f => {
-        f();
-        return {'catch': f => f()};
-      };
-      attemptChangeSizeStub.returns({then});
+      attemptChangeSizeStub.rejects();
 
       sendExpandMessage(550, 550);
 
-      expect(resizeIframeSpy).to.not.be.called;
-      expect(safeframeMock.height).to.equal('50');
-      expect(safeframeMock.width).to.equal('50');
-      expect(sendResizeResponseSpy).to.be.calledWith(
-          false, SERVICE.EXPAND_RESPONSE);
-      expect(resizeAmpAdAndSafeframeSpy).to.be.calledOnce;
+      return Services.timerFor(env.win).promise(100).then(() => {
+        expect(resizeSafeframeSpy).to.not.be.called;
+        expect(safeframeMock.height).to.equal('50');
+        expect(safeframeMock.width).to.equal('50');
+        expect(sendResizeResponseSpy).to.be.calledWith(
+            false, SERVICE.EXPAND_RESPONSE);
+        expect(resizeAmpAdAndSafeframeSpy).to.be.calledOnce;
+      });
     });
 
     function sendCollapseMessage() {
@@ -594,22 +608,18 @@ describes.realWin('DoubleClick Fast Fetch - Safeframe', realWinConfig, env => {
       ampAd.style.width = '600px';
       safeframeMock.style.height = 600;
       safeframeMock.style.width = 600;
-      // Sneaky hack to do a synchronous mock of attemptChangeSize
-      // Resize the ampAd to simulate a success.
-      const then = f => {
-        f();
-        return {'catch': f => f()};
-      };
-      attemptChangeSizeStub.returns({then});
+      attemptChangeSizeStub.rejects();
       sendCollapseMessage();
 
-      expect(resizeIframeSpy).to.be.calledOnce;
-      expect(resizeIframeSpy).to.be.calledWith(250, 300);
-      expect(safeframeMock.style.height).to.equal('250px');
-      expect(safeframeMock.style.width).to.equal('300px');
-      expect(sendResizeResponseSpy).to.be.calledWith(
-          true, SERVICE.COLLAPSE_RESPONSE);
-      expect(resizeAmpAdAndSafeframeSpy).to.be.calledOnce;
+      return Services.timerFor(env.win).promise(100).then(() => {
+        expect(resizeSafeframeSpy).to.be.calledOnce;
+        expect(resizeSafeframeSpy).to.be.calledWith(250, 300);
+        expect(safeframeMock.style.height).to.equal('250px');
+        expect(safeframeMock.style.width).to.equal('300px');
+        expect(sendResizeResponseSpy).to.be.calledWith(
+            true, SERVICE.COLLAPSE_RESPONSE);
+        expect(resizeAmpAdAndSafeframeSpy).to.be.calledOnce;
+      });
     });
 
     it('should collapse safeframe on amp-ad resize success', () => {
@@ -624,18 +634,20 @@ describes.realWin('DoubleClick Fast Fetch - Safeframe', realWinConfig, env => {
         ampAd.style.height = '250px';
         ampAd.style.width = '300px';
         f();
-        return {'catch': f => f()};
+        return {'catch': () => {}};
       };
       attemptChangeSizeStub.returns({then});
       sendCollapseMessage();
 
-      expect(resizeIframeSpy).to.be.calledOnce;
-      expect(resizeIframeSpy).to.be.calledWith(250, 300);
-      expect(safeframeMock.style.height).to.equal('250px');
-      expect(safeframeMock.style.width).to.equal('300px');
-      expect(sendResizeResponseSpy).to.be.calledWith(
-          true, SERVICE.COLLAPSE_RESPONSE);
-      expect(resizeAmpAdAndSafeframeSpy).to.be.calledOnce;
+      return Services.timerFor(env.win).promise(100).then(() => {
+        expect(resizeSafeframeSpy).to.be.calledOnce;
+        expect(resizeSafeframeSpy).to.be.calledWith(250, 300);
+        expect(safeframeMock.style.height).to.equal('250px');
+        expect(safeframeMock.style.width).to.equal('300px');
+        expect(sendResizeResponseSpy).to.be.calledWith(
+            true, SERVICE.COLLAPSE_RESPONSE);
+        expect(resizeAmpAdAndSafeframeSpy).to.be.calledOnce;
+      });
     });
   });
 });