Bug 1575038 - Quantumbar: Record engagement event telemetry when openViewOnFocus is triggered without mouse clicks. r=dao

We need to start engagement event recording when the view opens due to `openViewOnFocus`. We already do for mouse clicks since we call `engagementEvent.start` from `_on_mousedown`. But we don't for the Ctrl/Command-L key shortcut. The shortcut command calls `openLocation` in browser.js, which calls `gURLBar.startQuery` but not `engagementEvent.start`.

Every time we call `engagementEvent.start`, we do it before calling `input.startQuery`. The one exception is in `input._on_drop` because there we just handle the dropped value directly instead of starting a new query with it.

The inverse is also mostly true, i.e., every time we call `input.startQuery`, we also call `engagementEvent.start`. The three exceptions are: in UITour (where it looks like we should be calling `urlbar.search` instead), in `UrlbarInput` after picking a keyword offer result, and in `openLocation` in browser.js (mentioned above). So really the only valid place is after picking a keyword entry.

So, it makes sense to move `engagementEvent.start()` into `input.startQuery` so that callers don't have to call it. I added an `event` param to `startQuery`, since `engagementEvent.start` needs one. I considered removing that need. It's possible, but then we would need a way to avoid calling `engagementEvent.start` in the keyword offer case, so `startQuery` would need something like a `suppressEngagementEvent` param. `event` basically functions as that, so I left it.

Another thing to point out about this patch is that I chose to record a "typed" value when the pageproxystate is invalid and the view opens due to `openViewOnFocus`. The view does not show the user's top sites in that case, so "topsites" seems wrong.

Differential Revision: https://phabricator.services.mozilla.com/D42749

--HG--
extra : moz-landing-system : lando
This commit is contained in:
Drew Willcoxon 2019-08-22 14:50:42 +00:00
Родитель 5c3cba344d
Коммит 292a6bb2ce
6 изменённых файлов: 229 добавлений и 35 удалений

Просмотреть файл

@ -81,7 +81,7 @@
<command id="cmd_gestureRotateLeft" oncommand="gGestureSupport.rotate(event.sourceEvent)"/>
<command id="cmd_gestureRotateRight" oncommand="gGestureSupport.rotate(event.sourceEvent)"/>
<command id="cmd_gestureRotateEnd" oncommand="gGestureSupport.rotateEnd()"/>
<command id="Browser:OpenLocation" oncommand="openLocation();"/>
<command id="Browser:OpenLocation" oncommand="openLocation(event);"/>
<command id="Browser:RestoreLastSession" oncommand="SessionStore.restoreLastSession();" disabled="true"/>
<command id="Browser:NewUserContextTab" oncommand="openNewUserContextTab(event.sourceEvent);"/>
<command id="Browser:OpenAboutContainers" oncommand="openPreferences('paneContainers');"/>

Просмотреть файл

@ -2864,11 +2864,11 @@ function focusAndSelectUrlBar() {
gURLBar.select();
}
function openLocation() {
function openLocation(event) {
if (window.location.href == AppConstants.BROWSER_CHROME_URL) {
focusAndSelectUrlBar();
if (gURLBar.openViewOnFocus && !gURLBar.view.isOpen) {
gURLBar.startQuery();
gURLBar.startQuery({ event });
}
return;
}

Просмотреть файл

@ -349,8 +349,10 @@ class UrlbarController {
}
if (executeAction) {
this.userSelectionBehavior = "arrow";
this.engagementEvent.start(event);
this.input.startQuery({ searchString: this.input.value });
this.input.startQuery({
searchString: this.input.value,
event,
});
}
}
event.preventDefault();
@ -608,16 +610,19 @@ class TelemetryEvent {
}
/**
* Start measuring the elapsed time from an input event.
* Start measuring the elapsed time from a user-generated event.
* After this has been invoked, any subsequent calls to start() are ignored,
* until either record() or discard() are invoked. Thus, it is safe to keep
* invoking this on every input event.
* @param {event} event A DOM input event.
* invoking this on every input event as the user is typing, for example.
* @param {event} event A DOM event.
* @param {string} [searchString] Pass a search string related to the event if
* you have one. The event by itself sometimes isn't enough to
* determine the telemetry details we should record.
* @note This should never throw, or it may break the urlbar.
*/
start(event) {
// Start is invoked at any input, but we only count the first one.
// Once an engagement or abandoment happens, we clear the _startEventInfo.
start(event, searchString = null) {
// start is invoked on a user-generated event, but we only count the first
// one. Once an engagement or abandoment happens, we clear _startEventInfo.
if (!this._category || this._startEventInfo) {
return;
}
@ -625,23 +630,35 @@ class TelemetryEvent {
Cu.reportError("Must always provide an event");
return;
}
if (!["input", "drop", "mousedown", "keydown"].includes(event.type)) {
const validEvents = ["command", "drop", "input", "keydown", "mousedown"];
if (!validEvents.includes(event.type)) {
Cu.reportError("Can't start recording from event type: " + event.type);
return;
}
// "typed" is used when the user types something, while "pasted" and
// "dropped" are used when the text is inserted at once, by a paste or drop
// operation. "topsites" is a bit special, it is used when the user opens
// the empty search dropdown, that is supposed to show top sites. That
// happens by clicking on the urlbar dropmarker, or pressing DOWN with an
// empty input field. Even if the user later types something, we still
// report "topsites", with a positive numChars.
// Possible interaction types:
//
// typed:
// The user typed something and the view opened. We also use this when
// the user has opened the view without typing anything (by clicking the
// dropdown arrow, for example) after having left the pageproxystate
// invalid. In both cases, the view reflects what the user typed.
// pasted:
// The user pasted text.
// dropped:
// The user dropped text.
// topsites:
// The user opened the view with an empty search string (for example,
// after deleting all text, or by clicking the dropdown arrow when the
// pageproxystate is valid). The view shows the user's top sites.
let interactionType = "topsites";
if (event.type == "input") {
interactionType = UrlbarUtils.isPasteEvent(event) ? "pasted" : "typed";
} else if (event.type == "drop") {
interactionType = "dropped";
} else if (searchString) {
interactionType = "typed";
}
this._startEventInfo = {
@ -744,9 +761,9 @@ class TelemetryEvent {
}
/**
* Resets the currently tracked input event, that was registered via start(),
* so it won't be recorded.
* If there's no tracked input event, this is a no-op.
* Resets the currently tracked user-generated event that was registered via
* start(), so it won't be recorded. If there's no tracked event, this is a
* no-op.
*/
discard() {
this._startEventInfo = null;

Просмотреть файл

@ -703,12 +703,27 @@ class UrlbarInput {
* to false so that state is maintained during a single interaction. The
* intended use for this parameter is that it should be set to false when
* this method is called due to input events.
* @param {event} [options.event]
* The user-generated event that triggered the query, if any. If given, we
* will record engagement event telemetry for the query.
*/
startQuery({
allowAutofill = true,
searchString = null,
resetSearchState = true,
event = null,
} = {}) {
if (!searchString) {
searchString =
this.getAttribute("pageproxystate") == "valid" ? "" : this.value;
} else if (!this.value.startsWith(searchString)) {
throw new Error("The current value doesn't start with the search string");
}
if (event) {
this.controller.engagementEvent.start(event, searchString);
}
if (this._suppressStartQuery) {
return;
}
@ -717,13 +732,6 @@ class UrlbarInput {
this._resetSearchState();
}
if (!searchString) {
searchString =
this.getAttribute("pageproxystate") == "valid" ? "" : this.value;
} else if (!this.value.startsWith(searchString)) {
throw new Error("The current value doesn't start with the search string");
}
this._lastSearchString = searchString;
this._textValueOnLastSearch = this.value;
@ -1585,9 +1593,9 @@ class UrlbarInput {
this.editor.selectAll();
event.preventDefault();
} else if (this.openViewOnFocusForCurrentTab && !this.view.isOpen) {
this.controller.engagementEvent.start(event);
this.startQuery({
allowAutofill: false,
event,
});
}
return;
@ -1598,9 +1606,9 @@ class UrlbarInput {
this.view.close();
} else {
this.focus();
this.controller.engagementEvent.start(event);
this.startQuery({
allowAutofill: false,
event,
});
this._maybeSelectAll();
}
@ -1652,8 +1660,6 @@ class UrlbarInput {
return;
}
this.controller.engagementEvent.start(event);
// Autofill only when text is inserted (i.e., event.data is not empty) and
// it's not due to pasting.
let allowAutofill =
@ -1665,6 +1671,7 @@ class UrlbarInput {
searchString: value,
allowAutofill,
resetSearchState: false,
event,
});
}

Просмотреть файл

@ -246,7 +246,7 @@ var UrlbarTestUtils = {
},
/**
* Waits for the popup to be hidden.
* Waits for the popup to be shown.
* @param {object} win The window containing the urlbar
* @param {function} openFn Function to be used to open the popup.
* @returns {Promise} resolved once the popup is closed

Просмотреть файл

@ -414,6 +414,127 @@ const tests = [
};
},
async function() {
info(
"With pageproxystate=valid, open the panel with openViewOnFocus, select with DOWN, Enter."
);
gURLBar.value = "";
let promise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
Services.prefs.setBoolPref("browser.urlbar.openViewOnFocus", true);
await UrlbarTestUtils.promisePopupOpen(window, () => {
window.document.getElementById("Browser:OpenLocation").doCommand();
});
Services.prefs.clearUserPref("browser.urlbar.openViewOnFocus");
await UrlbarTestUtils.promiseSearchComplete(window);
while (gURLBar.untrimmedValue != "http://mochi.test:8888/") {
EventUtils.synthesizeKey("KEY_ArrowDown");
}
EventUtils.synthesizeKey("VK_RETURN");
await promise;
return {
category: "urlbar",
method: "engagement",
object: "enter",
value: "topsites",
extra: {
elapsed: val => parseInt(val) > 0,
numChars: "0",
selType: "history",
selIndex: val => parseInt(val) >= 0,
},
};
},
async function() {
info(
"With pageproxystate=valid, open the panel with openViewOnFocus, click on entry."
);
gURLBar.value = "";
let promise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
Services.prefs.setBoolPref("browser.urlbar.openViewOnFocus", true);
await UrlbarTestUtils.promisePopupOpen(window, () => {
window.document.getElementById("Browser:OpenLocation").doCommand();
});
Services.prefs.clearUserPref("browser.urlbar.openViewOnFocus");
while (gURLBar.untrimmedValue != "http://mochi.test:8888/") {
EventUtils.synthesizeKey("KEY_ArrowDown");
}
let element = UrlbarTestUtils.getSelectedElement(window);
EventUtils.synthesizeMouseAtCenter(element, {});
await promise;
return {
category: "urlbar",
method: "engagement",
object: "click",
value: "topsites",
extra: {
elapsed: val => parseInt(val) > 0,
numChars: "0",
selType: "history",
selIndex: "0",
},
};
},
async function() {
info(
"With pageproxystate=invalid, open the panel with openViewOnFocus, Enter."
);
gURLBar.value = "mochi.test";
gURLBar.setAttribute("pageproxystate", "invalid");
let promise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
Services.prefs.setBoolPref("browser.urlbar.openViewOnFocus", true);
await UrlbarTestUtils.promisePopupOpen(window, () => {
window.document.getElementById("Browser:OpenLocation").doCommand();
});
Services.prefs.clearUserPref("browser.urlbar.openViewOnFocus");
await UrlbarTestUtils.promiseSearchComplete(window);
EventUtils.synthesizeKey("VK_RETURN");
await promise;
return {
category: "urlbar",
method: "engagement",
object: "enter",
value: "typed",
extra: {
elapsed: val => parseInt(val) > 0,
numChars: "10",
selType: "autofill",
selIndex: "0",
},
};
},
async function() {
info(
"With pageproxystate=invalid, open the panel with openViewOnFocus, click on entry."
);
gURLBar.value = "mochi.test";
gURLBar.setAttribute("pageproxystate", "invalid");
let promise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
Services.prefs.setBoolPref("browser.urlbar.openViewOnFocus", true);
await UrlbarTestUtils.promisePopupOpen(window, () => {
window.document.getElementById("Browser:OpenLocation").doCommand();
});
Services.prefs.clearUserPref("browser.urlbar.openViewOnFocus");
await UrlbarTestUtils.promiseSearchComplete(window);
let element = UrlbarTestUtils.getSelectedElement(window);
EventUtils.synthesizeMouseAtCenter(element, {});
await promise;
return {
category: "urlbar",
method: "engagement",
object: "click",
value: "typed",
extra: {
elapsed: val => parseInt(val) > 0,
numChars: "10",
selType: "autofill",
selIndex: "0",
},
};
},
/*
* Abandonment tests.
*/
@ -480,6 +601,53 @@ const tests = [
};
},
async function() {
info(
"With pageproxystate=valid, open the panel with openViewOnFocus, don't type, blur it."
);
gURLBar.value = "";
Services.prefs.setBoolPref("browser.urlbar.openViewOnFocus", true);
await UrlbarTestUtils.promisePopupOpen(window, () => {
window.document.getElementById("Browser:OpenLocation").doCommand();
});
Services.prefs.clearUserPref("browser.urlbar.openViewOnFocus");
gURLBar.blur();
return {
category: "urlbar",
method: "abandonment",
object: "blur",
value: "topsites",
extra: {
elapsed: val => parseInt(val) > 0,
numChars: "0",
},
};
},
async function() {
info(
"With pageproxystate=invalid, open the panel with openViewOnFocus, don't type, blur it."
);
gURLBar.value = "mochi.test";
gURLBar.setAttribute("pageproxystate", "invalid");
Services.prefs.setBoolPref("browser.urlbar.openViewOnFocus", true);
await UrlbarTestUtils.promisePopupOpen(window, () => {
window.document.getElementById("Browser:OpenLocation").doCommand();
});
Services.prefs.clearUserPref("browser.urlbar.openViewOnFocus");
gURLBar.blur();
return {
category: "urlbar",
method: "abandonment",
object: "blur",
value: "typed",
extra: {
elapsed: val => parseInt(val) > 0,
numChars: "10",
},
};
},
/*
* No event tests.
*/
@ -570,7 +738,9 @@ add_task(async function test() {
// This is not necessary after each loop, because assertEvents does it.
Services.telemetry.clearEvents();
for (let testFn of tests) {
for (let i = 0; i < tests.length; i++) {
info(`Running test at index ${i}`);
let testFn = tests[i];
let expectedEvents = [await testFn()].filter(e => !!e);
// Always blur to ensure it's not accounted as an additional abandonment.
gURLBar.blur();