From 75abc6b58384e90d28ab3501eccd91e78c677bef Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Thu, 14 Jan 2021 16:05:22 -0500 Subject: [PATCH] Convert ad iframes to get initialIntersection async (#31753) * getContextMetadata --> Promise * ad iframes gather initialIntersection async * use experiments * remove log / fix test * revert test change now that under expeirment. * fix test-amp-ad-custom * caleb updates * toggle intersection needs win --- 3p/3d-gltf/viewer.js | 3 +- .../global-configs/canary-config.json | 1 + extensions/amp-a4a/0.1/amp-a4a.js | 71 ++++++++++++++----- extensions/amp-a4a/0.1/name-frame-renderer.js | 60 ++++++++++------ extensions/amp-a4a/0.1/test/test-amp-a4a.js | 7 +- .../0.1/test/test-name-frame-renderer.js | 15 +++- .../0.1/test/test-amp-ad-custom.js | 6 +- extensions/amp-ad/0.1/amp-ad-3p-impl.js | 35 ++++++--- .../legacy-ad-intersection-observer-host.js | 32 +-------- ...st-legacy-ad-intersection-observer-host.js | 4 +- src/3p-frame.js | 11 ++- src/iframe-attributes.js | 1 - test/integration/test-amp-ad-3p.js | 10 ++- test/unit/3p/test-3p-frame.js | 37 +++------- 14 files changed, 169 insertions(+), 124 deletions(-) diff --git a/3p/3d-gltf/viewer.js b/3p/3d-gltf/viewer.js index 1ee91bde01f9..b1c89772b10d 100644 --- a/3p/3d-gltf/viewer.js +++ b/3p/3d-gltf/viewer.js @@ -58,8 +58,7 @@ export default class GltfViewer { this.ampPlay_ = true; /** @private */ - this.ampInViewport_ = - options['initialIntersection']['intersectionRatio'] > 0; + this.ampInViewport_ = false; /** @private */ this.setSize_ = this.setupSize_(); diff --git a/build-system/global-configs/canary-config.json b/build-system/global-configs/canary-config.json index 662550cb0789..9f893f8976dc 100644 --- a/build-system/global-configs/canary-config.json +++ b/build-system/global-configs/canary-config.json @@ -16,6 +16,7 @@ "build-in-chunks": 1, "visibility-trigger-improvements": 1, "fie-resources": 1, + "ads-initialIntersection": 1, "amp-cid-backup": 1, "sticky-ad-transition": 0.1 } diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js index 5c7b6cc4a2d4..8aecd690ec18 100644 --- a/extensions/amp-a4a/0.1/amp-a4a.js +++ b/extensions/amp-a4a/0.1/amp-a4a.js @@ -55,7 +55,7 @@ import { getConsentPolicyState, } from '../../../src/consent'; import {getContextMetadata} from '../../../src/iframe-attributes'; -import {getExperimentBranch} from '../../../src/experiments'; +import {getExperimentBranch, isExperimentOn} from '../../../src/experiments'; import {getMode} from '../../../src/mode'; import {insertAnalyticsElement} from '../../../src/extension-analytics'; import { @@ -63,6 +63,10 @@ import { isSrcdocSupported, } from '../../../src/friendly-iframe-embed'; import {installUrlReplacementsForEmbed} from '../../../src/service/url-replacements-impl'; +import { + intersectionEntryToJson, + measureIntersection, +} from '../../../src/utils/intersection'; import {isAdPositionAllowed} from '../../../src/ad-helper'; import {isArray, isEnumValue, isObject} from '../../../src/types'; import {listenOnce} from '../../../src/event-helper'; @@ -2042,14 +2046,30 @@ export class AmpA4A extends AMP.BaseElement { */ renderViaIframeGet_(adUrl) { this.maybeTriggerAnalyticsEvent_('renderCrossDomainStart'); - return this.iframeRenderHelper_( - dict({ - 'src': Services.xhrFor(this.win).getCorsUrl(this.win, adUrl), - 'name': JSON.stringify( - getContextMetadata(this.win, this.element, this.sentinel) - ), - }) + const contextMetadata = getContextMetadata( + this.win, + this.element, + this.sentinel ); + const asyncIntersection = isExperimentOn( + this.win, + 'ads-initialIntersection' + ); + const intersectionPromise = asyncIntersection + ? measureIntersection(this.element) + : Promise.resolve(this.element.getIntersectionChangeEntry()); + + return intersectionPromise.then((intersection) => { + contextMetadata['_context'][ + 'initialIntersection' + ] = intersectionEntryToJson(intersection); + return this.iframeRenderHelper_( + dict({ + 'src': Services.xhrFor(this.win).getCorsUrl(this.win, adUrl), + 'name': JSON.stringify(contextMetadata), + }) + ); + }); } /** @@ -2115,17 +2135,30 @@ export class AmpA4A extends AMP.BaseElement { this.sentinel, this.getAdditionalContextMetadata(method == XORIGIN_MODE.SAFEFRAME) ); - // TODO(bradfrizzell) Clean up name assigning. - if (method == XORIGIN_MODE.NAMEFRAME) { - contextMetadata['creative'] = creative; - name = JSON.stringify(contextMetadata); - } else if (method == XORIGIN_MODE.SAFEFRAME) { - contextMetadata = JSON.stringify(contextMetadata); - name = - `${this.safeframeVersion};${creative.length};${creative}` + - `${contextMetadata}`; - } - return this.iframeRenderHelper_(dict({'src': srcPath, 'name': name})); + + const asyncIntersection = isExperimentOn( + this.win, + 'ads-initialIntersection' + ); + const intersectionPromise = asyncIntersection + ? measureIntersection(this.element) + : Promise.resolve(this.element.getIntersectionChangeEntry()); + return intersectionPromise.then((intersection) => { + contextMetadata['initialIntersection'] = intersectionEntryToJson( + intersection + ); + if (method == XORIGIN_MODE.NAMEFRAME) { + contextMetadata['creative'] = creative; + name = JSON.stringify(contextMetadata); + } else if (method == XORIGIN_MODE.SAFEFRAME) { + contextMetadata = JSON.stringify(contextMetadata); + name = + `${this.safeframeVersion};${creative.length};${creative}` + + `${contextMetadata}`; + } + + return this.iframeRenderHelper_(dict({'src': srcPath, 'name': name})); + }); }); } diff --git a/extensions/amp-a4a/0.1/name-frame-renderer.js b/extensions/amp-a4a/0.1/name-frame-renderer.js index 9dd3c3f59ceb..c6118870c9ae 100644 --- a/extensions/amp-a4a/0.1/name-frame-renderer.js +++ b/extensions/amp-a4a/0.1/name-frame-renderer.js @@ -19,6 +19,11 @@ import {createElementWithAttributes} from '../../../src/dom'; import {dict} from '../../../src/utils/object'; import {getContextMetadata} from '../../../src/iframe-attributes'; import {getDefaultBootstrapBaseUrl} from '../../../src/3p-frame'; +import { + intersectionEntryToJson, + measureIntersection, +} from '../../../src/utils/intersection'; +import {isExperimentOn} from '../../../src/experiments'; import {utf8Decode} from '../../../src/utils/bytes'; /** @@ -50,28 +55,39 @@ export class NameFrameRenderer extends Renderer { crossDomainData.additionalContextMetadata ); contextMetadata['creative'] = creative; - const attributes = dict({ - 'src': srcPath, - 'name': JSON.stringify(contextMetadata), - 'height': context.size.height, - 'width': context.size.width, - 'frameborder': '0', - 'allowfullscreen': '', - 'allowtransparency': '', - 'scrolling': 'no', - 'marginwidth': '0', - 'marginheight': '0', - }); - if (crossDomainData.sentinel) { - attributes['data-amp-3p-sentinel'] = crossDomainData.sentinel; - } - const iframe = createElementWithAttributes( - /** @type {!Document} */ (element.ownerDocument), - 'iframe', - /** @type {!JsonObject} */ (attributes) + const asyncIntersection = isExperimentOn( + element.ownerDocument.defaultView, + 'ads-initialIntersection' ); - // TODO(glevitzky): Ensure that applyFillContent or equivalent is called. - element.appendChild(iframe); - return Promise.resolve(); + const intersectionPromise = asyncIntersection + ? measureIntersection(element) + : Promise.resolve(element.getIntersectionChangeEntry()); + return intersectionPromise.then((intersection) => { + contextMetadata['_context'][ + 'initialIntersection' + ] = intersectionEntryToJson(intersection); + const attributes = dict({ + 'src': srcPath, + 'name': JSON.stringify(contextMetadata), + 'height': context.size.height, + 'width': context.size.width, + 'frameborder': '0', + 'allowfullscreen': '', + 'allowtransparency': '', + 'scrolling': 'no', + 'marginwidth': '0', + 'marginheight': '0', + }); + if (crossDomainData.sentinel) { + attributes['data-amp-3p-sentinel'] = crossDomainData.sentinel; + } + const iframe = createElementWithAttributes( + /** @type {!Document} */ (element.ownerDocument), + 'iframe', + /** @type {!JsonObject} */ (attributes) + ); + // TODO(glevitzky): Ensure that applyFillContent or equivalent is called. + element.appendChild(iframe); + }); } } diff --git a/extensions/amp-a4a/0.1/test/test-amp-a4a.js b/extensions/amp-a4a/0.1/test/test-amp-a4a.js index 0983c1d04383..34559abcaa2a 100644 --- a/extensions/amp-a4a/0.1/test/test-amp-a4a.js +++ b/extensions/amp-a4a/0.1/test/test-amp-a4a.js @@ -294,7 +294,12 @@ describe('amp-a4a', () => { element.getLayoutBox = () => layoutBox; element.getLayoutSize = () => layoutSizeFromRect(layoutBox); element.getIntersectionChangeEntry = () => { - return null; + return { + time: null, + boundingClientRect: {}, + rootBounds: {}, + intersectionRect: {}, + }; }; const signals = new Signals(); element.signals = () => signals; diff --git a/extensions/amp-a4a/0.1/test/test-name-frame-renderer.js b/extensions/amp-a4a/0.1/test/test-name-frame-renderer.js index 7fd21e64125f..d38da51be574 100644 --- a/extensions/amp-a4a/0.1/test/test-name-frame-renderer.js +++ b/extensions/amp-a4a/0.1/test/test-name-frame-renderer.js @@ -31,7 +31,7 @@ describes.realWin('NameFrameRenderer', realWinConfig, (env) => { let context; let creativeData; - beforeEach(() => { + beforeEach(async () => { context = { size: {width: '320', height: '50'}, requestUrl: 'http://www.google.com', @@ -48,10 +48,19 @@ describes.realWin('NameFrameRenderer', realWinConfig, (env) => { containerElement = env.win.document.createElement('div'); containerElement.setAttribute('height', 50); containerElement.setAttribute('width', 320); - containerElement.getIntersectionChangeEntry = () => ({}); + containerElement.getIntersectionChangeEntry = () => ({ + time: null, + boundingClientRect: {}, + rootBounds: {}, + intersectionRect: {}, + }); env.win.document.body.appendChild(containerElement); - new NameFrameRenderer().render(context, containerElement, creativeData); + await new NameFrameRenderer().render( + context, + containerElement, + creativeData + ); }); it('should append iframe child', () => { diff --git a/extensions/amp-ad-custom/0.1/test/test-amp-ad-custom.js b/extensions/amp-ad-custom/0.1/test/test-amp-ad-custom.js index a2a4b77dd896..da7a7436905d 100644 --- a/extensions/amp-ad-custom/0.1/test/test-amp-ad-custom.js +++ b/extensions/amp-ad-custom/0.1/test/test-amp-ad-custom.js @@ -55,7 +55,11 @@ describes.realWin('TemplateRenderer', realWinConfig, (env) => { width: 0, height: 0, }); - containerElement.getIntersectionChangeEntry = () => ({}); + containerElement.getIntersectionChangeEntry = () => ({ + rootBounds: {}, + intersectionRect: {}, + boundingClientRect: {}, + }); containerElement.isInViewport = () => true; containerElement.getAmpDoc = () => env.ampdoc; doc.body.appendChild(containerElement); diff --git a/extensions/amp-ad/0.1/amp-ad-3p-impl.js b/extensions/amp-ad/0.1/amp-ad-3p-impl.js index bf40db6031e7..ec47c76c4c3b 100644 --- a/extensions/amp-ad/0.1/amp-ad-3p-impl.js +++ b/extensions/amp-ad/0.1/amp-ad-3p-impl.js @@ -48,6 +48,11 @@ import { getConsentPolicyState, } from '../../../src/consent'; import {getIframe, preloadBootstrap} from '../../../src/3p-frame'; +import { + intersectionEntryToJson, + measureIntersection, +} from '../../../src/utils/intersection'; +import {isExperimentOn} from '../../../src/experiments'; import {moveLayoutRect} from '../../../src/layout-rect'; import { observeWithSharedInOb, @@ -407,16 +412,28 @@ export class AmpAd3PImpl extends AMP.BaseElement { // because both happen inside a cross-domain iframe. Separating them // here, though, allows us to measure the impact of ad throttling via // incrementLoadingAds(). - const iframe = getIframe( - toWin(this.element.ownerDocument.defaultView), - this.element, - this.type_, - opt_context, - {disallowCustom: this.config.remoteHTMLDisabled} + const asyncIntersection = isExperimentOn( + this.win, + 'ads-initialIntersection' ); - iframe.title = this.element.title || 'Advertisement'; - this.xOriginIframeHandler_ = new AmpAdXOriginIframeHandler(this); - return this.xOriginIframeHandler_.init(iframe); + const intersectionPromise = asyncIntersection + ? measureIntersection(this.element) + : Promise.resolve(this.element.getIntersectionChangeEntry()); + return intersectionPromise.then((intersection) => { + const iframe = getIframe( + toWin(this.element.ownerDocument.defaultView), + this.element, + this.type_, + opt_context, + { + disallowCustom: this.config.remoteHTMLDisabled, + initialIntersection: intersectionEntryToJson(intersection), + } + ); + iframe.title = this.element.title || 'Advertisement'; + this.xOriginIframeHandler_ = new AmpAdXOriginIframeHandler(this); + return this.xOriginIframeHandler_.init(iframe); + }); }) .then(() => { observeWithSharedInOb(this.element, (inViewport) => diff --git a/extensions/amp-ad/0.1/legacy-ad-intersection-observer-host.js b/extensions/amp-ad/0.1/legacy-ad-intersection-observer-host.js index a017abdfcf28..9ddf2a238c37 100644 --- a/extensions/amp-ad/0.1/legacy-ad-intersection-observer-host.js +++ b/extensions/amp-ad/0.1/legacy-ad-intersection-observer-host.js @@ -19,6 +19,7 @@ import {Services} from '../../../src/services'; import {SubscriptionApi} from '../../../src/iframe-helper'; import {devAssert} from '../../../src/log'; import {dict} from '../../../src/utils/object'; +import {intersectionEntryToJson} from '../../../src/utils/intersection'; import { layoutRectLtwh, moveLayoutRect, @@ -319,34 +320,3 @@ export class LegacyAdIntersectionObserverHost { this.postMessageApi_.destroy(); } } - -/** - * Convert a DOMRect to a regular object to make it serializable. - * - * @param {!DOMRect} domRect - * @return {!DOMRect} - */ -function domRectToJson(domRect) { - if (domRect == null) { - return domRect; - } - - const {x, y, width, height, top, right, bottom, left} = domRect; - return {x, y, width, height, top, right, bottom, left}; -} - -/** - * Convert an IntersectionObserverEntry to a regular object to make it serializable. - * - * @param {!IntersectionObserverEntry} entry - * @return {!IntersectionObserverEntry} - */ -function intersectionEntryToJson(entry) { - return { - time: entry.time, - rootBounds: domRectToJson(entry.rootBounds), - boundingClientRect: domRectToJson(entry.boundingClientRect), - intersectionRect: domRectToJson(entry.intersectionRect), - intersectionRatio: entry.intersectionRatio, - }; -} diff --git a/extensions/amp-ad/0.1/test/test-legacy-ad-intersection-observer-host.js b/extensions/amp-ad/0.1/test/test-legacy-ad-intersection-observer-host.js index 8223bfd51923..57b26e7a8194 100644 --- a/extensions/amp-ad/0.1/test/test-legacy-ad-intersection-observer-host.js +++ b/extensions/amp-ad/0.1/test/test-legacy-ad-intersection-observer-host.js @@ -461,7 +461,7 @@ describes.sandboxed('IntersectionObserverHostForAd', {}, () => { ); stubFireInOb(ioInstance); insert(testIframe); - ioInstance.onViewportCallback_(true); + ioInstance.onViewportCallback_(getInObEntry()); expect(sendElementIntersectionSpy).to.be.calledOnce; expect(onScrollSpy).to.be.calledOnce; expect(onChangeSpy).to.be.calledOnce; @@ -479,7 +479,7 @@ describes.sandboxed('IntersectionObserverHostForAd', {}, () => { ); insert(testIframe); ioInstance.onViewportCallback_(getInObEntry()); - ioInstance.onViewportCallback_({intersectionRatio: false}); + ioInstance.onViewportCallback_({...getInObEntry(), intersectionRatio: 0}); expect(sendElementIntersectionSpy).to.have.callCount(2); expect(ioInstance.unlistenViewportChanges_).to.be.null; }); diff --git a/src/3p-frame.js b/src/3p-frame.js index 6e38481a2659..254d6dd03224 100644 --- a/src/3p-frame.js +++ b/src/3p-frame.js @@ -68,6 +68,7 @@ function getFrameAttributes(parentWindow, element, opt_type, opt_context) { * @param {{ * disallowCustom: (boolean|undefined), * allowFullscreen: (boolean|undefined), + * initialIntersection: (IntersectionObserverEntry|undefined), * }=} options Options for the created iframe. * @return {!HTMLIFrameElement} The iframe. */ @@ -78,7 +79,11 @@ export function getIframe( opt_context, options = {} ) { - const {disallowCustom = false, allowFullscreen = false} = options; + const { + disallowCustom = false, + allowFullscreen = false, + initialIntersection, + } = options; // Check that the parentElement is already in DOM. This code uses a new and // fast `isConnected` API and thus only used when it's available. devAssert( @@ -92,6 +97,10 @@ export function getIframe( opt_type, opt_context ); + if (initialIntersection) { + attributes['_context']['initialIntersection'] = initialIntersection; + } + const iframe = /** @type {!HTMLIFrameElement} */ (parentWindow.document.createElement( 'iframe' )); diff --git a/src/iframe-attributes.js b/src/iframe-attributes.js index 994e36fd7ebf..14318d1b5f70 100644 --- a/src/iframe-attributes.js +++ b/src/iframe-attributes.js @@ -94,7 +94,6 @@ export function getContextMetadata( 'height': layoutRect.height, } : null, - 'initialIntersection': element.getIntersectionChangeEntry(), 'domFingerprint': DomFingerprint.generate(element), 'experimentToggles': experimentToggles(parentWindow), 'sentinel': sentinel, diff --git a/test/integration/test-amp-ad-3p.js b/test/integration/test-amp-ad-3p.js index 75aea522d8b4..b058c6b09f3c 100644 --- a/test/integration/test-amp-ad-3p.js +++ b/test/integration/test-amp-ad-3p.js @@ -19,6 +19,7 @@ import {createCustomEvent} from '../../src/event-helper'; import {createFixtureIframe, poll} from '../../testing/iframe'; import {installPlatformService} from '../../src/service/platform-impl'; import {layoutRectLtwh} from '../../src/layout-rect'; +import {toggleExperiment} from '../../src/experiments'; function createFixture() { return createFixtureIframe('test/fixtures/3p-ad.html', 3000, () => {}); @@ -35,6 +36,7 @@ describe('amp-ad 3P', () => { }); it('create an iframe with APIs', async function () { + toggleExperiment(window, 'ads-initialIntersection'); this.timeout(20000); let iframe; let lastIO = null; @@ -95,16 +97,12 @@ describe('amp-ad 3P', () => { }); const {initialIntersection} = context; expect(initialIntersection.rootBounds).to.deep.equal( - layoutRectLtwh(0, 0, 500, 3000) + layoutRectLtwh(0, 0, window.innerWidth, window.innerHeight) ); + expect(initialIntersection.boundingClientRect).to.deep.equal( layoutRectLtwh(0, platform.isIos() ? 1001 : 1000, 300, 250) ); - expect(initialIntersection.intersectionRect).to.deep.equal( - layoutRectLtwh(0, platform.isIos() ? 1001 : 1000, 300, 250) - ); - expect(initialIntersection.intersectionRatio).to.equal(1); - expect(initialIntersection.time).to.be.a('number'); expect(context.isMaster).to.exist; expect(context.computeInMasterFrame).to.exist; expect(context.location).to.deep.equal({ diff --git a/test/unit/3p/test-3p-frame.js b/test/unit/3p/test-3p-frame.js index 661bf54989b6..86c282acf00c 100644 --- a/test/unit/3p/test-3p-frame.js +++ b/test/unit/3p/test-3p-frame.js @@ -168,7 +168,6 @@ describes.realWin('3p-frame', {amp: true}, (env) => { div.setAttribute('width', '50'); div.setAttribute('height', '100'); - const {innerWidth: width, innerHeight: height} = window; setupElementFunctions(div); const viewer = Services.viewerForDoc(window.document); @@ -190,7 +189,16 @@ describes.realWin('3p-frame', {amp: true}, (env) => { .stub(WindowInterface, 'getLocation') .returns({href: locationHref}); - const iframe = getIframe(window, div, '_ping_', {clientId: 'cidValue'}); + const initialIntersection = {test: 'testIntersection'}; + const iframe = getIframe( + window, + div, + '_ping_', + {clientId: 'cidValue'}, + { + initialIntersection, + } + ); const {src} = iframe; const docInfo = Services.documentInfoForDoc(window.document); expect(docInfo.pageViewId).to.not.be.empty; @@ -230,30 +238,7 @@ describes.realWin('3p-frame', {amp: true}, (env) => { 'startTime': 1234567888, 'experimentToggles': {'exp-a': true, 'exp-b': true}, 'sentinel': sentinel, - 'initialIntersection': { - 'time': 1234567888, - 'rootBounds': { - 'left': 0, - 'top': 0, - 'width': width, - 'height': height, - 'bottom': height, - 'right': width, - 'x': 0, - 'y': 0, - }, - 'boundingClientRect': {'width': 100, 'height': 200}, - 'intersectionRect': { - 'left': 0, - 'top': 0, - 'width': 0, - 'height': 0, - 'bottom': 0, - 'right': 0, - 'x': 0, - 'y': 0, - }, - }, + initialIntersection, }, }; expect(src).to.equal(