Remove onLayoutMeasure and getLayoutSize (#32471)

* Remove onLayoutMeasure and getLayoutSize * cleanup * more tests, revert sidebar * fix tests * cleanup * debugging tests * revert test case * review fixes
ampproject · Feb 9, 2021 · 2ca31c4 · 2ca31c4
1 parent fd5fb3f
commit 2ca31c4
Show file tree

Hide file tree

Showing 9 changed files with 243 additions and 48 deletions.
diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js
@@ -1182,6 +1182,43 @@ const forbiddenTermsSrcInclusive = {
       'extensions/amp-story/1.0/page-advancement.js',
     ],
   },
+  '\\.getLayoutSize': {
+    message: measurementApiDeprecated,
+    allowlist: [
+      'builtins/amp-img.js',
+      'src/base-element.js',
+      'src/custom-element.js',
+      'src/iframe-helper.js',
+      'src/service/mutator-impl.js',
+      'src/service/resources-impl.js',
+      'src/service/video-manager-impl.js',
+      'extensions/amp-a4a/0.1/amp-a4a.js',
+      'extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js',
+      'extensions/amp-fx-flying-carpet/0.1/amp-fx-flying-carpet.js',
+      'extensions/amp-script/0.1/amp-script.js',
+      'extensions/amp-story/1.0/amp-story-page.js',
+    ],
+  },
+  'onLayoutMeasure': {
+    message: measurementApiDeprecated,
+    allowlist: [
+      'src/base-element.js',
+      'src/custom-element.js',
+      'extensions/amp-a4a/0.1/amp-a4a.js',
+      'extensions/amp-a4a/0.1/amp-ad-network-base.js',
+      'extensions/amp-ad/0.1/amp-ad-3p-impl.js',
+      'extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js',
+      'extensions/amp-ad-exit/0.1/amp-ad-exit.js',
+      'extensions/amp-ad-exit/0.1/filters/click-location.js',
+      'extensions/amp-ad-exit/0.1/filters/filter.js',
+      'extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js',
+      'extensions/amp-iframe/0.1/amp-iframe.js',
+      'extensions/amp-script/0.1/amp-script.js',
+      'extensions/amp-sidebar/0.1/amp-sidebar.js',
+      'extensions/amp-sidebar/0.2/amp-sidebar.js',
+      'extensions/amp-story/1.0/amp-story-page.js',
+    ],
+  },
   '\\.getIntersectionElementLayoutBox': {
     message: measurementApiDeprecated,
     allowlist: [

diff --git a/extensions/amp-3d-gltf/0.1/amp-3d-gltf.js b/extensions/amp-3d-gltf/0.1/amp-3d-gltf.js
@@ -22,6 +22,10 @@ import {dict} from '../../../src/utils/object';
 import {getIframe, preloadBootstrap} from '../../../src/3p-frame';
 import {isLayoutSizeDefined} from '../../../src/layout';
 import {listenFor, postMessage} from '../../../src/iframe-helper';
+import {
+  observeContentSize,
+  unobserveContentSize,
+} from '../../../src/utils/size-observer';
 import {
   observeWithSharedInOb,
   unobserveWithSharedInOb,
@@ -56,6 +60,8 @@ export class Amp3dGltf extends AMP.BaseElement {
 
     /** @private {?Function} */
     this.unlistenMessage_ = null;
+
+    this.onResized_ = this.onResized_.bind(this);
   }
 
   /**
@@ -97,6 +103,7 @@ export class Amp3dGltf extends AMP.BaseElement {
     this.willBeReady_ = new Deferred();
     this.willBeLoaded_ = new Deferred();
 
+    unobserveContentSize(this.element, this.onResized_);
     return true;
   }
 
@@ -167,6 +174,8 @@ export class Amp3dGltf extends AMP.BaseElement {
 
     this.element.appendChild(this.iframe_);
 
+    observeContentSize(this.element, this.onResized_);
+
     return this.willBeLoaded_.promise;
   }
 
@@ -242,10 +251,10 @@ export class Amp3dGltf extends AMP.BaseElement {
 
   /**
    * Sends `setSize` command when ready
-   *
+   * @param {!../layout-rect.LayoutSizeDef} size
+   * @private
    */
-  onLayoutMeasure() {
-    const {width, height} = this.getLayoutSize();
+  onResized_({width, height}) {
     this.sendCommandWhenReady_(
       'setSize',
       dict({'width': width, 'height': height})

diff --git a/extensions/amp-carousel/0.1/slidescroll.js b/extensions/amp-carousel/0.1/slidescroll.js
@@ -32,6 +32,10 @@ import {isExperimentOn} from '../../../src/experiments';
 import {isFiniteNumber} from '../../../src/types';
 import {isLayoutSizeDefined} from '../../../src/layout';
 import {numeric} from '../../../src/transition';
+import {
+  observeContentSize,
+  unobserveContentSize,
+} from '../../../src/utils/size-observer';
 import {
   observeWithSharedInOb,
   unobserveWithSharedInOb,
@@ -141,6 +145,8 @@ export class AmpSlideScroll extends BaseSlides {
       : this.isIos_
       ? false
       : !isExperimentOn(this.win, 'amp-carousel-chrome-scroll-snap');
+
+    this.onResized_ = this.onResized_.bind(this);
   }
 
   /** @override */
@@ -246,6 +252,16 @@ export class AmpSlideScroll extends BaseSlides {
     );
   }
 
+  /** @override */
+  attachedCallback() {
+    observeContentSize(this.element, this.onResized_);
+  }
+
+  /** @override */
+  detachedCallback() {
+    unobserveContentSize(this.element, this.onResized_);
+  }
+
   /** @override */
   isLoopingEligible() {
     return this.noOfSlides_ > 1;
@@ -309,9 +325,12 @@ export class AmpSlideScroll extends BaseSlides {
     this.waitForScrollSettled_(timeout);
   }
 
-  /** @override */
-  onLayoutMeasure() {
-    this.slideWidth_ = this.element./*OK*/ offsetWidth;
+  /**
+   * @param {!../layout-rect.LayoutSizeDef} size
+   * @private
+   */
+  onResized_(size) {
+    this.slideWidth_ = size.width;
   }
 
   /** @override */

diff --git a/extensions/amp-carousel/0.1/test/test-slidescroll.js b/extensions/amp-carousel/0.1/test/test-slidescroll.js
@@ -22,25 +22,27 @@ import {
   createElementWithAttributes,
   whenUpgradedToCustomElement,
 } from '../../../../src/dom';
+import {installResizeObserverStub} from '../../../../testing/resize-observer-stub';
 import {user} from '../../../../src/log';
 
 describes.realWin(
   'SlideScroll',
   {
     amp: {
       extensions: ['amp-carousel'],
-      runtimeOn: true,
     },
   },
   (env) => {
     const SHOW_CLASS = 'i-amphtml-slide-item-show';
     let win, doc;
+    let resizeObserverStub;
 
     beforeEach(() => {
       win = env.win;
       doc = win.document;
       env.iframe.width = '1000';
       env.iframe.height = '1000';
+      resizeObserverStub = installResizeObserverStub(env.sandbox, win);
     });
 
     function getAmpSlideScroll(
@@ -88,11 +90,9 @@ describes.realWin(
         return ampSlideScroll
           .buildInternal()
           .then(() => {
-            ampSlideScroll.updateLayoutBox({
-              top: 0,
-              left: 0,
-              width: 400,
-              height: 300,
+            resizeObserverStub.notifySync({
+              target: ampSlideScroll,
+              contentRect: {width: 400, height: 300},
             });
             return ampSlideScroll.layoutCallback();
           })
@@ -101,6 +101,18 @@ describes.realWin(
       return Promise.resolve(ampSlideScroll);
     }
 
+    /**
+     * @param {Element} element
+     * @returns {boolean}
+     */
+    function isScreenReaderHidden(element) {
+      const computedStyle = getComputedStyle(element);
+      return (
+        computedStyle.visibility === 'hidden' ||
+        computedStyle.display === 'none'
+      );
+    }
+
     it('should create container and wrappers and show initial slides', async () => {
       const ampSlideScroll = await getAmpSlideScroll();
       const impl = await ampSlideScroll.getImpl();
@@ -627,19 +639,21 @@ describes.realWin(
       const ampSlideScroll = await getAmpSlideScroll();
       const impl = await ampSlideScroll.getImpl();
 
-      const offsetWidthStub = env.sandbox.stub(ampSlideScroll, 'offsetWidth');
-
-      offsetWidthStub.value(200);
-      impl.onLayoutMeasure();
+      resizeObserverStub.notifySync({
+        target: ampSlideScroll,
+        contentRect: {width: 200, height: 400},
+      });
       expect(impl.slideWidth_).to.equal(200);
 
       // Show the first slide, make sure the scroll position is correct.
       impl.showSlide_(1);
       expect(impl.slidesContainer_./*OK*/ scrollLeft).to.equal(200);
 
       // Now do a layout measure letting the component know it changed size.
-      offsetWidthStub.value(400);
-      impl.onLayoutMeasure();
+      resizeObserverStub.notifySync({
+        target: ampSlideScroll,
+        contentRect: {width: 400, height: 200},
+      });
       expect(impl.slideWidth_).to.equal(400);
       expect(impl.slidesContainer_./*OK*/ scrollLeft).to.equal(200);
 
@@ -1280,7 +1294,6 @@ describes.realWin(
         impl.mutatedAttributesCallback({slide: 2});
         expect(showSlideSpy).to.not.have.been.called;
 
-        impl.onLayoutMeasure();
         ampSlideScroll.layoutCallback();
 
         // Should show the last slide index requested before layout.
@@ -1304,7 +1317,6 @@ describes.realWin(
         expect(showSlideSpy).to.not.have.been.called;
 
         // Test that showSlide_ is called after layout.
-        impl.onLayoutMeasure();
         ampSlideScroll.layoutCallback();
 
         expect(showSlideSpy).to.have.been.calledWith(1);
@@ -1321,7 +1333,6 @@ describes.realWin(
         expect(showSlideSpy).to.not.have.been.called;
 
         // Test that showSlide_ is called after layout.
-        impl.onLayoutMeasure();
         ampSlideScroll.layoutCallback();
 
         expect(showSlideSpy).to.have.been.calledWith(4);
@@ -1382,7 +1393,18 @@ describes.realWin(
         });
       });
     });
+  }
+);
 
+describes.realWin(
+  'SlideScroll with runtimeOn',
+  {
+    amp: {
+      extensions: ['amp-carousel'],
+      runtimeOn: true,
+    },
+  },
+  (env) => {
     it('should allow default actions in email documents', async () => {
       env.win.document.documentElement.setAttribute('amp4email', '');
       const action = new ActionService(env.ampdoc, env.win.document);
@@ -1441,15 +1463,3 @@ describes.realWin(
     });
   }
 );
-
-/**
- *
- * @param {Element} element
- * @returns {boolean}
- */
-function isScreenReaderHidden(element) {
-  const computedStyle = getComputedStyle(element);
-  return (
-    computedStyle.visibility === 'hidden' || computedStyle.display === 'none'
-  );
-}
diff --git a/extensions/amp-connatix-player/0.1/amp-connatix-player.js b/extensions/amp-connatix-player/0.1/amp-connatix-player.js
@@ -30,6 +30,10 @@ import {
 } from '../../../src/consent';
 import {getData} from '../../../src/event-helper';
 import {isLayoutSizeDefined} from '../../../src/layout';
+import {
+  observeContentSize,
+  unobserveContentSize,
+} from '../../../src/utils/size-observer';
 import {removeElement} from '../../../src/dom';
 import {setIsMediaComponent} from '../../../src/video-interface';
 import {tryParseJson} from '../../../src/json';
@@ -88,6 +92,8 @@ export class AmpConnatixPlayer extends AMP.BaseElement {
 
     /** @private {?Function} */
     this.playerReadyResolver_ = null;
+
+    this.onResized_ = this.onResized_.bind(this);
   }
 
   /**
@@ -293,6 +299,8 @@ export class AmpConnatixPlayer extends AMP.BaseElement {
     // bind to amp consent and send consent info to the iframe content and propagate to player
     this.bindToAmpConsent_();
 
+    observeContentSize(this.element, this.onResized_);
+
     return this.loadPromise(iframe).then(() => this.playerReadyPromise_);
   }
 
@@ -301,12 +309,14 @@ export class AmpConnatixPlayer extends AMP.BaseElement {
     return isLayoutSizeDefined(layout);
   }
 
-  /** @override */
-  onLayoutMeasure() {
+  /**
+   * @param {!../layout-rect.LayoutSizeDef} size
+   * @private
+   */
+  onResized_({width, height}) {
     if (!this.iframe_) {
       return;
     }
-    const {width, height} = this.getLayoutSize();
     this.sendCommand_('ampResize', {'width': width, 'height': height});
   }
 
@@ -323,6 +333,8 @@ export class AmpConnatixPlayer extends AMP.BaseElement {
     this.playerReadyPromise_ = deferred.promise;
     this.playerReadyResolver_ = deferred.resolve;
 
+    unobserveContentSize(this.element, this.onResized_);
+
     return true;
   }
 }