Skip to content

Commit

Permalink
Create an experiment to turn amp-sticky-ad to amp-ad sticky implement…
Browse files Browse the repository at this point in the history
…ation (#35302)

* Create an experiment to turn amp-sticky-ad to amp-ad sticky implementation

* Do not call upgrade manually
  • Loading branch information
powerivq committed Jul 22, 2021
1 parent 027d05c commit 7b1df94
Show file tree
Hide file tree
Showing 10 changed files with 63 additions and 16 deletions.
3 changes: 2 additions & 1 deletion build-system/global-configs/canary-config.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@
"story-ad-auto-advance": 1,
"story-ad-placements": 1,
"story-load-first-page-only": 0.1,
"amp-story-page-attachment-ui-v2": 1
"amp-story-page-attachment-ui-v2": 1,
"amp-sticky-ad-to-amp-ad": 0
}
3 changes: 2 additions & 1 deletion build-system/global-configs/prod-config.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@
"amp-cid-backup": 1,
"story-ad-placements": 0.01,
"story-load-first-page-only": 0.1,
"amp-story-page-attachment-ui-v2": 1
"amp-story-page-attachment-ui-v2": 1,
"amp-sticky-ad-to-amp-ad": 0
}
1 change: 1 addition & 0 deletions build-system/test-configs/forbidden-terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -862,6 +862,7 @@ const forbiddenTermsSrcInclusive = {
allowlist: [
'src/service/extensions-impl.js',
'extensions/amp-ad/0.1/amp-ad.js',
'extensions/amp-sticky-ad/1.0/amp-sticky-ad.js',
],
},
'reject\\(\\)': {
Expand Down
5 changes: 4 additions & 1 deletion extensions/amp-a4a/0.1/amp-a4a.js
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,10 @@ export class AmpA4A extends AMP.BaseElement {
);
return false;
}
if (!isAdPositionAllowed(this.element, this.win)) {
if (
!this.uiHandler.isStickyAd() &&
!isAdPositionAllowed(this.element, this.win)
) {
user().warn(
TAG,
`<${this.element.tagName}> is not allowed to be ` +
Expand Down
6 changes: 5 additions & 1 deletion extensions/amp-a4a/0.1/test/test-amp-a4a.js
Original file line number Diff line number Diff line change
Expand Up @@ -1702,12 +1702,12 @@ describes.realWin('amp-a4a', {amp: true}, (env) => {
env.sandbox
.stub(a4a, 'maybeValidateAmpCreative')
.returns(Promise.resolve());
a4a.onLayoutMeasure();
a4a.uiHandler = {
getScrollPromiseForStickyAd: () => Promise.resolve(null),
isStickyAd: () => false,
maybeInitStickyAd: () => {},
};
a4a.onLayoutMeasure();
await a4a.buildCallback();
await a4a.layoutCallback();
expect(
Expand Down Expand Up @@ -1781,6 +1781,9 @@ describes.realWin('amp-a4a', {amp: true}, (env) => {
doc.head.appendChild(s);
a4aElement.className = 'fixed';
const a4a = new MockA4AImpl(a4aElement);
a4a.uiHandler = {
isStickyAd: () => false,
};
a4a.onLayoutMeasure();
expect(a4a.adPromise_).to.not.be.ok;
});
Expand Down Expand Up @@ -2141,6 +2144,7 @@ describes.realWin('amp-a4a', {amp: true}, (env) => {
unlayoutUISpy();
},
getScrollPromiseForStickyAd: () => Promise.resolve(null),
isStickyAd: () => false,
maybeInitStickyAd: () => {},
cleanup: () => {},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,9 @@ describes.realWin('DoubleClick Fast Fetch Fluid', realWinConfig, (env) => {
});

it('should contain sz=320x50 in ad request by default', () => {
impl.uiHandler = {
isStickyAd: () => false,
};
impl.initiateAdRequest();
return impl.adPromise_.then(() => {
expect(impl.adUrl_).to.be.ok;
Expand All @@ -196,6 +199,9 @@ describes.realWin('DoubleClick Fast Fetch Fluid', realWinConfig, (env) => {
});

it('should contain mulitple sizes in ad request', () => {
multiSizeImpl.uiHandler = {
isStickyAd: () => false,
};
multiSizeImpl.initiateAdRequest();
return multiSizeImpl.adPromise_.then(() => {
expect(multiSizeImpl.adUrl_).to.be.ok;
Expand Down
10 changes: 5 additions & 5 deletions extensions/amp-ad/0.1/amp-ad-ui.js
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,11 @@ export class AmpAdUIHandler {
)
);
}

if (!this.closeButtonRendered_) {
this.addCloseButton_();
this.closeButtonRendered_ = true;
}
}
}

Expand All @@ -286,11 +291,6 @@ 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_) {
this.addCloseButton_();
this.closeButtonRendered_ = true;
}

if (this.isStickyAd() && !this.topStickyAdScrollListener_) {
const doc = this.element_.getAmpDoc();
this.topStickyAdScrollListener_ = Services.viewportForDoc(doc).onScroll(
Expand Down
2 changes: 2 additions & 0 deletions extensions/amp-ad/0.1/amp-ad.css
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,9 @@ amp-ad[sticky='top'] {
}

amp-ad[sticky='bottom'] {
max-width: 100%;
width: 100% !important;
max-height: 20%;
padding-bottom: env(safe-area-inset-bottom, 0px);
background: #fff;
bottom: 0;
Expand Down
10 changes: 3 additions & 7 deletions extensions/amp-ad/0.1/test/test-amp-ad-ui.js
Original file line number Diff line number Diff line change
Expand Up @@ -326,17 +326,13 @@ describes.realWin(
});

describe('sticky ads', () => {
it('should render close buttons on render once', () => {
it('should render close buttons', () => {
expect(uiHandler.unlisteners_).to.be.empty;
uiHandler.stickyAdPosition_ = 'bottom';
uiHandler.onResizeSuccess();
expect(uiHandler.closeButtonRendered_).to.be.true;
expect(uiHandler.unlisteners_.length).to.equal(2);
uiHandler.maybeInitStickyAd();
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(2);
});

it('onResizeSuccess top sticky ads shall cause padding top adjustment', () => {
Expand Down
33 changes: 33 additions & 0 deletions extensions/amp-sticky-ad/1.0/amp-sticky-ad.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@
import {CSS} from '../../../build/amp-sticky-ad-1.0.css';
import {CommonSignals} from '#core/constants/common-signals';
import {Services} from '#service';
import {addExperimentIdToElement} from '#ads/google/a4a/traffic-experiments';
import {
computedStyle,
removeAlphaFromColor,
setStyle,
toggle,
} from '#core/dom/style';
import {dev, user, userAssert} from '../../../src/log';
import {isExperimentOn} from '#experiments';
import {realChildElements} from '#core/dom/query';
import {removeElement} from '#core/dom';
import {whenUpgradedToCustomElement} from '../../../src/amp-element-helpers';
Expand Down Expand Up @@ -112,6 +114,37 @@ class AmpStickyAd extends AMP.BaseElement {
return Promise.resolve();
}

/** @override */
upgradeCallback() {
if (!isExperimentOn(this.win, 'amp-sticky-ad-to-amp-ad')) {
return null;
}

const children = realChildElements(this.element);
userAssert(
children.length == 1 && children[0].tagName == 'AMP-AD',
'amp-sticky-ad must have a single amp-ad child'
);

const ad = children[0];
const enableConversion = Math.random() < 0.5;

const adType = (ad.getAttribute('type') || '').toLowerCase();
if (adType == 'doubleclick' || adType == 'adsense') {
addExperimentIdToElement(enableConversion ? '31061856' : '31061855', ad);
}

if (!enableConversion) {
return null;
}

ad.setAttribute('sticky', 'bottom');
this.element.parentElement.replaceChild(ad, this.element);
return Services.extensionsFor(this.win)
.loadElementClass('amp-ad', '0.1')
.then((AmpAd) => new AmpAd(ad));
}

/** @override */
isAlwaysFixed() {
return true;
Expand Down

0 comments on commit 7b1df94

Please sign in to comment.