Skip to content

Commit

Permalink
✨ Set #amp=1 fragment in amp-video-iframe src (ampproject#20823)
Browse files Browse the repository at this point in the history 
Docs say we do it, but we don't, so let's do it!

Also cleaned up some tests to be clearer and to use `async/await`.
  • Loading branch information
@alanorozco
alanorozco authored Feb 13, 2019
1 parent f92f627 commit b58b6fa
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 62 deletions.
18 changes: 15 additions & 3 deletions extensions/amp-video-iframe/0.1/amp-video-iframe.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,18 @@ const getAnalyticsEventTypePrefixRegex = once(() =>
new RegExp(`^${ANALYTICS_EVENT_TYPE_PREFIX}`));


/**
* @param {string} src
* @return {string}
*/
function maybeAddAmpFragment(src) {
if (src.indexOf('#') > -1) {
return src;
}
return `${src}#amp=1`;
}


/** @implements {../../../src/video-interface.VideoInterface} */
class AmpVideoIframe extends AMP.BaseElement {

Expand Down Expand Up @@ -181,8 +193,8 @@ class AmpVideoIframe extends AMP.BaseElement {
/** @override */
createPlaceholderCallback() {
const {element} = this;
const poster =
htmlFor(element)`<amp-img layout=fill placeholder></amp-img>`;
const html = htmlFor(element);
const poster = html`<amp-img layout=fill placeholder></amp-img>`;

poster.setAttribute('src',
this.user().assertString(element.getAttribute('poster')));
Expand Down Expand Up @@ -226,7 +238,7 @@ class AmpVideoIframe extends AMP.BaseElement {
element);
}

return src;
return maybeAddAmpFragment(src);
}

/**
Expand Down
130 changes: 71 additions & 59 deletions extensions/amp-video-iframe/0.1/test/test-amp-video-iframe.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ describes.realWin('amp-video-iframe', {
extensions: ['amp-video-iframe'],
},
}, env => {

const {any} = sinon.match;
const defaultFixture = 'video-iframe.html';

let win;
Expand All @@ -62,22 +62,17 @@ describes.realWin('amp-video-iframe', {
fixture || defaultFixture}`);
}

function createVideoIframe(opt_size) {
const el = htmlFor(doc)`<amp-video-iframe></amp-video-iframe>`;

if (opt_size) {
addAttributesToElement(el, {
'layout': 'fixed',
'width': opt_size[0],
'height': opt_size[1],
});
} else {
addAttributesToElement(el, {
'layout': 'fill',
});
}

addAttributesToElement(el, {src: getIframeSrc(), poster: 'poster.html'});
const layoutConfigAttrs = size => !size ? {layout: 'fill'} : {
layout: 'fixed',
width: size[0],
height: size[1],
};

function createVideoIframe({size, src} = {}) {
const html = htmlFor(doc);
const el = html`<amp-video-iframe poster=foo.png></amp-video-iframe>`;
el.setAttribute('src', src || getIframeSrc());
addAttributesToElement(el, layoutConfigAttrs(size));
doc.body.appendChild(el);
return el;
}
Expand All @@ -91,7 +86,7 @@ describes.realWin('amp-video-iframe', {
.returns(true);
}

function whenLoaded(videoIframe) {
function layoutAndLoad(videoIframe) {
return whenUpgradedToCustomElement(videoIframe).then(() => {
videoIframe.implementation_.layoutCallback();
return listenOncePromise(videoIframe, VideoEvents.LOAD);
Expand All @@ -109,8 +104,8 @@ describes.realWin('amp-video-iframe', {
return entry;
}

describe('#buildCallback', () => {
it('sets metadata', function* () {
describe('#layoutCallback', () => {
it('sets metadata in iframe name', async() => {
const metadata = {
canonicalUrl: 'foo.html',
sourceUrl: 'bar.html',
Expand All @@ -120,18 +115,35 @@ describes.realWin('amp-video-iframe', {

const videoIframe = createVideoIframe();

yield whenLoaded(videoIframe);
await layoutAndLoad(videoIframe);

const {name} = videoIframe.implementation_.iframe_;

// Sinon does not support sinon.match on to.equal
const dummySpy = sandbox.spy();
expect(tryParseJson(name)).to.deep.equal(metadata);
});

it('sets amp=1 fragment in src', async() => {
const rawSrc = getIframeSrc();
const videoIframe = createVideoIframe({src: rawSrc});

await layoutAndLoad(videoIframe);

dummySpy(tryParseJson(name));
const {src} = videoIframe.implementation_.iframe_;
expect(src).to.equal(`${rawSrc}#amp=1`);
});

it('does not set amp=1 fragment in src when fragment present', async() => {
const rawSrc = `${getIframeSrc()}#my-fragment`;
const videoIframe = createVideoIframe({src: rawSrc});

await layoutAndLoad(videoIframe);

expect(dummySpy.withArgs(sinon.match(metadata))).to.have.been.calledOnce;
const {src} = videoIframe.implementation_.iframe_;
expect(src).to.equal(rawSrc);
});
});

describe('#buildCallback', () => {
it('rejects tracking iframes', () => {
const trackingSizes = [
[10, 10],
Expand All @@ -147,8 +159,10 @@ describes.realWin('amp-video-iframe', {
];

trackingSizes.forEach(size => {
const videoIframe = createVideoIframe(size);
expect(whenLoaded(videoIframe)).to.eventually.be.rejected;
const {implementation_} = createVideoIframe({size});
allowConsoleError(() => {
expect(() => implementation_.buildCallback()).to.throw();
});
});
});
});
Expand All @@ -170,21 +184,21 @@ describes.realWin('amp-video-iframe', {

describe('#onMessage_', () => {

it('should load and register on canplay', function* () {
it('should load and register on canplay', async() => {
// Fixture inside frame triggers `canplay`.
const videoIframe = createVideoIframe();
yield whenLoaded(videoIframe);
await layoutAndLoad(videoIframe);

const register =
videoManagerStub.register.withArgs(videoIframe.implementation_);

expect(register).to.have.been.calledOnce;
});

it('should not dispatch invalid events', function* () {
it('should not dispatch invalid events', async() => {
const videoIframe = createVideoIframe();

yield whenLoaded(videoIframe);
await layoutAndLoad(videoIframe);

const dispatch = spyDispatch(videoIframe);

Expand All @@ -196,10 +210,10 @@ describes.realWin('amp-video-iframe', {
});
});

it('should dispatch valid events', function* () {
it('should dispatch valid events', async() => {
const videoIframe = createVideoIframe();

yield whenLoaded(videoIframe);
await layoutAndLoad(videoIframe);

const dispatch = spyDispatch(videoIframe);

Expand All @@ -222,14 +236,14 @@ describes.realWin('amp-video-iframe', {
}
});

it('should return intersection ratio if in autoplay range', function* () {
it('should return intersection ratio if in autoplay range', async() => {
const id = 1234;
const time = 1.234;
const intersectionRatio = 2 / 3;

const videoIframe = createVideoIframe();

yield whenLoaded(videoIframe);
await layoutAndLoad(videoIframe);

const postMessage = stubPostMessage(videoIframe);

Expand All @@ -248,15 +262,15 @@ describes.realWin('amp-video-iframe', {
.to.have.been.calledOnce;
});

it('should return 0 if not in autoplay range', function* () {
it('should return 0 if not in autoplay range', async() => {
const id = 1234;
const time = 1.234;
const intersectionRatio = 1 / 3;
const reportedRatioShouldBe = 0;

const videoIframe = createVideoIframe();

yield whenLoaded(videoIframe);
await layoutAndLoad(videoIframe);

const postMessage = stubPostMessage(videoIframe);

Expand Down Expand Up @@ -302,11 +316,11 @@ describes.realWin('amp-video-iframe', {
].forEach(({sufix, eventType, vars, accept}) => {
const verb = accept ? 'dispatch' : 'reject';

it(`should ${verb} custom analytics event ${sufix}`, function* () {
it(`should ${verb} custom analytics event ${sufix}`, async() => {
const videoIframe = createVideoIframe();
const dispatch = spyDispatch(videoIframe);

yield whenLoaded(videoIframe);
await layoutAndLoad(videoIframe);

acceptMockedMessages(videoIframe);

Expand All @@ -317,50 +331,48 @@ describes.realWin('amp-video-iframe', {
},
};

const expectedVars = vars || {};

if (vars) {
Object.assign(data.analytics, {vars});
}

const {implementation_} = videoIframe;

if (accept) {
videoIframe.implementation_.onMessage_({data});
expect(
dispatch.withArgs(VideoAnalyticsEvents.CUSTOM),
expectedVars).to.have.been.calledOnce;
const expectedEventVars = {eventType, vars: vars || {}};
const expectedDispatch =
dispatch.withArgs(VideoAnalyticsEvents.CUSTOM, expectedEventVars);
implementation_.onMessage_({data});
expect(expectedDispatch).to.have.been.calledOnce;
} else {
allowConsoleError(() => {
expect(() => {
videoIframe.implementation_.onMessage_({data});
}).to.throw();
expect(() => implementation_.onMessage_({data})).to.throw();
});

expect(
dispatch.withArgs(VideoAnalyticsEvents.CUSTOM),
expectedVars).to.not.have.been.called;
expect(dispatch.withArgs(VideoAnalyticsEvents.CUSTOM, any))
.to.not.have.been.called;
}
});
});
});

describe('#mutatedAttributesCallback', () => {
it('updates src', function* () {
const videoIframe = createVideoIframe();
it('updates src', async() => {
const defaultSrc = getIframeSrc(defaultFixture);
const videoIframe = createVideoIframe({src: defaultSrc});
const {implementation_} = videoIframe;

yield whenLoaded(videoIframe);
await layoutAndLoad(videoIframe);

const {iframe_} = implementation_;

expect(iframe_.src).to.equal(getIframeSrc(defaultFixture));
expect(iframe_.src).to.match(new RegExp(`^${defaultSrc}#`));

const newSrc = getIframeSrc('video-iframe-2.html');

videoIframe.setAttribute('src', newSrc);

implementation_.mutatedAttributesCallback({'src': true});

expect(iframe_.src).to.equal(newSrc);
expect(iframe_.src).to.match(new RegExp(`^${newSrc}#`));
});
});

Expand All @@ -379,10 +391,10 @@ describes.realWin('amp-video-iframe', {
describe(`#${method}`, () => {
const lowercaseMethod = method.toLowerCase();

it(`should post '${lowercaseMethod}'`, function* () {
it(`should post '${lowercaseMethod}'`, async() => {
const videoIframe = createVideoIframe();

yield whenLoaded(videoIframe);
await layoutAndLoad(videoIframe);

const postMessage = stubPostMessage(videoIframe);

Expand Down

0 comments on commit b58b6fa

Please sign in to comment.