diff --git a/build-system/global-configs/canary-config.json b/build-system/global-configs/canary-config.json index 9f893f8976dc..cc59af44c670 100644 --- a/build-system/global-configs/canary-config.json +++ b/build-system/global-configs/canary-config.json @@ -17,6 +17,5 @@ "visibility-trigger-improvements": 1, "fie-resources": 1, "ads-initialIntersection": 1, - "amp-cid-backup": 1, - "sticky-ad-transition": 0.1 + "amp-cid-backup": 1 } diff --git a/build-system/global-configs/prod-config.json b/build-system/global-configs/prod-config.json index 51f8291abdbe..096b8b3f3f67 100644 --- a/build-system/global-configs/prod-config.json +++ b/build-system/global-configs/prod-config.json @@ -12,6 +12,5 @@ "intersect-resources": 0, "ios-fixed-no-transfer": 0, "fie-resources": 1, - "visibility-trigger-improvements": 1, - "sticky-ad-transition": 0.1 + "visibility-trigger-improvements": 1 } diff --git a/build-system/tasks/runtime-test/helpers-unit.js b/build-system/tasks/runtime-test/helpers-unit.js index 057857da29b2..b33284abfbf2 100644 --- a/build-system/tasks/runtime-test/helpers-unit.js +++ b/build-system/tasks/runtime-test/helpers-unit.js @@ -87,9 +87,7 @@ function extractCssJsFileMap() { */ function getImports(jsFile) { const jsFileContents = fs.readFileSync(jsFile, 'utf8'); - const {imports} = listImportsExports.parse(jsFileContents, [ - 'importAssertions', - ]); + const {imports} = listImportsExports.parse(jsFileContents); 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 8aecd690ec18..747cae24b46d 100644 --- a/extensions/amp-a4a/0.1/amp-a4a.js +++ b/extensions/amp-a4a/0.1/amp-a4a.js @@ -21,10 +21,6 @@ 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 { @@ -55,7 +51,6 @@ import { getConsentPolicyState, } from '../../../src/consent'; import {getContextMetadata} from '../../../src/iframe-attributes'; -import {getExperimentBranch, isExperimentOn} from '../../../src/experiments'; import {getMode} from '../../../src/mode'; import {insertAnalyticsElement} from '../../../src/extension-analytics'; import { @@ -69,6 +64,7 @@ import { } from '../../../src/utils/intersection'; import {isAdPositionAllowed} from '../../../src/ad-helper'; import {isArray, isEnumValue, isObject} from '../../../src/types'; +import {isExperimentOn} from '../../../src/experiments'; import {listenOnce} from '../../../src/event-helper'; import { observeWithSharedInOb, @@ -1768,6 +1764,7 @@ export class AmpA4A extends AMP.BaseElement { height, width ); + this.applyFillContent(this.iframe); let body = ''; const transferComplete = new Deferred(); @@ -1857,13 +1854,7 @@ export class AmpA4A extends AMP.BaseElement { 'title': this.getIframeTitle(), }) )); - divertStickyAdTransition(this.win); - if ( - getExperimentBranch(this.win, STICKY_AD_TRANSITION_EXP.id) !== - STICKY_AD_TRANSITION_EXP.experiment - ) { - this.applyFillContent(this.iframe); - } + 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 8c305f0553a4..ea7050cc427f 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,7 +44,6 @@ 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, @@ -227,16 +226,7 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A { * @visibleForTesting */ divertExperiments() { - const experimentInfoList = /** @type {!Array} */ ([ - { - experimentId: STICKY_AD_TRANSITION_EXP.id, - isTrafficEligible: () => true, - branches: [ - STICKY_AD_TRANSITION_EXP.control, - STICKY_AD_TRANSITION_EXP.experiment, - ], - }, - ]); + const experimentInfoList = /** @type {!Array} */ ([]); 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 ef12b11370ca..b207c7325d7b 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,7 +64,6 @@ 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 { @@ -107,7 +106,6 @@ 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'; @@ -485,14 +483,6 @@ 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 66ba9d454feb..d49cbe22817e 100644 --- a/extensions/amp-ad/0.1/amp-ad-ui.js +++ b/extensions/amp-ad/0.1/amp-ad-ui.js @@ -14,39 +14,21 @@ * 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 {setStyle, setStyles} from '../../../src/style'; +import {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 @@ -64,18 +46,10 @@ export class AmpAdUIHandler { this.containerElement_ = null; /** - * If this is a sticky ad unit, the sticky position option. - * @private {?StickyAdPositions} + * Whether this is a sticky ad unit. + * @private {boolean} */ - 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_); - } + this.isStickyAd_ = this.element_.hasAttribute('sticky'); /** * Whether the close button has been rendered for a sticky ad unit. @@ -204,26 +178,22 @@ export class AmpAdUIHandler { * @return {boolean} */ isStickyAd() { - return this.stickyAdPosition_ !== null; + return this.isStickyAd_; } /** * Initialize sticky ad related features */ maybeInitStickyAd() { - 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.' - ) - ); - } + if (this.isStickyAd_) { + setStyles(this.element_, { + position: 'fixed', + bottom: '0', + right: '0', + visibility: 'visible', + }); + + this.element_.classList.add('i-amphtml-amp-ad-sticky-layout'); } } @@ -232,7 +202,7 @@ export class AmpAdUIHandler { * @return {Promise} */ getScrollPromiseForStickyAd() { - if (this.isStickyAd()) { + if (this.isStickyAd_) { return new Promise((resolve) => { const unlisten = Services.viewportForDoc( this.element_.getAmpDoc() @@ -249,7 +219,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; } @@ -329,10 +299,10 @@ export class AmpAdUIHandler { } // Special case: for sticky ads, we enforce 20% size limit and 50% height limit - if (this.isStickyAd()) { + if (this.element_.hasAttribute('sticky')) { const viewport = this.baseInstance_.getViewport(); if ( - height * width > + newHeight * newWidth > STICKY_AD_MAX_SIZE_LIMIT * viewport.getHeight() * viewport.getWidth() || @@ -345,18 +315,7 @@ export class AmpAdUIHandler { return this.baseInstance_ .attemptChangeSize(newHeight, newWidth, event) .then( - () => { - 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, () => { resizeInfo.success = false; return resizeInfo; @@ -364,19 +323,6 @@ 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 0a83cb72c043..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 @@ -100,6 +100,7 @@ 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 e3ff90dca1c9..e9f48c24fe0e 100644 --- a/extensions/amp-ad/0.1/amp-ad.css +++ b/extensions/amp-ad/0.1/amp-ad.css @@ -52,7 +52,6 @@ amp-embed iframe { amp-ad[sticky] { visibility: hidden; - align-items: center; } amp-ad[type='adsense'], @@ -77,6 +76,10 @@ 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; @@ -116,42 +119,3 @@ 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 faa119d9933c..1d7f5cedf6b1 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.stickyAdPosition_ = 'bottom'; + ad3p.uiHandler.isStickyAd_ = true; 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 2bf13c45f10d..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 @@ -245,7 +245,6 @@ describes.realWin( height: '50px', }); env.win.document.body.appendChild(adElement); - env.sandbox.stub(uiHandler, 'setSize_'); env.sandbox .stub(adImpl, 'attemptChangeSize') .callsFake((height, width) => { @@ -263,7 +262,6 @@ describes.realWin( }); it('should tolerate string input', () => { - env.sandbox.stub(uiHandler, 'setSize_'); env.sandbox .stub(adImpl, 'attemptChangeSize') .callsFake((height, width) => { @@ -327,7 +325,7 @@ describes.realWin( describe('sticky ads', () => { it('should render close buttons on render once', () => { expect(uiHandler.unlisteners_).to.be.empty; - uiHandler.stickyAdPosition_ = 'bottom'; + uiHandler.isStickyAd_ = true; 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 f81a1b88f3f4..f994386f174c 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,14 +14,9 @@ * limitations under the License. */ -amp-ad[amp-story] iframe { - width: 100%; - height: 100%; -} - -.i-amphtml-story-desktop-panels amp-story-page[i-amphtml-loading][ad] { + .i-amphtml-story-desktop-panels amp-story-page[i-amphtml-loading][ad] { /* Move below viewport so that ad preloads */ - transform: scale(1) translateX(-100%) translateY(200%) !important; + transform: scale(1.0) translateX(-100%) translateY(200%) !important; } .i-amphtml-cta-container { @@ -48,23 +43,21 @@ 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'], -amp-story-page amp-ad[type='adsense'] { +amp-story-page amp-ad[data-a4a-upgrade-type="amp-ad-network-doubleclick-impl"] > iframe, +amp-story-page amp-ad[type="adsense"] > iframe { 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 { +.i-amphtml-story-desktop-fullbleed .i-amphtml-story-grid-template-fill > amp-ad > iframe { 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 64cbb36a29bf..0998ad1d8461 100644 --- a/package-lock.json +++ b/package-lock.json @@ -936,9 +936,9 @@ } }, "@babel/parser": { - "version": "7.12.10", - "resolved": "https://registry.npmjs.org/@babel/parser/-/parser-7.12.10.tgz", - "integrity": "sha512-PJdRPwyoOqFAWfLytxrWwGrAxghCgh/yTNCYciOz8QgjflA7aZhECPZAa2VUedKg2+QMWkI0L9lynh2SNmNEgA==", + "version": "7.11.5", + "resolved": "https://registry.npmjs.org/@babel/parser/-/parser-7.11.5.tgz", + "integrity": "sha512-X9rD8qqm695vgmeaQ4fvz/o3+Wk4ZzQvSHkDBgpYKxpD4qTAUm88ZKtHkVqIOsYFFbIQ6wQYhC6q7pjqVK0E0Q==", "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 deleted file mode 100644 index d9980177ded1..000000000000 --- a/src/experiments/sticky-ad-transition-exp.js +++ /dev/null @@ -1,42 +0,0 @@ -/** - * 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); -}