♻️ Make sticky ad available on a4a (ampproject#31372)

* Make sticky ad available on a4a * Fix the amp-ad sticky ad loading indicator issue * Revert * Update some css problems
henel677 · Dec 4, 2020 · 6bb8f68 · 6bb8f68
1 parent 2b730eb
commit 6bb8f68
Show file tree

Hide file tree

Showing 11 changed files with 173 additions and 109 deletions.
diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js
@@ -1297,9 +1297,16 @@ export class AmpA4A extends AMP.BaseElement {
     }
     const checkStillCurrent = this.verifyStillCurrent();
     // Promise chain will have determined if creative is valid AMP.
-    return this.adPromise_
-      .then((creativeMetaData) => {
+
+    return Promise.all([
+      this.adPromise_,
+      this.uiHandler.getScrollPromiseForStickyAd(),
+    ])
+      .then((values) => {
         checkStillCurrent();
+
+        this.uiHandler.maybeInitStickyAd();
+        const creativeMetaData = values[0];
         if (this.isCollapsed_) {
           return Promise.resolve();
         }
@@ -1442,6 +1449,9 @@ export class AmpA4A extends AMP.BaseElement {
       this.xOriginIframeHandler_.freeXOriginIframe();
       this.xOriginIframeHandler_ = null;
     }
+    if (this.uiHandler) {
+      this.uiHandler.cleanup();
+    }
   }
 
   // TODO: Rename to viewportCallback once BaseElement.viewportCallback has been removed.

diff --git a/extensions/amp-a4a/0.1/test/test-amp-a4a.js b/extensions/amp-a4a/0.1/test/test-amp-a4a.js
@@ -1515,6 +1515,11 @@ describe('amp-a4a', () => {
         .stub(a4a, 'maybeValidateAmpCreative')
         .returns(Promise.resolve());
       a4a.onLayoutMeasure();
+      a4a.uiHandler = {
+        getScrollPromiseForStickyAd: () => Promise.resolve(null),
+        isStickyAd: () => false,
+        maybeInitStickyAd: () => {},
+      };
       await a4a.layoutCallback();
       expect(
         renderNonAmpCreativeSpy.calledOnce,
@@ -1948,6 +1953,9 @@ describe('amp-a4a', () => {
           applyUnlayoutUI: () => {
             unlayoutUISpy();
           },
+          getScrollPromiseForStickyAd: () => Promise.resolve(null),
+          maybeInitStickyAd: () => {},
+          cleanup: () => {},
         };
         window.sandbox
           .stub(a4a, 'getLayoutBox')

diff --git a/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js b/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js
@@ -1006,7 +1006,7 @@ describes.realWin(
         impl.win.ampAdSlotIdCounter = 1;
         expect(impl.element.getAttribute('data-amp-slot-index')).to.not.be.ok;
         impl.layoutMeasureExecuted_ = true;
-        impl.uiHandler = {applyUnlayoutUI: () => {}};
+        impl.uiHandler = {applyUnlayoutUI: () => {}, cleanup: () => {}};
         const placeholder = doc.createElement('div');
         placeholder.setAttribute('placeholder', '');
         const fallback = doc.createElement('div');

diff --git a/extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js b/extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js
@@ -1123,7 +1123,7 @@ describes.realWin('amp-ad-network-doubleclick-impl', realWinConfig, (env) => {
       impl.win.ampAdSlotIdCounter = 1;
       expect(impl.element.getAttribute('data-amp-slot-index')).to.not.be.ok;
       impl.layoutMeasureExecuted_ = true;
-      impl.uiHandler = {applyUnlayoutUI: () => {}};
+      impl.uiHandler = {applyUnlayoutUI: () => {}, cleanup: () => {}};
       const placeholder = doc.createElement('div');
       placeholder.setAttribute('placeholder', '');
       const fallback = doc.createElement('div');

diff --git a/extensions/amp-ad/0.1/amp-ad-3p-impl.js b/extensions/amp-ad/0.1/amp-ad-3p-impl.js
@@ -31,8 +31,7 @@ import {
 import {Services} from '../../../src/services';
 import {adConfig} from '../../../ads/_config';
 import {clamp} from '../../../src/utils/math';
-import {computedStyle, setStyle, setStyles} from '../../../src/style';
-import {createElementWithAttributes, removeElement} from '../../../src/dom';
+import {computedStyle, setStyle} from '../../../src/style';
 import {dev, devAssert, userAssert} from '../../../src/log';
 import {dict} from '../../../src/utils/object';
 import {getAdCid} from '../../../src/ad-cid';
@@ -49,7 +48,6 @@ import {
   getConsentPolicyState,
 } from '../../../src/consent';
 import {getIframe, preloadBootstrap} from '../../../src/3p-frame';
-import {listen} from '../../../src/event-helper';
 import {moveLayoutRect} from '../../../src/layout-rect';
 import {
   observeWithSharedInOb,
@@ -79,12 +77,6 @@ export class AmpAd3PImpl extends AMP.BaseElement {
      */
     this.iframe_ = null;
 
-    /** @private {?../../../src/service/viewport/viewport-interface.ViewportInterface} */
-    this.viewport_ = null;
-
-    /** @private {?../../../src/service/vsync-impl.Vsync} */
-    this.vsync_ = null;
-
     /** @type {?Object} */
     this.config = null;
 
@@ -152,17 +144,6 @@ export class AmpAd3PImpl extends AMP.BaseElement {
      * @private {boolean}
      */
     this.isFullWidthRequested_ = false;
-
-    /**
-     * Whether this is a sticky ad unit.
-     * @private {boolean}
-     */
-    this.isStickyAd_ = false;
-
-    /**
-     * Whether the close button has been rendered for a sticky ad unit.
-     */
-    this.closeButtonRendered_ = false;
   }
 
   /** @override */
@@ -211,10 +192,7 @@ export class AmpAd3PImpl extends AMP.BaseElement {
 
   /** @override */
   buildCallback() {
-    this.viewport_ = this.getViewport();
-    this.vsync_ = this.getVsync();
     this.type_ = this.element.getAttribute('type');
-    this.isStickyAd_ = this.element.hasAttribute('sticky');
     const upgradeDelayMs = Math.round(this.getResource().getUpgradeDelayMs());
     dev().info(TAG_3P_IMPL, `upgradeDelay ${this.type_}: ${upgradeDelayMs}`);
 
@@ -231,8 +209,6 @@ export class AmpAd3PImpl extends AMP.BaseElement {
     if (this.isFullWidthRequested_) {
       return this.attemptFullWidthSizeChange_();
     }
-
-    this.maybeSetStyleForSticky_();
   }
 
   /**
@@ -259,21 +235,6 @@ export class AmpAd3PImpl extends AMP.BaseElement {
     return true;
   }
 
-  /**
-   * Set sidekick ads
-   */
-  maybeSetStyleForSticky_() {
-    if (this.isStickyAd_) {
-      setStyles(this.element, {
-        position: 'fixed',
-        bottom: '0',
-        right: '0',
-      });
-
-      this.element.classList.add('i-amphtml-amp-ad-sticky-layout');
-    }
-  }
-
   /**
    * Prefetches and preconnects URLs related to the ad.
    * @param {boolean=} opt_onLayout
@@ -393,7 +354,7 @@ export class AmpAd3PImpl extends AMP.BaseElement {
       return this.layoutPromise_;
     }
     userAssert(
-      !this.isInFixedContainer_ || this.isStickyAd_,
+      !this.isInFixedContainer_ || this.uiHandler.isStickyAd(),
       '<amp-ad> is not allowed to be placed in elements with ' +
         'position:fixed: %s unless it has sticky attribute',
       this.element
@@ -412,14 +373,7 @@ export class AmpAd3PImpl extends AMP.BaseElement {
       : Promise.resolve(null);
 
     // For sticky ad only: must wait for scrolling event before loading the ad
-    const scrollPromise = this.isStickyAd_
-      ? new Promise((resolve) => {
-          const unlisten = this.viewport_.onScroll(() => {
-            resolve();
-            unlisten();
-          });
-        })
-      : Promise.resolve(null);
+    const scrollPromise = this.uiHandler.getScrollPromiseForStickyAd();
 
     this.layoutPromise_ = Promise.all([
       getAdCid(this),
@@ -430,6 +384,8 @@ export class AmpAd3PImpl extends AMP.BaseElement {
       scrollPromise,
     ])
       .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
@@ -498,6 +454,9 @@ export class AmpAd3PImpl extends AMP.BaseElement {
       this.xOriginIframeHandler_.freeXOriginIframe();
       this.xOriginIframeHandler_ = null;
     }
+    if (this.uiHandler) {
+      this.uiHandler.cleanup();
+    }
     return true;
   }
 
@@ -511,23 +470,6 @@ export class AmpAd3PImpl extends AMP.BaseElement {
       : Promise.resolve(null);
   }
 
-  /**
-   * @return {boolean}
-   */
-  isStickyAd() {
-    return this.isStickyAd_;
-  }
-
-  /**
-   * When a sticky ad is shown, the close button should be rendered at the same time.
-   */
-  onResizeSuccess() {
-    if (this.isStickyAd_ && !this.closeButtonRendered_) {
-      this.addCloseButton_();
-      this.closeButtonRendered_ = true;
-    }
-  }
-
   /**
    * Calculates and attempts to set the appropriate height & width for a
    * responsive full width ad unit.
@@ -575,32 +517,4 @@ export class AmpAd3PImpl extends AMP.BaseElement {
       maxHeight
     );
   }
-
-  /**
-   * The function that add a close button to sticky ad
-   */
-  addCloseButton_() {
-    const closeButton = createElementWithAttributes(
-      /** @type {!Document} */ (this.element.ownerDocument),
-      'button',
-      dict({
-        'aria-label':
-          this.element.getAttribute('data-close-button-aria-label') ||
-          'Close this ad',
-      })
-    );
-
-    this.unlisteners_.push(
-      listen(closeButton, 'click', () => {
-        this.vsync_.mutate(() => {
-          this.viewport_.removeFromFixedLayer(this.element);
-          removeElement(this.element);
-          this.viewport_.updatePaddingBottom(0);
-        });
-      })
-    );
-
-    closeButton.classList.add('amp-ad-close-button');
-    this.element.appendChild(closeButton);
-  }
 }