From 704a4f21c0f39e1eac81b6ed67f0c60642ef1a84 Mon Sep 17 00:00:00 2001 From: powerivq Date: Fri, 21 Jan 2022 16:47:49 -0800 Subject: [PATCH] Force amp sticky ad layout and move scroll listener to earlier stage --- extensions/amp-a4a/0.1/amp-a4a.js | 18 ++++++++----- extensions/amp-ad/0.1/amp-ad-3p-impl.js | 18 ++++++++----- .../amp-ad/0.1/test/test-amp-ad-3p-impl.js | 26 +++++++++---------- 3 files changed, 34 insertions(+), 28 deletions(-) diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js index 76bac845b4bd..facf328c0694 100644 --- a/extensions/amp-a4a/0.1/amp-a4a.js +++ b/extensions/amp-a4a/0.1/amp-a4a.js @@ -48,6 +48,7 @@ import { getDefaultBootstrapBaseUrl, } from '../../../src/3p-frame'; import {isAdPositionAllowed} from '../../../src/ad-helper'; +import {ChunkPriority_Enum, chunk} from '../../../src/chunk'; import { getConsentMetadata, getConsentPolicyInfo, @@ -408,6 +409,14 @@ export class AmpA4A extends AMP.BaseElement { this.uiHandler = new AMP.AmpAdUIHandler(this); this.uiHandler.validateStickyAd(); + this.uiHandler + .getScrollPromiseForStickyAd() + .then(() => this.uiHandler.maybeInitStickyAd()); + + if (this.uiHandler.isStickyAd()) { + chunk(this.element, () => this.layoutCallback(), ChunkPriority_Enum.LOW); + } + // Disable crypto key fetching if we are not going to use it in no-signing path. // TODO(ccordry): clean up with no-signing launch. if (!this.isInNoSigningExp()) { @@ -1340,15 +1349,10 @@ export class AmpA4A extends AMP.BaseElement { const checkStillCurrent = this.verifyStillCurrent(); // Promise chain will have determined if creative is valid AMP. - return Promise.all([ - this.adPromise_, - this.uiHandler.getScrollPromiseForStickyAd(), - ]) - .then((values) => { + return this.adPromise_ + .then((creativeMetaData) => { checkStillCurrent(); - this.uiHandler.maybeInitStickyAd(); - const creativeMetaData = values[0]; if (this.isCollapsed_) { return Promise.resolve(); } 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 e0302fbe59c0..d1e092c77a23 100644 --- a/extensions/amp-ad/0.1/amp-ad-3p-impl.js +++ b/extensions/amp-ad/0.1/amp-ad-3p-impl.js @@ -34,6 +34,7 @@ import { import {getIframe, preloadBootstrap} from '../../../src/3p-frame'; import {getAdCid} from '../../../src/ad-cid'; import {getAdContainer, isAdPositionAllowed} from '../../../src/ad-helper'; +import {ChunkPriority_Enum, chunk} from '../../../src/chunk'; import { getConsentMetadata, getConsentPolicyInfo, @@ -194,6 +195,15 @@ export class AmpAd3PImpl extends AMP.BaseElement { this.uiHandler = new AmpAdUIHandler(this); this.uiHandler.validateStickyAd(); + // For sticky ad only: must wait for scrolling event before loading the ad + this.uiHandler + .getScrollPromiseForStickyAd() + .then(() => this.uiHandler.maybeInitStickyAd()); + + if (this.uiHandler.isStickyAd()) { + chunk(this.element, () => this.layoutCallback(), ChunkPriority_Enum.LOW); + } + this.isFullWidthRequested_ = this.shouldRequestFullWidth_(); if (this.isFullWidthRequested_) { @@ -360,21 +370,15 @@ export class AmpAd3PImpl extends AMP.BaseElement { this.element ).pageViewId64; - // For sticky ad only: must wait for scrolling event before loading the ad - const scrollPromise = this.uiHandler.getScrollPromiseForStickyAd(); - this.layoutPromise_ = Promise.all([ getAdCid(this), consentPromise, sharedDataPromise, consentStringPromise, consentMetadataPromise, - scrollPromise, pageViewId64Promise, ]) .then((consents) => { - this.uiHandler.maybeInitStickyAd(); - // Use JsonObject to preserve field names so that ampContext can access // values with name // ampcontext.js and this file are compiled in different compilation unit @@ -390,7 +394,7 @@ export class AmpAd3PImpl extends AMP.BaseElement { 'consentSharedData': consents[2], 'initialConsentValue': consents[3], 'initialConsentMetadata': consents[4], - 'pageViewId64': consents[6], + 'pageViewId64': consents[5], }; // In this path, the request and render start events are entangled, diff --git a/extensions/amp-ad/0.1/test/test-amp-ad-3p-impl.js b/extensions/amp-ad/0.1/test/test-amp-ad-3p-impl.js index db169a922368..e4cd1f3d834e 100644 --- a/extensions/amp-ad/0.1/test/test-amp-ad-3p-impl.js +++ b/extensions/amp-ad/0.1/test/test-amp-ad-3p-impl.js @@ -1,6 +1,7 @@ import '../../../amp-sticky-ad/1.0/amp-sticky-ad'; import '../amp-ad'; import * as fakeTimers from '@sinonjs/fake-timers'; +import {expect} from 'chai'; import {adConfig} from '#ads/_config'; @@ -287,20 +288,17 @@ describes.realWin( describe('during layout', () => { it('sticky ad: should not layout w/o scroll', () => { - ad3p.uiHandler.stickyAdPosition_ = 'bottom'; - expect(ad3p.xOriginIframeHandler_).to.be.null; - const layoutPromise = ad3p.layoutCallback(); - return Promise.race([macroTask(), layoutPromise]) - .then(() => { - expect(ad3p.xOriginIframeHandler_).to.be.null; - }) - .then(() => { - Services.viewportForDoc(env.ampdoc).scrollObservable_.fire(); - return layoutPromise; - }) - .then(() => { - expect(ad3p.xOriginIframeHandler_).to.not.be.null; - }); + ad3p.element.setAttribute('sticky', 'bottom'); + ad3p.buildCallback(); + const maybeInitStickyAdSpy = env.sandbox.spy( + ad3p.uiHandler, + 'maybeInitStickyAd' + ); + expect(maybeInitStickyAdSpy).to.not.be.called; + Services.viewportForDoc(env.ampdoc).scrollObservable_.fire(); + return Promise.resolve().then(() => { + expect(maybeInitStickyAdSpy).to.be.called; + }); }); });