Skip to content

Commit

Permalink
Lower the load priority of ad shaped iframes. (#3863)
Browse files Browse the repository at this point in the history
Some sites use `amp-iframe` to load ads. This is not OK in AMP. With this change at least they render with the same priority as ads.
  • Loading branch information
cramforce authored Jul 2, 2016
1 parent c6bc6e8 commit 8755a24
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 4 deletions.
38 changes: 38 additions & 0 deletions extensions/amp-iframe/0.1/amp-iframe.js
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,14 @@ export class AmpIframe extends AMP.BaseElement {
}
}

/** @override */
getPriority() {
if (isAdLike(this.element)) {
return 2; // See AmpAd3PImpl.
}
return super.getPriority();
}

/**
* Makes the iframe visible.
* @private
Expand Down Expand Up @@ -436,6 +444,36 @@ function makeIOsScrollable(element) {
return element;
}

// Most common ad sizes
// Array of [width, height] pairs.
const adSizes = [[300, 250], [320, 50], [300, 50], [320, 100]];

/**
* Guess whether this element might be an ad.
* @param {!Element} element An amp-iframe element.
* @return {boolean}
* @visibleForTesting
*/
export function isAdLike(element) {
const height = parseInt(element.getAttribute('height'), 10);
const width = parseInt(element.getAttribute('width'), 10);
for (let i = 0; i < adSizes.length; i++) {
const refWidth = adSizes[i][0];
const refHeight = adSizes[i][1];
if (refHeight > height) {
continue;
}
if (refWidth > width) {
continue;
}
// Fuzzy matching to account for padding.
if (height - refHeight <= 20 && width - refWidth <= 20) {
return true;
}
}
return false;
}

/**
* @param {number} ms
*/
Expand Down
30 changes: 26 additions & 4 deletions extensions/amp-iframe/0.1/test/test-amp-iframe.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@
*/

import {Timer} from '../../../../src/timer';
import {AmpIframe, setTrackingIframeTimeoutForTesting} from '../amp-iframe';
import {
AmpIframe,
isAdLike,
setTrackingIframeTimeoutForTesting,
} from '../amp-iframe';
import {adopt} from '../../../../src/runtime';
import {
createIframePromise,
Expand Down Expand Up @@ -142,20 +146,23 @@ describe('amp-iframe', () => {
expect(amp.iframe.getAttribute('sandbox')).to.equal('');
expect(amp.iframe.parentNode).to.equal(amp.scrollWrapper);
expect(impl.looksLikeTrackingIframe_()).to.be.false;
expect(impl.getPriority()).to.equal(0);
return timer.promise(50).then(() => {
expect(ranJs).to.equal(0);
});
});
});

it('should allow JS and propagate scrolling', () => {
it('should allow JS and propagate scrolling and have lower priority', () => {
return getAmpIframe({
src: iframeSrc,
sandbox: 'allow-scripts',
width: 100,
height: 100,
width: 320,
height: 250,
scrolling: 'no',
}).then(amp => {
const impl = amp.container.implementation_;
expect(impl.getPriority()).to.equal(2);
expect(amp.iframe.getAttribute('sandbox')).to.equal('allow-scripts');
return waitForJsInIframe().then(() => {
expect(ranJs).to.equal(1);
Expand Down Expand Up @@ -519,4 +526,19 @@ describe('amp-iframe', () => {
expect(impl.looksLikeTrackingIframe_()).to.be.false;
});
});

it('should correctly classify ads', () => {
function e(width, height) {
const element = document.createElement('test');
element.setAttribute('width', width);
element.setAttribute('height', height);
return element;
}
expect(isAdLike(e(300, 250))).to.be.true;
expect(isAdLike(e(320, 270))).to.be.true;
expect(isAdLike(e(299, 249))).to.be.false;
expect(isAdLike(e(320, 100))).to.be.true;
expect(isAdLike(e(335, 100))).to.be.true;
expect(isAdLike(e(341, 100))).to.be.false;
});
});

0 comments on commit 8755a24

Please sign in to comment.