Force amp sticky ad layout and move scroll listener to earlier stage (#…

…37456)
ampproject · Jan 29, 2022 · ccf1f0f · ccf1f0f
1 parent 39964d5
commit ccf1f0f
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 28 deletions.
diff --git 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
@@ -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
@@ -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;
+          });
         });
       });