From 6bb8f6814743e528e639d94129c47602f1c21389 Mon Sep 17 00:00:00 2001 From: Shihua Zheng Date: Fri, 4 Dec 2020 15:43:32 -0800 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Make=20sticky=20ad=20avail?= =?UTF-8?q?able=20on=20a4a=20(#31372)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Make sticky ad available on a4a * Fix the amp-ad sticky ad loading indicator issue * Revert * Update some css problems --- extensions/amp-a4a/0.1/amp-a4a.js | 14 ++- extensions/amp-a4a/0.1/test/test-amp-a4a.js | 8 ++ .../test/test-amp-ad-network-adsense-impl.js | 2 +- .../test-amp-ad-network-doubleclick-impl.js | 2 +- extensions/amp-ad/0.1/amp-ad-3p-impl.js | 102 ++-------------- extensions/amp-ad/0.1/amp-ad-ui.js | 115 +++++++++++++++++- .../0.1/amp-ad-xorigin-iframe-handler.js | 6 +- extensions/amp-ad/0.1/amp-ad.css | 4 + .../amp-ad/0.1/test/test-amp-ad-3p-impl.js | 7 +- extensions/amp-ad/0.1/test/test-amp-ad-ui.js | 15 +++ .../test-amp-ad-xorigin-iframe-handler.js | 7 +- 11 files changed, 173 insertions(+), 109 deletions(-) diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js index 6a81fd3b9490..46361344fa02 100644 --- 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 index 1bb566e43294..78a6c25ae460 100644 --- 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 index df88e71e9ca5..8ede0998b332 100644 --- 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 index fff6c4090097..fb7b11bf44a5 100644 --- 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 index 6ce81fe136a4..bf40db6031e7 100644 --- 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(), ' 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); - } } diff --git a/extensions/amp-ad/0.1/amp-ad-ui.js b/extensions/amp-ad/0.1/amp-ad-ui.js index df38fe559359..d49cbe22817e 100644 --- a/extensions/amp-ad/0.1/amp-ad-ui.js +++ b/extensions/amp-ad/0.1/amp-ad-ui.js @@ -15,8 +15,16 @@ */ import {Services} from '../../../src/services'; -import {ancestorElementsByTag} from '../../../src/dom'; +import { + ancestorElementsByTag, + createElementWithAttributes, + removeElement, +} from '../../../src/dom'; +import {dict} from '../../../src/utils/object'; + import {getAdContainer} from '../../../src/ad-helper'; +import {listen} from '../../../src/event-helper'; +import {setStyles} from '../../../src/style'; const STICKY_AD_MAX_SIZE_LIMIT = 0.2; const STICKY_AD_MAX_HEIGHT_LIMIT = 0.5; @@ -37,6 +45,23 @@ export class AmpAdUIHandler { this.containerElement_ = null; + /** + * Whether this is a sticky ad unit. + * @private {boolean} + */ + this.isStickyAd_ = this.element_.hasAttribute('sticky'); + + /** + * Whether the close button has been rendered for a sticky ad unit. + */ + this.closeButtonRendered_ = false; + + /** + * Unlisteners to be unsubscribed after destroying. + * @private {!Array} + */ + this.unlisteners_ = []; + if (this.element_.hasAttribute('data-ad-container-id')) { const id = this.element_.getAttribute('data-ad-container-id'); const container = this.doc_.getElementById(id); @@ -149,6 +174,86 @@ export class AmpAdUIHandler { return uiComponent; } + /** + * @return {boolean} + */ + isStickyAd() { + return this.isStickyAd_; + } + + /** + * Initialize sticky ad related features + */ + maybeInitStickyAd() { + if (this.isStickyAd_) { + setStyles(this.element_, { + position: 'fixed', + bottom: '0', + right: '0', + visibility: 'visible', + }); + + this.element_.classList.add('i-amphtml-amp-ad-sticky-layout'); + } + } + + /** + * Scroll promise for sticky ad + * @return {Promise} + */ + getScrollPromiseForStickyAd() { + if (this.isStickyAd_) { + return new Promise((resolve) => { + const unlisten = Services.viewportForDoc( + this.element_.getAmpDoc() + ).onScroll(() => { + resolve(); + unlisten(); + }); + }); + } + return Promise.resolve(null); + } + + /** + * 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; + } + } + + /** + * 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', () => { + Services.vsyncFor(this.baseInstance_.win).mutate(() => { + const viewport = Services.viewportForDoc(this.element_.getAmpDoc()); + viewport.removeFromFixedLayer(this.element); + removeElement(this.element_); + viewport.updatePaddingBottom(0); + }); + }) + ); + + closeButton.classList.add('amp-ad-close-button'); + this.element_.appendChild(closeButton); + } + /** * @param {number|string|undefined} height * @param {number|string|undefined} width @@ -217,6 +322,14 @@ export class AmpAdUIHandler { } ); } + + /** + * Clean up the listeners + */ + cleanup() { + this.unlisteners_.forEach((unlistener) => unlistener()); + this.unlisteners_.length = 0; + } } // Make the class available to other late loaded amp-ad implementations diff --git a/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js b/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js index 9b78b0080d19..c422194fe3f6 100644 --- a/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js +++ b/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js @@ -183,7 +183,7 @@ export class AmpAdXOriginIframeHandler { ) ); - if (this.baseInstance_.isStickyAd()) { + if (this.uiHandler_.isStickyAd()) { setStyle(iframe, 'pointer-events', 'none'); this.unlisteners_.push( listenFor( @@ -458,9 +458,7 @@ export class AmpAdXOriginIframeHandler { .updateSize(height, width, iframeHeight, iframeWidth, event) .then( (info) => { - if (this.baseInstance_.onResizeSuccess) { - this.baseInstance_.onResizeSuccess(); - } + this.uiHandler_.onResizeSuccess(); this.sendEmbedSizeResponse_( info.success, id, diff --git a/extensions/amp-ad/0.1/amp-ad.css b/extensions/amp-ad/0.1/amp-ad.css index 0b27fd3c5e61..e9f48c24fe0e 100644 --- a/extensions/amp-ad/0.1/amp-ad.css +++ b/extensions/amp-ad/0.1/amp-ad.css @@ -50,6 +50,10 @@ amp-embed iframe { border: 1px, solid, #696969; } +amp-ad[sticky] { + visibility: hidden; +} + amp-ad[type='adsense'], amp-ad[type='doubleclick'] { direction: ltr; 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 24fd1c980087..9dda633daa1a 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 @@ -20,6 +20,7 @@ import * as adCid from '../../../../src/ad-cid'; import * as consent from '../../../../src/consent'; import * as lolex from 'lolex'; import {AmpAd3PImpl} from '../amp-ad-3p-impl'; +import {AmpAdUIHandler} from '../amp-ad-ui'; import {CONSENT_POLICY_STATE} from '../../../../src/consent-state'; import {LayoutPriority} from '../../../../src/layout'; import {Services} from '../../../../src/services'; @@ -199,6 +200,7 @@ describes.realWin( adContainerElement.style.position = 'fixed'; win.document.body.appendChild(adContainerElement); const ad3p = createAmpAd(win); + ad3p.uiHandler = new AmpAdUIHandler(ad3p); adContainerElement.appendChild(ad3p.element); ad3p.onLayoutMeasure(); @@ -287,14 +289,15 @@ describes.realWin( describe('during layout', () => { it('sticky ad: should not layout w/o scroll', () => { - ad3p.isStickyAd_ = true; + ad3p.uiHandler.isStickyAd_ = true; + expect(ad3p.xOriginIframeHandler_).to.be.null; const layoutPromise = ad3p.layoutCallback(); return Promise.race([macroTask(), layoutPromise]) .then(() => { expect(ad3p.xOriginIframeHandler_).to.be.null; }) .then(() => { - ad3p.viewport_.scrollObservable_.fire(); + Services.viewportForDoc(env.ampdoc).scrollObservable_.fire(); return layoutPromise; }) .then(() => { diff --git a/extensions/amp-ad/0.1/test/test-amp-ad-ui.js b/extensions/amp-ad/0.1/test/test-amp-ad-ui.js index f18ad7980e34..256a19cc007f 100644 --- a/extensions/amp-ad/0.1/test/test-amp-ad-ui.js +++ b/extensions/amp-ad/0.1/test/test-amp-ad-ui.js @@ -321,5 +321,20 @@ describes.realWin( }); }); }); + + describe('sticky ads', () => { + it('should render close buttons on render once', () => { + expect(uiHandler.unlisteners_).to.be.empty; + uiHandler.isStickyAd_ = true; + uiHandler.onResizeSuccess(); + expect(uiHandler.closeButtonRendered_).to.be.true; + expect(uiHandler.unlisteners_.length).to.equal(1); + expect(uiHandler.element_.querySelector('.amp-ad-close-button')).to.be + .not.null; + + uiHandler.onResizeSuccess(); + expect(uiHandler.unlisteners_.length).to.equal(1); + }); + }); } ); diff --git a/extensions/amp-ad/0.1/test/test-amp-ad-xorigin-iframe-handler.js b/extensions/amp-ad/0.1/test/test-amp-ad-xorigin-iframe-handler.js index ae1e4d0c53fa..97c81b718139 100644 --- a/extensions/amp-ad/0.1/test/test-amp-ad-xorigin-iframe-handler.js +++ b/extensions/amp-ad/0.1/test/test-amp-ad-xorigin-iframe-handler.js @@ -59,10 +59,9 @@ describe('amp-ad-xorigin-iframe-handler', () => { adImpl.lifecycleReporter = { addPingsForVisibility: (unusedElement) => {}, }; - adImpl.isStickyAd = () => false; - adImpl.onResizeSuccess = window.sandbox.spy(); document.body.appendChild(adElement); adImpl.uiHandler = new AmpAdUIHandler(adImpl); + adImpl.uiHandler.onResizeSuccess = window.sandbox.spy(); iframeHandler = new AmpAdXOriginIframeHandler(adImpl); testIndex++; @@ -366,7 +365,7 @@ describe('amp-ad-xorigin-iframe-handler', () => { type: 'embed-size-changed', sentinel: 'amp3ptest' + testIndex, }); - expect(adImpl.onResizeSuccess).to.be.called; + expect(adImpl.uiHandler.onResizeSuccess).to.be.called; }); }); @@ -392,7 +391,7 @@ describe('amp-ad-xorigin-iframe-handler', () => { type: 'embed-size-changed', sentinel: 'amp3ptest' + testIndex, }); - expect(adImpl.onResizeSuccess).to.be.called; + expect(adImpl.uiHandler.onResizeSuccess).to.be.called; }); });