address feedback

ampproject · Jan 23, 2020 · 3a2c0c8 · 3a2c0c8
1 parent e9d27c8
commit 3a2c0c8
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 11 deletions.
diff --git a/extensions/amp-script/0.1/amp-script.js b/extensions/amp-script/0.1/amp-script.js
@@ -84,6 +84,9 @@ export class AmpScript extends AMP.BaseElement {
     /** @private {string} */
     this.debugId_ = 'amp-script[unknown].js';
 
+    /** @private {boolean} */
+    this.layoutCallbackHappened_ = false;
+
     /**
      * If true, most production constraints are disabled including script size,
      * script hash sum for local scripts, etc. Default is false.
@@ -138,6 +141,10 @@ export class AmpScript extends AMP.BaseElement {
    */
   onLayoutMeasure() {
     super.onLayoutMeasure();
+    if (this.layoutCallbackHappened_) {
+      return;
+    }
+
     const {width, height} = this.getLayoutBox();
     if (width === 0 && height === 0) {
       user().warn(
@@ -166,6 +173,7 @@ export class AmpScript extends AMP.BaseElement {
 
   /** @override */
   layoutCallback() {
+    this.layoutCallbackHappened_ = true;
     this.userActivation_ = new UserActivationTracker(this.element);
 
     // The displayed name of the combined script in dev tools.

diff --git a/extensions/amp-script/0.1/test/unit/test-amp-script.js b/extensions/amp-script/0.1/test/unit/test-amp-script.js
@@ -94,17 +94,35 @@ describes.fakeWin('AmpScript', {amp: {runtimeOn: false}}, env => {
     expect(service.checkSha384).to.be.called;
   });
 
-  it('should warn if there is zero width/height', () => {
-    const warnStub = env.sandbox.stub(user(), 'warn');
-    env.sandbox.stub(script, 'getLayoutBox').returns({height: 0, width: 0});
-    script.onLayoutMeasure();
-
-    expect(warnStub).calledWith(
-      'amp-script',
-      'Skipped initializing amp-script due to zero width and height.',
-      script.element
-    );
-    expect(warnStub).to.have.callCount(1);
+  describe('Initialization skipped warning due to zero height/width', () => {
+    it('should not warn when there is positive width/height', () => {
+      const warnStub = env.sandbox.stub(user(), 'warn');
+      env.sandbox.stub(script, 'getLayoutBox').returns({height: 1, width: 1});
+      script.onLayoutMeasure();
+      expect(warnStub).to.have.callCount(0);
+    });
+
+    it('should warn if there is zero width/height', () => {
+      const warnStub = env.sandbox.stub(user(), 'warn');
+      env.sandbox.stub(script, 'getLayoutBox').returns({height: 0, width: 0});
+      script.onLayoutMeasure();
+
+      expect(warnStub).calledWith(
+        'amp-script',
+        'Skipped initializing amp-script due to zero width and height.',
+        script.element
+      );
+      expect(warnStub).to.have.callCount(1);
+    });
+
+    it('should only warn if layoutCallback hasnt happened', () => {
+      const warnStub = env.sandbox.stub(user(), 'warn');
+      allowConsoleError(() => {
+        script.layoutCallback();
+      });
+      script.onLayoutMeasure();
+      expect(warnStub).to.have.callCount(0);
+    });
   });
 
   it('should fail on invalid sha384(author_js) for cross-origin src', () => {