Skip to content

Commit

Permalink
Bug 1806425 - Make value of source in Feature Callout and page in its…
Browse files Browse the repository at this point in the history
… telementry more consistent r=jprickett,aminomancer

Unify the values of "source" and "page" as used in FeatureCallout.sys.mjs:
- Explicitly pass in a value for "page" when instantiating a Feature Callout and use this for the value of "page" when sending Feature Callout telemetry and as the "source" when making calls to `sendTriggerMessage`. This avoids the risk of including non-about: page URLs or PDF file extensions in our telemetry.
- Set the value of "page" in an HTML data attribute that can be accessed for use in about:welcome telemetry for Spotlight and Feature Callouts.
- Update references to the page value previously used as the page/source for telemetry from `about:firefoxview` Feature Callouts from "firefoxview"  to "about:firefoxview"
- Pass the token "chrome" when creating a callout from the browser chrome and update references to the source in PDF.js messages' targeting
- Update the page value expected in automated tests as needed

Differential Revision: https://phabricator.services.mozilla.com/D165910
  • Loading branch information
lmegviar committed Jan 20, 2023
1 parent 4dfea91 commit a3e99a2
Show file tree
Hide file tree
Showing 10 changed files with 36 additions and 36 deletions.
2 changes: 2 additions & 0 deletions browser/base/content/spotlight.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ function renderMultistage(ready) {

document.body.classList.add("onboardingContainer");
document.body.id = "root";
// This value is reported as the "page" in telemetry
document.body.dataset.page = "spotlight";

// Prevent applying the default modal shadow and margins because the content
// handles styling, including its own modal shadowing.
Expand Down
2 changes: 1 addition & 1 deletion browser/base/content/tabbrowser.js
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@
this._featureCallout = new FeatureCallout({
win: window,
prefName: "browser.pdfjs.feature-tour",
source: location.spec,
page: "chrome",
});
},
_setupInitialBrowserAndTab() {
Expand Down
1 change: 1 addition & 0 deletions browser/components/firefoxview/firefoxview.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const launchFeatureTour = () => {
let callout = new FeatureCallout({
win: window,
prefName: "browser.firefox-view.feature-tour",
page: "about:firefoxview",
});
callout.showFeatureCallout();
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,15 +166,15 @@ add_task(async function feature_callout_closes_on_dismiss() {
event: "CLICK_BUTTON",
event_context: {
source: "dismiss_button",
page: document.location.href,
page: "about:firefoxview",
},
message_id: sinon.match("FEATURE_CALLOUT_2"),
});
spy.assertCalledWith({
event: "DISMISS",
event_context: {
source: "dismiss_button",
page: document.location.href,
page: "about:firefoxview",
},
message_id: sinon.match("FEATURE_CALLOUT_2"),
});
Expand Down Expand Up @@ -418,7 +418,7 @@ add_task(async function feature_callout_dismiss_on_page_click() {
action: "DISMISS",
reason: "CLICK",
source: sinon.match(testClickSelector),
page: document.location.href,
page: "about:firefoxview",
},
message_id: screenId,
});
Expand All @@ -428,7 +428,7 @@ add_task(async function feature_callout_dismiss_on_page_click() {
source: sinon
.match("PAGE_EVENT:")
.and(sinon.match(testClickSelector)),
page: document.location.href,
page: "about:firefoxview",
},
message_id: screenId,
});
Expand Down
2 changes: 1 addition & 1 deletion browser/components/firefoxview/tests/browser/head.js
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ const getCalloutMessageById = id => {
const createSandboxWithCalloutTriggerStub = testMessage => {
const firefoxViewMatch = sinon.match({
id: "featureCalloutCheck",
context: { source: "firefoxview" },
context: { source: "about:firefoxview" },
});
const sandbox = sinon.createSandbox();
const sendTriggerStub = sandbox.stub(ASRouter, "sendTriggerMessage");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ __webpack_require__.r(__webpack_exports__);
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */
// If we're in a subdialog, then this is a spotlight modal.
// Otherwise, this is about:welcome or a Feature Callout
// in another "about" page and we should return the current page.
const page = document.querySelector(":root[dialogroot=true]") ? "spotlight" : document.location.href;
// If the container has a "page" data attribute, then this is
// a Spotlight modal or Feature Callout. Otherwise, this is
// about:welcome and we should return the current page.
const page = document.querySelector("#root.onboardingContainer[data-page]") ? document.querySelector("#root[data-page]").dataset.page : document.location.href;
const AboutWelcomeUtils = {
handleUserAction(action) {
window.AWSendToParent("SPECIAL_ACTION", action);
Expand Down
10 changes: 5 additions & 5 deletions browser/components/newtab/content-src/lib/aboutwelcome-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

// If we're in a subdialog, then this is a spotlight modal.
// Otherwise, this is about:welcome or a Feature Callout
// in another "about" page and we should return the current page.
const page = document.querySelector(":root[dialogroot=true]")
? "spotlight"
// If the container has a "page" data attribute, then this is
// a Spotlight modal or Feature Callout. Otherwise, this is
// about:welcome and we should return the current page.
const page = document.querySelector("#root.onboardingContainer[data-page]")
? document.querySelector("#root[data-page]").dataset.page
: document.location.href;

export const AboutWelcomeUtils = {
Expand Down
17 changes: 8 additions & 9 deletions browser/components/newtab/lib/FeatureCalloutMessages.jsm
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ const FIREFOX_VIEW_PREF = "browser.firefox-view.feature-tour";
const PDFJS_PREF = "browser.pdfjs.feature-tour";
// Empty screens are included as placeholders to ensure step
// indicator shows the correct number of total steps in the tour
const PDF_SOURCE = `(source || "") | regExpMatch('(?<!q\=.+)\.pdf') | length > 0`;
const EMPTY_SCREEN = { content: {} };
const ONE_DAY_IN_MS = 24 * 60 * 60 * 1000;

Expand Down Expand Up @@ -139,7 +138,7 @@ const MESSAGES = () => {
// Add the highest possible cap to ensure impressions are recorded while allowing the Spotlight to sync across windows/tabs with Firefox View open
lifetime: 100,
},
targeting: `!inMr2022Holdback && source == "firefoxview" &&
targeting: `!inMr2022Holdback && source == "about:firefoxview" &&
!'browser.newtabpage.activity-stream.asrouter.providers.cfr'|preferenceIsUserSet &&
'browser.newtabpage.activity-stream.asrouter.userprefs.cfr.features'|preferenceValue &&
${matchCurrentScreenTargeting(
Expand Down Expand Up @@ -212,7 +211,7 @@ const MESSAGES = () => {
],
},
priority: 3,
targeting: `!inMr2022Holdback && source == "firefoxview" && ${matchCurrentScreenTargeting(
targeting: `!inMr2022Holdback && source == "about:firefoxview" && ${matchCurrentScreenTargeting(
FIREFOX_VIEW_PREF,
"FEATURE_CALLOUT_1"
)}`,
Expand Down Expand Up @@ -278,7 +277,7 @@ const MESSAGES = () => {
],
},
priority: 3,
targeting: `!inMr2022Holdback && source == "firefoxview" && ${matchCurrentScreenTargeting(
targeting: `!inMr2022Holdback && source == "about:firefoxview" && ${matchCurrentScreenTargeting(
FIREFOX_VIEW_PREF,
"FEATURE_CALLOUT_2"
)}`,
Expand Down Expand Up @@ -337,7 +336,7 @@ const MESSAGES = () => {
],
},
priority: 2,
targeting: `!inMr2022Holdback && source == "firefoxview" && "browser.firefox-view.view-count" | preferenceValue > 2
targeting: `!inMr2022Holdback && source == "about:firefoxview" && "browser.firefox-view.view-count" | preferenceValue > 2
&& (("identity.fxaccounts.enabled" | preferenceValue == false) || !(("services.sync.engine.tabs" | preferenceValue == true) && ("services.sync.username" | preferenceValue))) && (!messageImpressions.FIREFOX_VIEW_SPOTLIGHT[messageImpressions.FIREFOX_VIEW_SPOTLIGHT | length - 1] || messageImpressions.FIREFOX_VIEW_SPOTLIGHT[messageImpressions.FIREFOX_VIEW_SPOTLIGHT | length - 1] < currentDate|date - ${ONE_DAY_IN_MS})`,
frequency: {
lifetime: 1,
Expand Down Expand Up @@ -407,7 +406,7 @@ const MESSAGES = () => {
],
},
priority: 1,
targeting: `${PDF_SOURCE} && ${matchCurrentScreenTargeting(
targeting: `source == "chrome" && ${matchCurrentScreenTargeting(
PDFJS_PREF,
"FEATURE_CALLOUT_1_A"
)}`,
Expand Down Expand Up @@ -477,7 +476,7 @@ const MESSAGES = () => {
],
},
priority: 1,
targeting: `${PDF_SOURCE} && ${matchCurrentScreenTargeting(
targeting: `source == "chrome" && ${matchCurrentScreenTargeting(
PDFJS_PREF,
"FEATURE_CALLOUT_2_A"
)}`,
Expand Down Expand Up @@ -546,7 +545,7 @@ const MESSAGES = () => {
],
},
priority: 1,
targeting: `${PDF_SOURCE} && ${matchCurrentScreenTargeting(
targeting: `source == "chrome" && ${matchCurrentScreenTargeting(
PDFJS_PREF,
"FEATURE_CALLOUT_1_B"
)}`,
Expand Down Expand Up @@ -616,7 +615,7 @@ const MESSAGES = () => {
],
},
priority: 1,
targeting: `${PDF_SOURCE} && ${matchCurrentScreenTargeting(
targeting: `source == "chrome" && ${matchCurrentScreenTargeting(
PDFJS_PREF,
"FEATURE_CALLOUT_2_B"
)}`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@ async function openURLInNewTab(window, url) {
}

const pdfMatch = sinon.match(val => {
return (
val?.id === "featureCalloutCheck" && val?.context?.source === PDF_TEST_URL
);
return val?.id === "featureCalloutCheck" && val?.context?.source === "chrome";
});

const validateCalloutCustomPosition = (element, positionOverride, doc) => {
Expand Down
18 changes: 9 additions & 9 deletions browser/modules/FeatureCallout.sys.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ const CONTAINER_ID = "root";
* the parent page pointing to the element they describe.
* @param {Window} Window in which messages will be rendered
* @param {String} Name of the pref used to track progress through a given feature tour
* @param {String} Optional string to pass as the source when checking for messages to show,
* defaults to this.doc.location.pathname.toLowerCase().
* @param {String} Optional string to pass as the page when checking for messages to show,
* in the case of the browser chrome the string "chrome" is used.
* @param {Browser} browser
*/
export class FeatureCallout {
constructor({ win, prefName, source, browser }) {
constructor({ win, prefName, page, browser }) {
this.win = win || window;
this.doc = win.document;
this.browser = browser || this.win.docShell.chromeEventHandler;
Expand All @@ -32,7 +32,7 @@ export class FeatureCallout {
this.ready = false;
this.listenersRegistered = false;
this.AWSetup = false;
this.source = source || this.doc.location.pathname.toLowerCase();
this.page = page;
this.focusHandler = this._focusHandler.bind(this);

XPCOMUtils.defineLazyPreferenceGetter(
Expand Down Expand Up @@ -70,12 +70,10 @@ export class FeatureCallout {
return this.win.pageEventManager;
});

const inChrome =
this.win.location.toString() === "chrome://browser/content/browser.xhtml";
// When the window is focused, ensure tour is synced with tours in
// any other instances of the parent page. This does not apply when
// the Callout is shown in the browser chrome.
if (!inChrome) {
if (this.page !== "chrome") {
this.win.addEventListener(
"visibilitychange",
this._handlePrefChange.bind(this)
Expand Down Expand Up @@ -197,6 +195,8 @@ export class FeatureCallout {
"hidden"
);
container.id = CONTAINER_ID;
// This value is reported as the "page" in about:welcome telemetry
container.dataset.page = this.page;
container.setAttribute(
"aria-describedby",
`#${CONTAINER_ID} .welcome-text`
Expand Down Expand Up @@ -687,7 +687,7 @@ export class FeatureCallout {
browser: this.browser,
// triggerId and triggerContext
id: "featureCalloutCheck",
context: { source: this.source },
context: { source: this.page },
});
this.loadingConfig = false;

Expand Down Expand Up @@ -789,7 +789,7 @@ export class FeatureCallout {
* @param {Event} event Triggering event
*/
_handlePageEventAction(action, event) {
const page = this.doc.location.href;
const page = this.page;
const message_id = this.config?.id.toUpperCase();
const source = this._getUniqueElementIdentifier(event.target);
this.win.AWSendEventTelemetry?.({
Expand Down

0 comments on commit a3e99a2

Please sign in to comment.