From 50fd083b464888a1a599818a8b41a75aa5f1b93b Mon Sep 17 00:00:00 2001 From: powerivq Date: Wed, 13 Apr 2022 11:10:47 -0700 Subject: [PATCH 1/2] Experiment: story auto advance and new cta button --- .../0.1/amp-ad-network-adsense-impl.js | 9 +++++++++ .../0.1/amp-ad-network-doubleclick-impl.js | 9 +++++++++ .../amp-story-auto-ads/0.1/amp-story-auto-ads.js | 8 +++++++- extensions/amp-story-auto-ads/0.1/story-ad-page.js | 12 ++++++++++-- extensions/amp-story-auto-ads/0.1/story-ad-ui.js | 13 +++++++++++-- .../0.1/test/test-story-ad-ui.js | 9 +++++++-- .../0.1/amp-story-open-page-attachment.css | 8 ++++++++ .../0.1/amp-story-open-page-attachment.js | 14 +++++++++++++- extensions/amp-story/1.0/amp-story-page.js | 12 ++++++++++-- extensions/amp-story/1.0/amp-story.js | 10 ++++++++-- extensions/amp-story/1.0/progress-bar.js | 11 +++++++++-- .../1.0/test/test-amp-story-progress-bar.js | 9 +++++++-- src/experiments/story-ad-progress-segment.js | 11 +++++++++++ 13 files changed, 119 insertions(+), 16 deletions(-) create mode 100644 src/experiments/story-ad-progress-segment.js 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 a7c9ce30ec12..0c149999803c 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 @@ -43,6 +43,7 @@ import { randomlySelectUnsetExperiments, } from '#experiments'; import {StoryAdPlacements} from '#experiments/story-ad-placements'; +import {StoryAdSegmentExp} from '#experiments/story-ad-progress-segment'; import {Services} from '#service'; import {Navigation} from '#service/navigation'; @@ -243,6 +244,14 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A { if (storyAdPlacementsExpId) { addExperimentIdToElement(storyAdPlacementsExpId, this.element); } + + const storyAdSegmentBranch = getExperimentBranch( + this.win, + StoryAdSegmentExp.ID + ); + if (storyAdSegmentBranch) { + addExperimentIdToElement(storyAdSegmentBranch, this.element); + } } /** 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 fc845e143eb1..c319fc677f1c 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 @@ -68,6 +68,7 @@ import { randomlySelectUnsetExperiments, } from '#experiments'; import {StoryAdPlacements} from '#experiments/story-ad-placements'; +import {StoryAdSegmentExp} from '#experiments/story-ad-progress-segment'; import {Services} from '#service'; import {Navigation} from '#service/navigation'; @@ -495,6 +496,14 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A { if (storyAdPlacementsExpId) { addExperimentIdToElement(storyAdPlacementsExpId, this.element); } + + const storyAdSegmentBranch = getExperimentBranch( + this.win, + StoryAdSegmentExp.ID + ); + if (storyAdSegmentBranch) { + addExperimentIdToElement(storyAdSegmentBranch, this.element); + } } /** diff --git a/extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.js b/extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.js index b4bf2ffae551..9cd394552411 100644 --- a/extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.js +++ b/extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.js @@ -2,6 +2,7 @@ import {CommonSignals_Enum} from '#core/constants/common-signals'; import {forceExperimentBranch} from '#experiments'; import {divertStoryAdPlacements} from '#experiments/story-ad-placements'; +import {StoryAdSegmentExp} from '#experiments/story-ad-progress-segment'; import {Services} from '#service'; @@ -43,7 +44,12 @@ const MUSTACHE_TAG = 'amp-mustache'; * @const {Object} * @visibleForTesting */ -export const RELEVANT_PLAYER_EXPS = {}; +export const RELEVANT_PLAYER_EXPS = { + [StoryAdSegmentExp.CONTROL]: StoryAdSegmentExp.ID, + [StoryAdSegmentExp.AUTO_ADVANCE_OLD_CTA]: StoryAdSegmentExp.ID, + [StoryAdSegmentExp.AUTO_ADVANCE_NEW_CTA]: StoryAdSegmentExp.ID, + [StoryAdSegmentExp.AUTO_ADVANCE_NEW_CTA_NOT_ANIMATED]: StoryAdSegmentExp.ID, +}; /** @enum {string} */ export const Attributes = { diff --git a/extensions/amp-story-auto-ads/0.1/story-ad-page.js b/extensions/amp-story-auto-ads/0.1/story-ad-page.js index ba286bdaa9e8..176f3c3cb327 100644 --- a/extensions/amp-story-auto-ads/0.1/story-ad-page.js +++ b/extensions/amp-story-auto-ads/0.1/story-ad-page.js @@ -9,7 +9,8 @@ import {setStyle} from '#core/dom/style'; import {map} from '#core/types/object'; import {parseJson} from '#core/types/object/json'; -import {isExperimentOn} from '#experiments'; +import {getExperimentBranch} from '#experiments'; +import {StoryAdSegmentExp} from '#experiments/story-ad-progress-segment'; import {getData, listen} from '#utils/event-helper'; import {dev, devAssert, userAssert} from '#utils/log'; @@ -313,7 +314,14 @@ export class StoryAdPage { 'id': this.id_, }; - if (isExperimentOn(this.win_, 'story-ad-auto-advance')) { + const storyAdSegmentBranch = getExperimentBranch( + this.win_, + StoryAdSegmentExp.ID + ); + if ( + storyAdSegmentBranch && + storyAdSegmentBranch != StoryAdSegmentExp.CONTROL + ) { attributes['auto-advance-after'] = '10s'; } diff --git a/extensions/amp-story-auto-ads/0.1/story-ad-ui.js b/extensions/amp-story-auto-ads/0.1/story-ad-ui.js index dccd2f8fde6e..7c0b096687be 100644 --- a/extensions/amp-story-auto-ads/0.1/story-ad-ui.js +++ b/extensions/amp-story-auto-ads/0.1/story-ad-ui.js @@ -2,7 +2,8 @@ import {createElementWithAttributes} from '#core/dom'; import {map} from '#core/types/object'; import {getWin} from '#core/window'; -import {isExperimentOn} from '#experiments'; +import {getExperimentBranch} from '#experiments'; +import {StoryAdSegmentExp} from '#experiments/story-ad-progress-segment'; import {Services} from '#service'; @@ -309,7 +310,15 @@ export function createCta(doc, buttonFitter, container, uiMetadata) { return null; } - if (isExperimentOn(doc.defaultView, 'story-ad-auto-advance')) { + const storyAdSegmentBranch = getExperimentBranch( + doc.defaultView, + StoryAdSegmentExp.ID + ); + if ( + storyAdSegmentBranch == StoryAdSegmentExp.AUTO_ADVANCE_NEW_CTA || + storyAdSegmentBranch == + StoryAdSegmentExp.AUTO_ADVANCE_NEW_CTA_NOT_ANIMATED + ) { return createPageOutlink_(doc, uiMetadata, container); } else { return createCtaLayer_(a, doc, container); diff --git a/extensions/amp-story-auto-ads/0.1/test/test-story-ad-ui.js b/extensions/amp-story-auto-ads/0.1/test/test-story-ad-ui.js index 5ffe2bf8ddf5..af06ebc5a14d 100644 --- a/extensions/amp-story-auto-ads/0.1/test/test-story-ad-ui.js +++ b/extensions/amp-story-auto-ads/0.1/test/test-story-ad-ui.js @@ -1,4 +1,5 @@ -import {toggleExperiment} from '#experiments'; +import {forceExperimentBranch} from '#experiments'; +import {StoryAdSegmentExp} from '#experiments/story-ad-progress-segment'; import {ButtonTextFitter} from '../story-ad-button-text-fitter'; import { @@ -248,7 +249,11 @@ describes.realWin('story-ad-ui', {amp: true}, (env) => { beforeEach(() => { buttonFitter = new ButtonTextFitter(env.ampdoc); - toggleExperiment(win, 'story-ad-auto-advance', true); + forceExperimentBranch( + win, + StoryAdSegmentExp.ID, + StoryAdSegmentExp.AUTO_ADVANCE_NEW_CTA + ); }); it('createCta page outlink custom theme element', () => { diff --git a/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.css b/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.css index f2488f3e6ae0..0e9daf6d64ff 100644 --- a/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.css +++ b/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.css @@ -168,6 +168,10 @@ animation: i-amphtml-tap-scale var(--i-amphtml-page-attachment-ui-animation-duration) var(--i-amphtml-page-attachment-ui-animation-delay) both !important; } +.i-amphtml-story-page-open-attachment-outlink-no-animation-exp.i-amphtml-story-page-open-attachment-outlink-no-animation-exp[active] { + animation: none !important; +} + .i-amphtml-story-outlink-page-attachment-arrow { display: block !important; cursor: pointer !important; @@ -180,6 +184,10 @@ animation: i-amphtml-move-up-arrow var(--i-amphtml-page-attachment-ui-animation-duration) var(--i-amphtml-page-attachment-ui-animation-delay) both !important; } +.i-amphtml-story-page-open-attachment.i-amphtml-story-page-open-attachment-outlink-no-animation-exp[active] .i-amphtml-story-outlink-page-attachment-arrow { + animation: none !important; +} + @keyframes i-amphtml-move-up-arrow { 0%, 100% { opacity: 1; diff --git a/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.js b/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.js index 13c95076d088..55f43006f6d9 100644 --- a/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.js +++ b/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.js @@ -1,11 +1,15 @@ /** * @fileoverview Helper for amp-story rendering of page-attachment UI. */ +import objStr from 'obj-str'; + import * as Preact from '#core/dom/jsx'; import {scopedQuerySelector} from '#core/dom/query'; import {computedStyle, setImportantStyles} from '#core/dom/style'; import {getWin} from '#core/window'; +import {getExperimentBranch} from '#experiments'; + import {Services} from '#service'; import {LocalizedStringId_Enum} from '#service/localization/strings'; @@ -128,13 +132,21 @@ const renderOutlinkUI = (pageEl, attachmentEl) => { // Set image. const openImgAttr = attachmentEl.getAttribute('cta-image'); + const noAnimation = + getExperimentBranch(getWin(pageEl), 'story-ad-auto-advance') == 'c'; + return openLabelOrFallback( pageEl, anchorChild?.textContent || ctaLabelFromAttr(attachmentEl) ).then((openLabel) => { const openAttachmentEl = ( { if ( // Do not show progress bar for the ad page (control group). // Show progress bar for the ad page (experiment group). (!this.isAdSegment_(id) || - isExperimentOn(this.win_, 'story-ad-auto-advance')) && + (storyAdSegmentBranch && + storyAdSegmentBranch != StoryAdSegmentExp.CONTROL)) && !(id in this.segmentIdMap_) ) { this.addSegment_(id); diff --git a/extensions/amp-story/1.0/test/test-amp-story-progress-bar.js b/extensions/amp-story/1.0/test/test-amp-story-progress-bar.js index 181fc1fa963d..e274808254f3 100644 --- a/extensions/amp-story/1.0/test/test-amp-story-progress-bar.js +++ b/extensions/amp-story/1.0/test/test-amp-story-progress-bar.js @@ -1,6 +1,7 @@ import {expect} from 'chai'; -import {toggleExperiment} from '#experiments'; +import {forceExperimentBranch} from '#experiments'; +import {StoryAdSegmentExp} from '#experiments/story-ad-progress-segment'; import {Services} from '#service'; @@ -45,7 +46,11 @@ describes.realWin('amp-story-progress-bar', {amp: true}, (env) => { describe('story ad progress segment', async () => { it('should create progress bar with ad pages', () => { - toggleExperiment(win, 'story-ad-auto-advance', true); + forceExperimentBranch( + win, + StoryAdSegmentExp.ID, + StoryAdSegmentExp.AUTO_ADVANCE_NEW_CTA + ); expect(doc.querySelector('.i-amphtml-story-ad-progress-value')).not.to .exist; progressBar.activeSegmentId_ = 'page-3'; diff --git a/src/experiments/story-ad-progress-segment.js b/src/experiments/story-ad-progress-segment.js new file mode 100644 index 000000000000..f9560213550f --- /dev/null +++ b/src/experiments/story-ad-progress-segment.js @@ -0,0 +1,11 @@ +/** + * @const + * @type {Object} + */ +export const StoryAdSegmentExp = { + ID: 'story-ad-auto-advance', + CONTROL: '31067116', // No change + AUTO_ADVANCE_OLD_CTA: '31067117', // Auto advance, old CTA button + AUTO_ADVANCE_NEW_CTA: '31067118', // Auto advance, new CTA button + AUTO_ADVANCE_NEW_CTA_NOT_ANIMATED: '31067119', // Auto advance, new CTA button but not animated +}; From 594e25b8a24335b964c5b406dc84067af00f1787 Mon Sep 17 00:00:00 2001 From: powerivq Date: Fri, 15 Apr 2022 13:34:56 -0700 Subject: [PATCH 2/2] Review comments --- .../0.1/amp-story-open-page-attachment.css | 12 ++---------- .../0.1/amp-story-open-page-attachment.js | 4 +++- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.css b/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.css index 0e9daf6d64ff..a5fcb1186dbb 100644 --- a/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.css +++ b/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.css @@ -164,14 +164,10 @@ --i-amphtml-outlink-cta-text-color: white !important; } -.i-amphtml-story-page-open-attachment-outlink[active] { +.i-amphtml-story-page-open-attachment-outlink[active]:not(.i-amphtml-story-page-open-attachment-outlink-no-animation-exp) { animation: i-amphtml-tap-scale var(--i-amphtml-page-attachment-ui-animation-duration) var(--i-amphtml-page-attachment-ui-animation-delay) both !important; } -.i-amphtml-story-page-open-attachment-outlink-no-animation-exp.i-amphtml-story-page-open-attachment-outlink-no-animation-exp[active] { - animation: none !important; -} - .i-amphtml-story-outlink-page-attachment-arrow { display: block !important; cursor: pointer !important; @@ -180,14 +176,10 @@ filter: drop-shadow(0px 2px 6px rgba(0, 0, 0, 0.3)) !important; } -.i-amphtml-story-page-open-attachment[active] .i-amphtml-story-outlink-page-attachment-arrow { +.i-amphtml-story-page-open-attachment[active]:not(.i-amphtml-story-page-open-attachment-outlink-no-animation-exp) .i-amphtml-story-outlink-page-attachment-arrow { animation: i-amphtml-move-up-arrow var(--i-amphtml-page-attachment-ui-animation-duration) var(--i-amphtml-page-attachment-ui-animation-delay) both !important; } -.i-amphtml-story-page-open-attachment.i-amphtml-story-page-open-attachment-outlink-no-animation-exp[active] .i-amphtml-story-outlink-page-attachment-arrow { - animation: none !important; -} - @keyframes i-amphtml-move-up-arrow { 0%, 100% { opacity: 1; diff --git a/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.js b/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.js index 55f43006f6d9..d92819ccb6a9 100644 --- a/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.js +++ b/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.js @@ -9,6 +9,7 @@ import {computedStyle, setImportantStyles} from '#core/dom/style'; import {getWin} from '#core/window'; import {getExperimentBranch} from '#experiments'; +import {StoryAdSegmentExp} from '#experiments/story-ad-progress-segment'; import {Services} from '#service'; import {LocalizedStringId_Enum} from '#service/localization/strings'; @@ -133,7 +134,8 @@ const renderOutlinkUI = (pageEl, attachmentEl) => { const openImgAttr = attachmentEl.getAttribute('cta-image'); const noAnimation = - getExperimentBranch(getWin(pageEl), 'story-ad-auto-advance') == 'c'; + getExperimentBranch(getWin(pageEl), 'story-ad-auto-advance') == + StoryAdSegmentExp.AUTO_ADVANCE_NEW_CTA_NOT_ANIMATED; return openLabelOrFallback( pageEl,