Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force amp sticky ad layout and move scroll listener to earlier stage #37456

Merged
merged 1 commit into from
Jan 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions extensions/amp-a4a/0.1/amp-a4a.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import {
getDefaultBootstrapBaseUrl,
} from '../../../src/3p-frame';
import {isAdPositionAllowed} from '../../../src/ad-helper';
import {ChunkPriority_Enum, chunk} from '../../../src/chunk';
import {
getConsentMetadata,
getConsentPolicyInfo,
Expand Down Expand Up @@ -408,6 +409,14 @@ export class AmpA4A extends AMP.BaseElement {
this.uiHandler = new AMP.AmpAdUIHandler(this);
this.uiHandler.validateStickyAd();

this.uiHandler
.getScrollPromiseForStickyAd()
.then(() => this.uiHandler.maybeInitStickyAd());

if (this.uiHandler.isStickyAd()) {
chunk(this.element, () => this.layoutCallback(), ChunkPriority_Enum.LOW);
}

// Disable crypto key fetching if we are not going to use it in no-signing path.
// TODO(ccordry): clean up with no-signing launch.
if (!this.isInNoSigningExp()) {
Expand Down Expand Up @@ -1340,15 +1349,10 @@ export class AmpA4A extends AMP.BaseElement {
const checkStillCurrent = this.verifyStillCurrent();
// Promise chain will have determined if creative is valid AMP.

return Promise.all([
this.adPromise_,
this.uiHandler.getScrollPromiseForStickyAd(),
])
.then((values) => {
return this.adPromise_
.then((creativeMetaData) => {
checkStillCurrent();

this.uiHandler.maybeInitStickyAd();
const creativeMetaData = values[0];
if (this.isCollapsed_) {
return Promise.resolve();
}
Expand Down
18 changes: 11 additions & 7 deletions extensions/amp-ad/0.1/amp-ad-3p-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
import {getIframe, preloadBootstrap} from '../../../src/3p-frame';
import {getAdCid} from '../../../src/ad-cid';
import {getAdContainer, isAdPositionAllowed} from '../../../src/ad-helper';
import {ChunkPriority_Enum, chunk} from '../../../src/chunk';
import {
getConsentMetadata,
getConsentPolicyInfo,
Expand Down Expand Up @@ -194,6 +195,15 @@ export class AmpAd3PImpl extends AMP.BaseElement {
this.uiHandler = new AmpAdUIHandler(this);
this.uiHandler.validateStickyAd();

// For sticky ad only: must wait for scrolling event before loading the ad
this.uiHandler
.getScrollPromiseForStickyAd()
.then(() => this.uiHandler.maybeInitStickyAd());

if (this.uiHandler.isStickyAd()) {
chunk(this.element, () => this.layoutCallback(), ChunkPriority_Enum.LOW);
}

this.isFullWidthRequested_ = this.shouldRequestFullWidth_();

if (this.isFullWidthRequested_) {
Expand Down Expand Up @@ -360,21 +370,15 @@ export class AmpAd3PImpl extends AMP.BaseElement {
this.element
).pageViewId64;

// For sticky ad only: must wait for scrolling event before loading the ad
const scrollPromise = this.uiHandler.getScrollPromiseForStickyAd();

this.layoutPromise_ = Promise.all([
getAdCid(this),
consentPromise,
sharedDataPromise,
consentStringPromise,
consentMetadataPromise,
scrollPromise,
pageViewId64Promise,
])
.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
Expand All @@ -390,7 +394,7 @@ export class AmpAd3PImpl extends AMP.BaseElement {
'consentSharedData': consents[2],
'initialConsentValue': consents[3],
'initialConsentMetadata': consents[4],
'pageViewId64': consents[6],
'pageViewId64': consents[5],
};

// In this path, the request and render start events are entangled,
Expand Down
26 changes: 12 additions & 14 deletions extensions/amp-ad/0.1/test/test-amp-ad-3p-impl.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import '../../../amp-sticky-ad/1.0/amp-sticky-ad';
import '../amp-ad';
import * as fakeTimers from '@sinonjs/fake-timers';
import {expect} from 'chai';

import {adConfig} from '#ads/_config';

Expand Down Expand Up @@ -287,20 +288,17 @@ describes.realWin(

describe('during layout', () => {
it('sticky ad: should not layout w/o scroll', () => {
ad3p.uiHandler.stickyAdPosition_ = 'bottom';
expect(ad3p.xOriginIframeHandler_).to.be.null;
const layoutPromise = ad3p.layoutCallback();
return Promise.race([macroTask(), layoutPromise])
.then(() => {
expect(ad3p.xOriginIframeHandler_).to.be.null;
})
.then(() => {
Services.viewportForDoc(env.ampdoc).scrollObservable_.fire();
return layoutPromise;
})
.then(() => {
expect(ad3p.xOriginIframeHandler_).to.not.be.null;
});
ad3p.element.setAttribute('sticky', 'bottom');
ad3p.buildCallback();
const maybeInitStickyAdSpy = env.sandbox.spy(
ad3p.uiHandler,
'maybeInitStickyAd'
);
expect(maybeInitStickyAdSpy).to.not.be.called;
Services.viewportForDoc(env.ampdoc).scrollObservable_.fire();
return Promise.resolve().then(() => {
expect(maybeInitStickyAdSpy).to.be.called;
});
});
});

Expand Down