From 5f430805d8fa38682abdca74e60821d7e8628a72 Mon Sep 17 00:00:00 2001 From: Shihua Zheng Date: Thu, 7 Jan 2021 13:54:47 -0800 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Implement=20sticky=20ad=20bottom=20?= =?UTF-8?q?type=20ad=20on=20amp-ad=20(#31491)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add 3 sticky ad positions to amp-ad * Implement sticky ad bottom type ad * Updates * Upgrade @babel/parser to support import assertions Not sure why this version is stale. v 7.11 uses `with` syntax, which was changed to `assert` syntax. * Fix style Co-authored-by: Justin Ridgewell --- .../global-configs/canary-config.json | 3 +- build-system/global-configs/prod-config.json | 3 +- .../tasks/runtime-test/helpers-unit.js | 4 +- extensions/amp-a4a/0.1/amp-a4a.js | 14 ++- .../0.1/amp-ad-network-adsense-impl.js | 12 ++- .../0.1/amp-ad-network-doubleclick-impl.js | 10 ++ extensions/amp-ad/0.1/amp-ad-ui.js | 92 +++++++++++++++---- .../0.1/amp-ad-xorigin-iframe-handler.js | 1 - extensions/amp-ad/0.1/amp-ad.css | 44 ++++++++- .../amp-ad/0.1/test/test-amp-ad-3p-impl.js | 2 +- extensions/amp-ad/0.1/test/test-amp-ad-ui.js | 4 +- .../0.1/amp-story-auto-ads.css | 21 +++-- package-lock.json | 6 +- src/experiments/sticky-ad-transition-exp.js | 42 +++++++++ 14 files changed, 216 insertions(+), 42 deletions(-) create mode 100644 src/experiments/sticky-ad-transition-exp.js diff --git a/build-system/global-configs/canary-config.json b/build-system/global-configs/canary-config.json index 3758691b0eff..3e54b7e2d03f 100644 --- a/build-system/global-configs/canary-config.json +++ b/build-system/global-configs/canary-config.json @@ -22,5 +22,6 @@ "build-in-chunks": 1, "visibility-trigger-improvements": 1, "fie-resources": 1, - "amp-cid-backup": 1 + "amp-cid-backup": 1, + "sticky-ad-transition": 0.1 } diff --git a/build-system/global-configs/prod-config.json b/build-system/global-configs/prod-config.json index fe74fafe0d00..14a2e67d7649 100644 --- a/build-system/global-configs/prod-config.json +++ b/build-system/global-configs/prod-config.json @@ -18,5 +18,6 @@ "ios-fixed-no-transfer": 0, "pump-early-frame": 1, "fie-resources": 0.1, - "visibility-trigger-improvements": 1 + "visibility-trigger-improvements": 1, + "sticky-ad-transition": 0.1 } diff --git a/build-system/tasks/runtime-test/helpers-unit.js b/build-system/tasks/runtime-test/helpers-unit.js index 28f46f91f07f..d85489a3ae2f 100644 --- a/build-system/tasks/runtime-test/helpers-unit.js +++ b/build-system/tasks/runtime-test/helpers-unit.js @@ -87,7 +87,9 @@ function extractCssJsFileMap() { */ function getImports(jsFile) { const jsFileContents = fs.readFileSync(jsFile, 'utf8'); - const {imports} = listImportsExports.parse(jsFileContents); + const {imports} = listImportsExports.parse(jsFileContents, [ + 'importAssertions', + ]); const files = []; const jsFileDir = path.dirname(jsFile); imports.forEach(function (file) { diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js index c8b7ee5068d5..7bb82cb9a300 100644 --- a/extensions/amp-a4a/0.1/amp-a4a.js +++ b/extensions/amp-a4a/0.1/amp-a4a.js @@ -21,6 +21,10 @@ import {DetachedDomStream} from '../../../src/utils/detached-dom-stream'; import {DomTransformStream} from '../../../src/utils/dom-tranform-stream'; import {GEO_IN_GROUP} from '../../amp-geo/0.1/amp-geo-in-group'; import {Layout, LayoutPriority, isLayoutSizeDefined} from '../../../src/layout'; +import { + STICKY_AD_TRANSITION_EXP, + divertStickyAdTransition, +} from '../../../src/experiments/sticky-ad-transition-exp'; import {Services} from '../../../src/services'; import {SignatureVerifier, VerificationStatus} from './signature-verifier'; import { @@ -51,6 +55,7 @@ import { getConsentPolicyState, } from '../../../src/consent'; import {getContextMetadata} from '../../../src/iframe-attributes'; +import {getExperimentBranch} from '../../../src/experiments'; import {getMode} from '../../../src/mode'; import {insertAnalyticsElement} from '../../../src/extension-analytics'; import { @@ -1759,7 +1764,6 @@ export class AmpA4A extends AMP.BaseElement { height, width ); - this.applyFillContent(this.iframe); let body = ''; const transferComplete = new Deferred(); @@ -1848,7 +1852,13 @@ export class AmpA4A extends AMP.BaseElement { 'title': this.getIframeTitle(), }) )); - this.applyFillContent(this.iframe); + divertStickyAdTransition(this.win); + if ( + getExperimentBranch(this.win, STICKY_AD_TRANSITION_EXP.id) !== + STICKY_AD_TRANSITION_EXP.experiment + ) { + this.applyFillContent(this.iframe); + } const fontsArray = []; if (creativeMetaData.customStylesheets) { creativeMetaData.customStylesheets.forEach((s) => { diff --git a/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js b/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js index ea7050cc427f..8c305f0553a4 100644 --- a/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js +++ b/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js @@ -44,6 +44,7 @@ import { maybeAppendErrorParameter, } from '../../../ads/google/a4a/utils'; import {ResponsiveState} from './responsive-state'; +import {STICKY_AD_TRANSITION_EXP} from '../../../src/experiments/sticky-ad-transition-exp'; import {Services} from '../../../src/services'; import { addAmpExperimentIdToElement, @@ -226,7 +227,16 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A { * @visibleForTesting */ divertExperiments() { - const experimentInfoList = /** @type {!Array} */ ([]); + const experimentInfoList = /** @type {!Array} */ ([ + { + experimentId: STICKY_AD_TRANSITION_EXP.id, + isTrafficEligible: () => true, + branches: [ + STICKY_AD_TRANSITION_EXP.control, + STICKY_AD_TRANSITION_EXP.experiment, + ], + }, + ]); const setExps = randomlySelectUnsetExperiments( this.win, experimentInfoList diff --git a/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js b/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js index b207c7325d7b..ef12b11370ca 100644 --- a/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js +++ b/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js @@ -64,6 +64,7 @@ import { RefreshManager, // eslint-disable-line no-unused-vars getRefreshManager, } from '../../amp-a4a/0.1/refresh-manager'; +import {STICKY_AD_TRANSITION_EXP} from '../../../src/experiments/sticky-ad-transition-exp'; import {SafeframeHostApi} from './safeframe-host'; import {Services} from '../../../src/services'; import { @@ -106,6 +107,7 @@ import { } from '../../../src/experiments'; import {getMode} from '../../../src/mode'; import {getMultiSizeDimensions} from '../../../ads/google/utils'; + import {getOrCreateAdCid} from '../../../src/ad-cid'; import {getPageLayoutBoxBlocking} from '../../../src/utils/page-layout-box'; @@ -483,6 +485,14 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A { }, branches: Object.values(IDLE_CWV_EXP_BRANCHES), }, + { + experimentId: STICKY_AD_TRANSITION_EXP.id, + isTrafficEligible: () => true, + branches: [ + STICKY_AD_TRANSITION_EXP.control, + STICKY_AD_TRANSITION_EXP.experiment, + ], + }, ]); const setExps = this.randomlySelectUnsetExperiments_(experimentInfoList); Object.keys(setExps).forEach( diff --git a/extensions/amp-ad/0.1/amp-ad-ui.js b/extensions/amp-ad/0.1/amp-ad-ui.js index d49cbe22817e..66ba9d454feb 100644 --- a/extensions/amp-ad/0.1/amp-ad-ui.js +++ b/extensions/amp-ad/0.1/amp-ad-ui.js @@ -14,21 +14,39 @@ * limitations under the License. */ +import { + STICKY_AD_TRANSITION_EXP, + divertStickyAdTransition, +} from '../../../src/experiments/sticky-ad-transition-exp'; import {Services} from '../../../src/services'; import { ancestorElementsByTag, createElementWithAttributes, removeElement, } from '../../../src/dom'; +import {devAssert} from '../../../src/log'; import {dict} from '../../../src/utils/object'; +import {getExperimentBranch} from '../../../src/experiments'; import {getAdContainer} from '../../../src/ad-helper'; import {listen} from '../../../src/event-helper'; -import {setStyles} from '../../../src/style'; +import {setStyle, setStyles} from '../../../src/style'; const STICKY_AD_MAX_SIZE_LIMIT = 0.2; const STICKY_AD_MAX_HEIGHT_LIMIT = 0.5; +/** + * Permissible sticky ad options. + * @const @enum {string} + */ +const StickyAdPositions = { + TOP: 'top', + BOTTOM: 'bottom', + BOTTOM_RIGHT: 'bottom-right', +}; + +const STICKY_AD_PROP = 'sticky'; + export class AmpAdUIHandler { /** * @param {!AMP.BaseElement} baseInstance @@ -46,10 +64,18 @@ export class AmpAdUIHandler { this.containerElement_ = null; /** - * Whether this is a sticky ad unit. - * @private {boolean} + * If this is a sticky ad unit, the sticky position option. + * @private {?StickyAdPositions} */ - this.isStickyAd_ = this.element_.hasAttribute('sticky'); + this.stickyAdPosition_ = null; + if (this.element_.hasAttribute(STICKY_AD_PROP)) { + // TODO(powerivq@) Kargo is currently running an experiment using empty sticky attribute, so + // we default the position to bottom right. Remove this default afterwards. + this.stickyAdPosition_ = + this.element_.getAttribute(STICKY_AD_PROP) || + StickyAdPositions.BOTTOM_RIGHT; + this.element_.setAttribute(STICKY_AD_PROP, this.stickyAdPosition_); + } /** * Whether the close button has been rendered for a sticky ad unit. @@ -178,22 +204,26 @@ export class AmpAdUIHandler { * @return {boolean} */ isStickyAd() { - return this.isStickyAd_; + return this.stickyAdPosition_ !== null; } /** * 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'); + if (this.isStickyAd()) { + setStyle(this.element_, 'visibility', 'visible'); + + if (this.stickyAdPosition_ == StickyAdPositions.BOTTOM) { + const paddingBar = this.doc_.createElement('amp-ad-sticky-padding'); + this.element_.insertBefore( + paddingBar, + devAssert( + this.element_.firstChild, + 'amp-ad should have been expanded.' + ) + ); + } } } @@ -202,7 +232,7 @@ export class AmpAdUIHandler { * @return {Promise} */ getScrollPromiseForStickyAd() { - if (this.isStickyAd_) { + if (this.isStickyAd()) { return new Promise((resolve) => { const unlisten = Services.viewportForDoc( this.element_.getAmpDoc() @@ -219,7 +249,7 @@ export class AmpAdUIHandler { * When a sticky ad is shown, the close button should be rendered at the same time. */ onResizeSuccess() { - if (this.isStickyAd_ && !this.closeButtonRendered_) { + if (this.isStickyAd() && !this.closeButtonRendered_) { this.addCloseButton_(); this.closeButtonRendered_ = true; } @@ -299,10 +329,10 @@ export class AmpAdUIHandler { } // Special case: for sticky ads, we enforce 20% size limit and 50% height limit - if (this.element_.hasAttribute('sticky')) { + if (this.isStickyAd()) { const viewport = this.baseInstance_.getViewport(); if ( - newHeight * newWidth > + height * width > STICKY_AD_MAX_SIZE_LIMIT * viewport.getHeight() * viewport.getWidth() || @@ -315,7 +345,18 @@ export class AmpAdUIHandler { return this.baseInstance_ .attemptChangeSize(newHeight, newWidth, event) .then( - () => resizeInfo, + () => { + divertStickyAdTransition(this.baseInstance_.win); + if ( + getExperimentBranch( + this.baseInstance_.win, + STICKY_AD_TRANSITION_EXP.id + ) === STICKY_AD_TRANSITION_EXP.experiment + ) { + this.setSize_(this.element_.querySelector('iframe'), height, width); + } + return resizeInfo; + }, () => { resizeInfo.success = false; return resizeInfo; @@ -323,6 +364,19 @@ export class AmpAdUIHandler { ); } + /** + * Force set the dimensions for an element + * @param {Any} element + * @param {number} newHeight + * @param {number} newWidth + */ + setSize_(element, newHeight, newWidth) { + setStyles(element, { + 'height': `${newHeight}px`, + 'width': `${newWidth}px`, + }); + } + /** * Clean up the listeners */ 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 c422194fe3f6..0a83cb72c043 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 @@ -100,7 +100,6 @@ export class AmpAdXOriginIframeHandler { devAssert(!this.iframe, 'multiple invocations of init without destroy!'); this.iframe = iframe; this.iframe.setAttribute('scrolling', 'no'); - this.baseInstance_.applyFillContent(this.iframe); const timer = Services.timerFor(this.baseInstance_.win); // Init the legacy observeInterection API service. diff --git a/extensions/amp-ad/0.1/amp-ad.css b/extensions/amp-ad/0.1/amp-ad.css index e9f48c24fe0e..e3ff90dca1c9 100644 --- a/extensions/amp-ad/0.1/amp-ad.css +++ b/extensions/amp-ad/0.1/amp-ad.css @@ -52,6 +52,7 @@ amp-embed iframe { amp-ad[sticky] { visibility: hidden; + align-items: center; } amp-ad[type='adsense'], @@ -76,10 +77,6 @@ amp-ad[data-a4a-upgrade-type='amp-ad-network-doubleclick-impl'][height='fluid'] width: 100% !important; } -amp-ad.i-amphtml-amp-ad-sticky-layout { - overflow: visible !important; -} - amp-ad .amp-ad-close-button { position: absolute; visibility: visible; @@ -119,3 +116,42 @@ amp-ad .amp-ad-close-button:before { right: -20px; left: 0; } + +amp-ad-sticky-padding { + display: block; + width: 100% !important; + background: #fff; + height: 4px; + max-height: 5px !important; + overflow-x: hidden; + overflow-y: hidden; + /** Must be above the dismiss button to cover bottom shadow */ + z-index: 12; +} + +amp-ad[sticky] { + z-index: 11; + position: fixed; + overflow: visible !important; + padding-bottom: env(safe-area-inset-bottom, 0px); + box-shadow: 0 0 5px 0 rgba(0, 0, 0, 0.2) !important; + display: flex; + flex-direction: column; +} + +amp-ad[sticky='top'] { + width: 100% !important; + background: #fff; + top: 0; +} + +amp-ad[sticky='bottom'] { + width: 100% !important; + background: #fff; + bottom: 0; +} + +amp-ad[sticky='bottom-right'] { + bottom: 0; + right: 0; +} 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 1d7f5cedf6b1..faa119d9933c 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 @@ -289,7 +289,7 @@ describes.realWin( describe('during layout', () => { it('sticky ad: should not layout w/o scroll', () => { - ad3p.uiHandler.isStickyAd_ = true; + ad3p.uiHandler.stickyAdPosition_ = 'bottom'; expect(ad3p.xOriginIframeHandler_).to.be.null; const layoutPromise = ad3p.layoutCallback(); return Promise.race([macroTask(), layoutPromise]) 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 256a19cc007f..2bf13c45f10d 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 @@ -245,6 +245,7 @@ describes.realWin( height: '50px', }); env.win.document.body.appendChild(adElement); + env.sandbox.stub(uiHandler, 'setSize_'); env.sandbox .stub(adImpl, 'attemptChangeSize') .callsFake((height, width) => { @@ -262,6 +263,7 @@ describes.realWin( }); it('should tolerate string input', () => { + env.sandbox.stub(uiHandler, 'setSize_'); env.sandbox .stub(adImpl, 'attemptChangeSize') .callsFake((height, width) => { @@ -325,7 +327,7 @@ describes.realWin( describe('sticky ads', () => { it('should render close buttons on render once', () => { expect(uiHandler.unlisteners_).to.be.empty; - uiHandler.isStickyAd_ = true; + uiHandler.stickyAdPosition_ = 'bottom'; uiHandler.onResizeSuccess(); expect(uiHandler.closeButtonRendered_).to.be.true; expect(uiHandler.unlisteners_.length).to.equal(1); diff --git a/extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.css b/extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.css index f994386f174c..f81a1b88f3f4 100644 --- a/extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.css +++ b/extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.css @@ -14,9 +14,14 @@ * limitations under the License. */ - .i-amphtml-story-desktop-panels amp-story-page[i-amphtml-loading][ad] { +amp-ad[amp-story] iframe { + width: 100%; + height: 100%; +} + +.i-amphtml-story-desktop-panels amp-story-page[i-amphtml-loading][ad] { /* Move below viewport so that ad preloads */ - transform: scale(1.0) translateX(-100%) translateY(200%) !important; + transform: scale(1) translateX(-100%) translateY(200%) !important; } .i-amphtml-cta-container { @@ -43,21 +48,23 @@ amp-story-page[xdomain-ad] .i-amphtml-glass-pane { } /* TODO(ccordry): refactor centering logic in amp-ad.css and remove this hack. */ -amp-story-page amp-ad[data-a4a-upgrade-type="amp-ad-network-doubleclick-impl"] > iframe, -amp-story-page amp-ad[type="adsense"] > iframe { +amp-story-page amp-ad[data-a4a-upgrade-type='amp-ad-network-doubleclick-impl'], +amp-story-page amp-ad[type='adsense'] { top: 0 !important; left: 0 !important; transform: translate(0) !important; } /* TODO(ccordry) allow advertisers to opt-in to fullscreen ads. */ -.i-amphtml-story-desktop-fullbleed .i-amphtml-story-grid-template-fill > amp-ad > iframe { +.i-amphtml-story-desktop-fullbleed + .i-amphtml-story-grid-template-fill + > amp-ad { left: 50% !important; right: auto !important; margin: auto !important; min-height: 75vh !important; max-height: 75vh !important; - min-width: calc(3/5 * 75vh) !important; - max-width: calc(3/5 * 75vh) !important; + min-width: calc(3 / 5 * 75vh) !important; + max-width: calc(3 / 5 * 75vh) !important; transform: translateX(-50%) !important; } diff --git a/package-lock.json b/package-lock.json index 1cc1adf31e86..4beb1bb88187 100644 --- a/package-lock.json +++ b/package-lock.json @@ -936,9 +936,9 @@ } }, "@babel/parser": { - "version": "7.11.5", - "resolved": "https://registry.npmjs.org/@babel/parser/-/parser-7.11.5.tgz", - "integrity": "sha512-X9rD8qqm695vgmeaQ4fvz/o3+Wk4ZzQvSHkDBgpYKxpD4qTAUm88ZKtHkVqIOsYFFbIQ6wQYhC6q7pjqVK0E0Q==", + "version": "7.12.10", + "resolved": "https://registry.npmjs.org/@babel/parser/-/parser-7.12.10.tgz", + "integrity": "sha512-PJdRPwyoOqFAWfLytxrWwGrAxghCgh/yTNCYciOz8QgjflA7aZhECPZAa2VUedKg2+QMWkI0L9lynh2SNmNEgA==", "dev": true }, "@babel/plugin-proposal-async-generator-functions": { diff --git a/src/experiments/sticky-ad-transition-exp.js b/src/experiments/sticky-ad-transition-exp.js new file mode 100644 index 000000000000..d9980177ded1 --- /dev/null +++ b/src/experiments/sticky-ad-transition-exp.js @@ -0,0 +1,42 @@ +/** + * Copyright 2020 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import {randomlySelectUnsetExperiments} from '../experiments'; + +/** @const @enum{string} */ +export const STICKY_AD_TRANSITION_EXP = { + id: 'sticky-ad-transition', + control: '21069722', + experiment: '21069723', +}; + +/** + * Select exp vs control for sticky-ad-transition. + * @param {!Window} win + */ +export function divertStickyAdTransition(win) { + const expInfoList = /** @type {!Array} */ ([ + { + experimentId: STICKY_AD_TRANSITION_EXP.id, + isTrafficEligible: () => true, + branches: [ + STICKY_AD_TRANSITION_EXP.control, + STICKY_AD_TRANSITION_EXP.experiment, + ], + }, + ]); + randomlySelectUnsetExperiments(win, expInfoList); +}