Skip to content

Commit

Permalink
🐛 amp-analytics failing in some cases when using amp shadow (#39498)
Browse files Browse the repository at this point in the history
* fix: amp-pixel error for shadow dom

* fix args
  • Loading branch information
marcmrf authored Oct 4, 2023
1 parent ecf5bcc commit fe977c0
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 15 deletions.
3 changes: 3 additions & 0 deletions extensions/amp-analytics/0.1/test/test-transport.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ describes.realWin(
sendXhrStub = env.sandbox.stub();
sendBeaconStub = env.sandbox.stub();

env.sandbox.spy(Services, 'urlReplacementsForDoc');

// Needed for PreconnectService.
installDocService(win, true);
});
Expand Down Expand Up @@ -578,6 +580,7 @@ describes.realWin(

function expectImagePixel(url, referrerPolicy, attributionSrc) {
imagePixelVerifier.verifyRequest(url, referrerPolicy, attributionSrc);
expect(Services.urlReplacementsForDoc).to.be.calledWith(ampdoc);
}

function expectNoImagePixel() {
Expand Down
16 changes: 13 additions & 3 deletions extensions/amp-analytics/0.1/transport.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,8 @@ export class Transport {
getRequest(false),
suppressWarnings,
/** @type {string|undefined} */ (this.referrerPolicy_),
/** @type {string|undefined} */ (this.attributionSrc_)
/** @type {string|undefined} */ (this.attributionSrc_),
this.ampdoc_
);
return;
}
Expand Down Expand Up @@ -248,18 +249,27 @@ export class Transport {
* @param {boolean} suppressWarnings
* @param {string|undefined} referrerPolicy
* @param {string|undefined} attributionSrc
* @param {!Element|!./service/ampdoc-impl.AmpDoc} elementOrAmpDoc Whether services are provided by an
* element.
*/
static sendRequestUsingImage(
win,
request,
suppressWarnings,
referrerPolicy,
attributionSrc
attributionSrc,
elementOrAmpDoc
) {
if (!win) {
return;
}
const image = createPixel(win, request.url, referrerPolicy, attributionSrc);
const image = createPixel(
win,
request.url,
referrerPolicy,
attributionSrc,
elementOrAmpDoc
);
loadPromise(image)
.then(() => {
dev().fine(TAG_, 'Sent image request', request.url);
Expand Down
3 changes: 2 additions & 1 deletion src/builtins/amp-pixel/amp-pixel.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,8 @@ export class AmpPixel extends BaseElement {
this.win,
src,
this.referrerPolicy_,
this.element.getAttribute('attributionsrc')
this.element.getAttribute('attributionsrc'),
this.element
);
dev().info(TAG, 'pixel triggered: ', src);
return pixel;
Expand Down
62 changes: 51 additions & 11 deletions src/pixel.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,28 +18,44 @@ const TAG = 'pixel';
* @param {string} src
* @param {?string=} referrerPolicy
* @param {string=} attributionSrc
* @param {(Element|./service/ampdoc-impl.AmpDoc)=} opt_elementOrAmpDoc Whether services are provided by an
* element.
* @return {!Element}
*/
export function createPixel(win, src, referrerPolicy, attributionSrc) {
export function createPixel(
win,
src,
referrerPolicy,
attributionSrc,
opt_elementOrAmpDoc
) {
// Caller need to verify window is not destroyed when creating pixel
if (referrerPolicy && referrerPolicy !== 'no-referrer') {
user().error(TAG, 'Unsupported referrerPolicy: %s', referrerPolicy);
}

return referrerPolicy === 'no-referrer'
? createNoReferrerPixel(win, src, attributionSrc)
: createImagePixel(win, src, false, attributionSrc);
? createNoReferrerPixel(win, src, attributionSrc, opt_elementOrAmpDoc)
: createImagePixel(win, src, false, attributionSrc, opt_elementOrAmpDoc);
}

/**
* @param {!Window} win
* @param {string} src
* @param {string=} attributionSrc
* @param {(Element|./service/ampdoc-impl.AmpDoc)=} opt_elementOrAmpDoc Whether services are provided by an
* element.
* @return {!Element}
*/
function createNoReferrerPixel(win, src, attributionSrc) {
function createNoReferrerPixel(win, src, attributionSrc, opt_elementOrAmpDoc) {
if (isReferrerPolicySupported()) {
return createImagePixel(win, src, true, attributionSrc);
return createImagePixel(
win,
src,
true,
attributionSrc,
opt_elementOrAmpDoc
);
} else {
// if "referrerPolicy" is not supported, use iframe wrapper
// to scrub the referrer.
Expand All @@ -52,7 +68,13 @@ function createNoReferrerPixel(win, src, attributionSrc) {
}
);
iframe.onload = () => {
createImagePixel(iframe.contentWindow, src);
createImagePixel(
iframe.contentWindow,
src,
undefined,
undefined,
opt_elementOrAmpDoc
);
};
win.document.body.appendChild(iframe);
return iframe;
Expand All @@ -64,9 +86,17 @@ function createNoReferrerPixel(win, src, attributionSrc) {
* @param {string} src
* @param {boolean=} noReferrer
* @param {string=} attributionSrc
* @param {(Element|./service/ampdoc-impl.AmpDoc)=} opt_elementOrAmpDoc Whether services are provided by an
* element.
* @return {!Image}
*/
function createImagePixel(win, src, noReferrer = false, attributionSrc) {
function createImagePixel(
win,
src,
noReferrer = false,
attributionSrc,
opt_elementOrAmpDoc
) {
const Image = WindowInterface.getImage(win);
const image = new Image();
if (noReferrer) {
Expand All @@ -82,7 +112,8 @@ function createImagePixel(win, src, noReferrer = false, attributionSrc) {
const substituteVariables =
getAttributionReportingStatusUrlVariableRewriter(
win,
attributionReportingStatus
attributionReportingStatus,
opt_elementOrAmpDoc
);
attributionSrc = substituteVariables(attributionSrc);
image.attributionSrc = attributionSrc;
Expand All @@ -93,7 +124,8 @@ function createImagePixel(win, src, noReferrer = false, attributionSrc) {
}
const substituteVariables = getAttributionReportingStatusUrlVariableRewriter(
win,
attributionReportingStatus
attributionReportingStatus,
opt_elementOrAmpDoc
);
src = substituteVariables(src);
image.src = src;
Expand All @@ -113,13 +145,21 @@ function isReferrerPolicySupported() {
/**
* @param {!Window} win
* @param {string=} status
* @param {(Element|./service/ampdoc-impl.AmpDoc)=} opt_elementOrAmpDoc Whether services are provided by an
* element.
* @return {function(string): string}
*/
function getAttributionReportingStatusUrlVariableRewriter(win, status) {
function getAttributionReportingStatusUrlVariableRewriter(
win,
status,
opt_elementOrAmpDoc
) {
const substitutionFunctions = {
'ATTRIBUTION_REPORTING_STATUS': () => status,
};
const replacements = Services.urlReplacementsForDoc(win.document);
const replacements = Services.urlReplacementsForDoc(
opt_elementOrAmpDoc || win.document
);
const allowlist = {
'ATTRIBUTION_REPORTING_STATUS': true,
};
Expand Down
4 changes: 4 additions & 0 deletions test/unit/builtins/test-amp-pixel.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {Services} from '#service';
import {installUrlReplacementsForEmbed} from '#service/url-replacements-impl';
import {VariableSource} from '#service/variable-source';

Expand All @@ -21,6 +22,7 @@ describes.realWin('amp-pixel', {amp: true}, (env) => {
await createPixel(
'https://pubads.g.doubleclick.net/activity;dc_iu=1/abc;ord=1?'
);
env.sandbox.spy(Services, 'urlReplacementsForDoc');
});

function createPixel(src, referrerPolicy) {
Expand Down Expand Up @@ -170,6 +172,7 @@ describes.realWin('amp-pixel', {amp: true}, (env) => {
'https://pubads.g.doubleclick.net/activity;dc_iu=1/abc;ord=1?ars=6'
);
expect(img.attributionSrc).to.equal('');
expect(Services.urlReplacementsForDoc).to.be.calledWith(pixel);
});
});

Expand All @@ -189,6 +192,7 @@ describes.realWin('amp-pixel', {amp: true}, (env) => {
'https://pubads.g.doubleclick.net/activity;dc_iu=1/abc;ord=1?ars=6'
);
expect(img.attributionSrc).to.equal('https://adtech.example?ars=6');
expect(Services.urlReplacementsForDoc).to.be.calledWith(pixel);
});
});
});
Expand Down

0 comments on commit fe977c0

Please sign in to comment.