Skip to content

Commit

Permalink
address feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
samouri committed Jan 23, 2020
1 parent e9d27c8 commit 3a2c0c8
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 11 deletions.
8 changes: 8 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.layoutCallbackHappened_ = 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 @@ -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(
Expand Down Expand Up @@ -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.
Expand Down
40 changes: 29 additions & 11 deletions extensions/amp-script/0.1/test/unit/test-amp-script.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down

0 comments on commit 3a2c0c8

Please sign in to comment.