Move the iframe's box, not the wrapping AMP Element's

extensions/amp-ad/0.1/amp-ad-3p-impl.js

@@ -26,7 +26,7 @@ import {isAdPositionAllowed, getAdContainer,}
     from '../../../src/ad-helper';
 import {adConfig} from '../../../ads/_config';
 import {getLifecycleReporter} from '../../../ads/google/a4a/performance';
-import {user} from '../../../src/log';
+import {user, dev} from '../../../src/log';
 import {getIframe} from '../../../src/3p-frame';
 import {setupA2AListener} from './a2a-listener';
 import {moveLayoutRect} from '../../../src/layout-rect';
@@ -184,6 +184,9 @@ export class AmpAd3PImpl extends AMP.BaseElement {
       const iframeBox =
           this.getViewport().getLayoutRect(this.xOriginIframeHandler_.iframe);
       const box = this.getLayoutBox();
+      // Cache the iframe's relative position to the amp-ad. This is
+      // necessary for fixed-position containers which "move" with the
+      // viewport.
       this.iframeLayoutBox_ = moveLayoutRect(iframeBox, -box.left, -box.top);
     }
   }
@@ -199,9 +202,10 @@ export class AmpAd3PImpl extends AMP.BaseElement {
     if (!this.iframeLayoutBox_) {
       this.measureIframeLayoutBox_();
     }
-    // If the iframe is full size, we avoid an object allocation by moving box.
-    return moveLayoutRect(box, this.iframeLayoutBox_.left,
-        this.iframeLayoutBox_.top);
+
+    const iframe = /** @type {!../../../src/layout-rect.LayoutRectDef} */(
+        dev().assert(this.iframeLayoutBox_));
+    return moveLayoutRect(iframe, box.left, box.top);
   }
 
   /** @override */

extensions/amp-ad/0.1/test/test-amp-ad-3p-impl.js

@@ -218,4 +218,42 @@ describe('amp-ad-3p-impl', () => {
       expect(ad3p2.renderOutsideViewport()).to.equal(3);
     });
   });
+
+  describe('#getIntersectionElementLayoutBox', () => {
+    it('should not cache intersection box', () => {
+      return ad3p.layoutCallback().then(() => {
+        const iframe = ad3p.element.firstChild;
+
+        // Force some styles on the iframe, to display it without loading
+        // the iframe and have different size than the ad itself.
+        iframe.style.width = '300px';
+        iframe.style.height = '200px';
+        iframe.style.display = 'block';
+
+        const stub = sandbox.stub(ad3p, 'getLayoutBox');
+        const box = {
+          top: 100,
+          bottom: 200,
+          left: 0,
+          right: 100,
+          width: 100,
+          height: 100,
+        };
+        stub.returns(box);
+
+        ad3p.onLayoutMeasure();
+        const intersection = ad3p.getIntersectionElementLayoutBox();
+
+        // Simulate a fixed position element "moving" 100px by scrolling down
+        // the page.
+        box.top += 100;
+        box.bottom += 100;
+        const newIntersection = ad3p.getIntersectionElementLayoutBox();
+        expect(newIntersection).not.to.deep.equal(intersection);
+        expect(newIntersection.top).to.equal(intersection.top + 100);
+        expect(newIntersection.width).to.equal(300);
+        expect(newIntersection.height).to.equal(200);
+      });
+    });
+  });
 });

extensions/amp-iframe/0.1/amp-iframe.js

@@ -23,7 +23,7 @@ import {listenFor} from '../../../src/iframe-helper';
 import {parseUrl} from '../../../src/url';
 import {removeElement} from '../../../src/dom';
 import {timerFor} from '../../../src/timer';
-import {user} from '../../../src/log';
+import {user, dev} from '../../../src/log';
 import {utf8EncodeSync} from '../../../src/utils/bytes.js';
 import {urls} from '../../../src/config';
 import {moveLayoutRect} from '../../../src/layout-rect';
@@ -198,7 +198,7 @@ export class AmpIframe extends AMP.BaseElement {
     // free now and it might have changed.
     this.measureIframeLayoutBox_();
 
-    this.isAdLike_ = isAdLike(this);
+    this.isAdLike_ = isAdLike(this.element);
     this.isTrackingFrame_ = this.looksLikeTrackingIframe_();
     this.isDisallowedAsAd_ = this.isAdLike_ &&
         !isAdPositionAllowed(this.element, this.win);
@@ -219,6 +219,9 @@ export class AmpIframe extends AMP.BaseElement {
     if (this.iframe_) {
       const iframeBox = this.getViewport().getLayoutRect(this.iframe_);
       const box = this.getLayoutBox();
+      // Cache the iframe's relative position to the amp-iframe. This is
+      // necessary for fixed-position containers which "move" with the
+      // viewport.
       this.iframeLayoutBox_ = moveLayoutRect(iframeBox, -box.left, -box.top);
     }
   }
@@ -234,9 +237,10 @@ export class AmpIframe extends AMP.BaseElement {
     if (!this.iframeLayoutBox_) {
       this.measureIframeLayoutBox_();
     }
-    // If the iframe is full size, we avoid an object allocation by moving box.
-    return moveLayoutRect(box, this.iframeLayoutBox_.left,
-        this.iframeLayoutBox_.top);
+
+    const iframe = /** @type {!../../../src/layout-rect.LayoutRectDef} */(
+        dev().assert(this.iframeLayoutBox_));
+    return moveLayoutRect(iframe, box.left, box.top);
   }
 
   /** @override */
@@ -512,12 +516,12 @@ const adSizes = [[300, 250], [320, 50], [300, 50], [320, 100]];
 
 /**
  * Guess whether this element might be an ad.
- * @param {!BaseElement} ampElement An amp-iframe element.
+ * @param {!Element} element An amp-iframe element.
  * @return {boolean}
  * @visibleForTesting
  */
-export function isAdLike(ampElement) {
-  const box = ampElement.getIntersectionElementLayoutBox();
+export function isAdLike(element) {
+  const box = element.getLayoutBox();
   const height = box.height;
   const width = box.width;
   for (let i = 0; i < adSizes.length; i++) {

extensions/amp-iframe/0.1/test/test-amp-iframe.js

@@ -564,7 +564,7 @@ describe('amp-iframe', () => {
   it('should correctly classify ads', () => {
     function e(width, height) {
       return {
-        getIntersectionElementLayoutBox() {
+        getLayoutBox() {
           return {width, height};
         },
       };
@@ -590,4 +590,37 @@ describe('amp-iframe', () => {
       expect(error.message).to.match(/not used for displaying fixed ad/);
     });
   });
+
+  it('should not cache intersection box', () => {
+    return getAmpIframeObject({
+      src: iframeSrc,
+      sandbox: 'allow-scripts allow-same-origin',
+      width: 300,
+      height: 250,
+    }).then(impl => {
+      const stub = sandbox.stub(impl, 'getLayoutBox');
+      const box = {
+        top: 100,
+        bottom: 200,
+        left: 0,
+        right: 100,
+        width: 100,
+        height: 100,
+      };
+      stub.returns(box);
+
+      impl.onLayoutMeasure();
+      const intersection = impl.getIntersectionElementLayoutBox();
+
+      // Simulate a fixed position element "moving" 100px by scrolling down
+      // the page.
+      box.top += 100;
+      box.bottom += 100;
+      const newIntersection = impl.getIntersectionElementLayoutBox();
+      expect(newIntersection).not.to.deep.equal(intersection);
+      expect(newIntersection.top).to.equal(intersection.top + 100);
+      expect(newIntersection.width).to.equal(300);
+      expect(newIntersection.height).to.equal(250);
+    });
+  });
 });