Skip to content

Commit

Permalink
♻️ amp-script: emit warning if zero height and width. (#26444)
Browse files Browse the repository at this point in the history
* Emit warning if attempting to load a 0 height/width amp-script

* address feedback

* improve name for layoutCallbackHappened --> layoutCompleted_

* jridgewell@ nit

* anotha day anotha nit
  • Loading branch information
samouri authored Jan 23, 2020
1 parent 553061d commit ae8156f
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 0 deletions.
23 changes: 23 additions & 0 deletions extensions/amp-script/0.1/amp-script.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ export class AmpScript extends AMP.BaseElement {
/** @private {string} */
this.debugId_ = 'amp-script[unknown].js';

/** @private {boolean} */
this.layoutCompleted_ = false;

/**
* If true, most production constraints are disabled including script size,
* script hash sum for local scripts, etc. Default is false.
Expand Down Expand Up @@ -133,6 +136,24 @@ export class AmpScript extends AMP.BaseElement {
});
}

/**
* @override
*/
onMeasureChanged() {
if (this.layoutCompleted_) {
return;
}

const {width, height} = this.getLayoutBox();
if (width === 0 && height === 0) {
user().warn(
TAG,
'Skipped initializing amp-script due to zero width and height.',
this.element
);
}
}

/**
* @param {!AmpScriptService} service
* @visibleForTesting
Expand All @@ -151,6 +172,8 @@ export class AmpScript extends AMP.BaseElement {

/** @override */
layoutCallback() {
this.layoutCompleted_ = true;

// Layouts that use sizers (responsive, fluid) require the worker-dom
// subtree to be wrapped in a "fill content" container. This is because
// these layouts do _not_ constrain the size of the amp-script element
Expand Down
32 changes: 32 additions & 0 deletions extensions/amp-script/0.1/test/unit/test-amp-script.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
} from '../../amp-script';
import {FakeWindow} from '../../../../../testing/fake-dom';
import {Services} from '../../../../../src/services';
import {user} from '../../../../../src/log';

describes.fakeWin('AmpScript', {amp: {runtimeOn: false}}, env => {
let element;
Expand Down Expand Up @@ -93,6 +94,37 @@ describes.fakeWin('AmpScript', {amp: {runtimeOn: false}}, env => {
expect(service.checkSha384).to.be.called;
});

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.onMeasureChanged();
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.onMeasureChanged();

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.onMeasureChanged();
expect(warnStub).to.have.callCount(0);
});
});

it('should fail on invalid sha384(author_js) for cross-origin src', () => {
env.sandbox.stub(env.ampdoc, 'getUrl').returns('https://foo.example/');
element.setAttribute('src', 'https://bar.example/bar.js');
Expand Down

0 comments on commit ae8156f

Please sign in to comment.