From 34cbd31c4c7ce5383ab728ed3b19b918ffa423e7 Mon Sep 17 00:00:00 2001 From: James Teow Date: Wed, 22 Mar 2023 13:59:06 +0000 Subject: [PATCH] Bug 1815971 - Count number of times persisted search is viewed and reverted due to Popups - r=adw Differential Revision: https://phabricator.services.mozilla.com/D172690 --- browser/base/content/browser.js | 8 +- browser/components/urlbar/UrlbarInput.sys.mjs | 21 +- browser/components/urlbar/docs/telemetry.rst | 23 ++ .../urlbar/tests/browser/browser.ini | 1 + .../browser_UrlbarInput_searchTerms_popup.js | 42 ++ ...owser_UrlbarInput_searchTerms_telemetry.js | 378 ++++++++++++++++++ toolkit/components/telemetry/Scalars.yaml | 38 ++ 7 files changed, 507 insertions(+), 4 deletions(-) create mode 100644 browser/components/urlbar/tests/browser/browser_UrlbarInput_searchTerms_telemetry.js diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js index 6c72f3966fa1..c3ad4f75e751 100644 --- a/browser/base/content/browser.js +++ b/browser/base/content/browser.js @@ -5263,7 +5263,13 @@ var XULBrowserWindow = { // via simulated locationchange events such as switching between tabs, however // if this is a document navigation then PopupNotifications will be updated // via TabsProgressListener.onLocationChange and we do not want it called twice - gURLBar.setURI(aLocationURI, aIsSimulated, isSessionRestore); + gURLBar.setURI( + aLocationURI, + aIsSimulated, + isSessionRestore, + false, + isSameDocument + ); BookmarkingUI.onLocationChange(); // If we've actually changed document, update the toolbar visibility. diff --git a/browser/components/urlbar/UrlbarInput.sys.mjs b/browser/components/urlbar/UrlbarInput.sys.mjs index 635e4a59f1bb..530a81f017ba 100644 --- a/browser/components/urlbar/UrlbarInput.sys.mjs +++ b/browser/components/urlbar/UrlbarInput.sys.mjs @@ -329,12 +329,17 @@ export class UrlbarInput { * @param {boolean} [dontShowSearchTerms] * True if userTypedValue should not be overidden by search terms * and false otherwise. + * @param {boolean} [isSameDocument] + * True if the caller of setURI loaded a new document and false + * otherwise (e.g. the location change was from an anchor scroll + * or a pushState event). */ setURI( uri = null, dueToTabSwitch = false, dueToSessionRestore = false, - dontShowSearchTerms = false + dontShowSearchTerms = false, + isSameDocument = false ) { if (!this.window.gBrowser.userTypedValue) { this.window.gBrowser.selectedBrowser.searchTerms = ""; @@ -363,6 +368,12 @@ export class UrlbarInput { if (this.window.gBrowser.selectedBrowser.searchTerms) { value = this.window.gBrowser.selectedBrowser.searchTerms; valid = !dueToSessionRestore; + if (!isSameDocument) { + Services.telemetry.scalarAdd( + "urlbar.persistedsearchterms.view_count", + 1 + ); + } } else { uri = this.window.gBrowser.selectedBrowser.currentAuthPromptURI || @@ -767,10 +778,14 @@ export class UrlbarInput { maybeHandleRevertFromPopup(anchorElement) { if ( anchorElement?.closest("#urlbar") && - this.window.gBrowser.selectedBrowser.searchTerms + this.window.gBrowser.selectedBrowser.searchTerms && + !this.window.gBrowser.userTypedValue ) { this.handleRevert(true); - // TODO: Bug 1815971, add telemetry. + Services.telemetry.scalarAdd( + "urlbar.persistedsearchterms.revert_by_popup_count", + 1 + ); } } diff --git a/browser/components/urlbar/docs/telemetry.rst b/browser/components/urlbar/docs/telemetry.rst index f5c0122c4ac4..6baaf1fede2b 100644 --- a/browser/components/urlbar/docs/telemetry.rst +++ b/browser/components/urlbar/docs/telemetry.rst @@ -122,6 +122,29 @@ urlbar.impression.* - ``autofill_url`` For url type autofill. +urlbar.persistedsearchterms.revert_by_popup_count +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + + A uint that is incremented when search terms are persisted in the Urlbar and + the Urlbar is reverted to show a full URL due to a PopupNotification. This + can happen when a user is on a SERP and permissions are requested, e.g. + request access to location. If the popup is persistent and the user did not + dismiss it before switching tabs, the popup will reappear when they return to + the tab. Thus, when returning to the tab with the persistent popup, this + value will be incremented because it should have persisted search terms but + instead showed a full URL. + +urlbar.persistedsearchterms.view_count +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + + A uint that is incremented when search terms should be persisted in the + Urlbar. This will trigger when a user loads a SERP from any SAP that results + in the search terms persisting in the Urlbar, as well as switching to a tab + containing a SERP that should be persisting the search terms in the Urlbar, + regardless of whether a PopupNotification is present. Thus, for every + ``revert_by_popup_count``, there should be at least one corresponding + ``view_count``. + urlbar.tips ~~~~~~~~~~~ diff --git a/browser/components/urlbar/tests/browser/browser.ini b/browser/components/urlbar/tests/browser/browser.ini index ad1299767718..61546e7ad83f 100644 --- a/browser/components/urlbar/tests/browser/browser.ini +++ b/browser/components/urlbar/tests/browser/browser.ini @@ -399,6 +399,7 @@ skip-if = [browser_UrlbarInput_searchTerms_searchBar.js] [browser_UrlbarInput_searchTerms_searchMode.js] [browser_UrlbarInput_searchTerms_switch_tab.js] +[browser_UrlbarInput_searchTerms_telemetry.js] [browser_UrlbarInput_setURI.js] https_first_disabled = true skip-if = diff --git a/browser/components/urlbar/tests/browser/browser_UrlbarInput_searchTerms_popup.js b/browser/components/urlbar/tests/browser/browser_UrlbarInput_searchTerms_popup.js index fd987ef69414..2baed08eba3d 100644 --- a/browser/components/urlbar/tests/browser/browser_UrlbarInput_searchTerms_popup.js +++ b/browser/components/urlbar/tests/browser/browser_UrlbarInput_searchTerms_popup.js @@ -100,6 +100,48 @@ add_task(async function generic_popup_when_persist_is_enabled() { await SpecialPowers.popPrefEnv(); }); +// Ensure the urlbar is not being reverted when a user +// has written something. +add_task(async function generic_popup_no_revert_when_persist_is_disabled() { + let { tab } = await searchWithTab( + SEARCH_TERM, + null, + defaultTestEngine, + false + ); + + let userTypedValue = "chocolate cake recipe"; + // Have a user typed value in the urlbar to make + // pageproxystate invalid. + await UrlbarTestUtils.promiseAutocompleteResultPopup({ + window, + value: userTypedValue, + waitForFocus, + fireInputEvent: true, + }); + gURLBar.blur(); + + await waitForPopupNotification(); + + // Wait a brief amount of time between when the popup is shown + // and when the event handler should fire if it's enabled. + await TestUtils.waitForTick(); + + Assert.equal( + gURLBar.getAttribute("pageproxystate"), + "invalid", + "Urlbar should not be reverted." + ); + + Assert.equal( + gURLBar.value, + userTypedValue, + "User typed value should remain in urlbar." + ); + + BrowserTestUtils.removeTab(tab); +}); + // Ensure the urlbar is not being reverted when a prompt is shown // and the persist feature is disabled. add_task(async function generic_popup_no_revert_when_persist_is_disabled() { diff --git a/browser/components/urlbar/tests/browser/browser_UrlbarInput_searchTerms_telemetry.js b/browser/components/urlbar/tests/browser/browser_UrlbarInput_searchTerms_telemetry.js new file mode 100644 index 000000000000..0c48b906c0c0 --- /dev/null +++ b/browser/components/urlbar/tests/browser/browser_UrlbarInput_searchTerms_telemetry.js @@ -0,0 +1,378 @@ +/* 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/. */ + +/** + * These tests check that we record the number of times search terms + * persist in the Urlbar, and when search terms are cleared due to a + * PopupNotification. + * + * This is different from existing telemetry that tracks whether users + * interacted with the Urlbar or made another search while the search + * terms were peristed. + */ + +let defaultTestEngine; + +// The main search string used in tests +const SEARCH_STRING = "chocolate cake"; + +// Telemetry keys. +const PERSISTED_VIEWED = "urlbar.persistedsearchterms.view_count"; +const PERSISTED_REVERTED = "urlbar.persistedsearchterms.revert_by_popup_count"; + +add_setup(async function() { + await SpecialPowers.pushPrefEnv({ + set: [["browser.urlbar.showSearchTerms.featureGate", true]], + }); + await SearchTestUtils.installSearchExtension( + { + name: "MozSearch", + search_url: "https://www.example.com/", + search_url_get_params: "q={searchTerms}&pc=fake_code", + }, + { setAsDefault: true } + ); + defaultTestEngine = Services.search.getEngineByName("MozSearch"); + Services.telemetry.clearScalars(); + + registerCleanupFunction(async function() { + await PlacesUtils.history.clear(); + Services.telemetry.clearScalars(); + }); +}); + +// Starts a search with a tab and asserts that +// the state of the Urlbar contains the search term. +async function searchWithTab( + searchString, + tab = null, + engine = defaultTestEngine, + assertSearchString = true +) { + if (!tab) { + tab = await BrowserTestUtils.openNewForegroundTab(gBrowser); + } + + let [expectedSearchUrl] = UrlbarUtils.getSearchQueryUrl(engine, searchString); + let browserLoadedPromise = BrowserTestUtils.browserLoaded( + tab.linkedBrowser, + false, + expectedSearchUrl + ); + + gURLBar.focus(); + await UrlbarTestUtils.promiseAutocompleteResultPopup({ + window, + waitForFocus, + value: searchString, + fireInputEvent: true, + }); + EventUtils.synthesizeKey("KEY_Enter"); + await browserLoadedPromise; + + if (assertSearchString) { + assertSearchStringIsInUrlbar(searchString); + } + + return { tab, expectedSearchUrl }; +} + +add_task(async function load_page_with_persisted_search() { + let { tab } = await searchWithTab(SEARCH_STRING); + const scalars = TelemetryTestUtils.getProcessScalars("parent", false, true); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 1); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, undefined); + + // Clean up. + BrowserTestUtils.removeTab(tab); +}); + +add_task(async function load_page_without_persisted_search() { + await SpecialPowers.pushPrefEnv({ + set: [["browser.urlbar.showSearchTerms.featureGate", false]], + }); + + let { tab } = await searchWithTab( + SEARCH_STRING, + null, + defaultTestEngine, + false + ); + + let scalars = TelemetryTestUtils.getProcessScalars("parent", false, true); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, undefined); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, undefined); + + // Clean up. + BrowserTestUtils.removeTab(tab); + await SpecialPowers.popPrefEnv(); +}); + +// Multiple searches should result in the correct number of recorded views. +add_task(async function load_page_n_times() { + let N = 5; + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser); + for (let index = 0; index < N; ++index) { + await searchWithTab(SEARCH_STRING, tab); + } + + const scalars = TelemetryTestUtils.getProcessScalars("parent", false, true); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, N); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, undefined); + + // Clean up. + BrowserTestUtils.removeTab(tab); +}); + +// A persisted search view event should not be recorded when unfocusing the urlbar. +add_task(async function focus_and_unfocus() { + let { tab } = await searchWithTab(SEARCH_STRING); + + let scalars = TelemetryTestUtils.getProcessScalars("parent", false, false); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 1); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, undefined); + + gURLBar.focus(); + gURLBar.select(); + gURLBar.blur(); + + // Focusing and unfocusing the urlbar shouldn't change the persisted view count. + scalars = TelemetryTestUtils.getProcessScalars("parent", false, true); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 1); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, undefined); + + // Clean up. + BrowserTestUtils.removeTab(tab); +}); + +// A persisted search view event should not be recorded by a +// pushState event after a page has been loaded. +add_task(async function history_api() { + let { tab } = await searchWithTab(SEARCH_STRING); + + let scalars = TelemetryTestUtils.getProcessScalars("parent", false, false); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 1); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, undefined); + + await SpecialPowers.spawn(tab.linkedBrowser, [], function() { + let url = new URL(content.window.location); + let someState = { value: true }; + url.searchParams.set("pc", "fake_code_2"); + content.history.pushState(someState, "", url); + someState.value = false; + content.history.replaceState(someState, "", url); + }); + + scalars = TelemetryTestUtils.getProcessScalars("parent", false, true); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 1); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, undefined); + + // Clean up. + BrowserTestUtils.removeTab(tab); +}); + +// A persisted search view event should be recorded when switching back to a tab +// that contains search terms. +add_task(async function switch_tabs() { + let { tab } = await searchWithTab(SEARCH_STRING); + + let scalars = TelemetryTestUtils.getProcessScalars("parent", false, false); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 1); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, undefined); + + let tab2 = await BrowserTestUtils.openNewForegroundTab(gBrowser); + await BrowserTestUtils.switchTab(gBrowser, tab); + + scalars = TelemetryTestUtils.getProcessScalars("parent", false, true); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 2); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, undefined); + + // Clean up. + BrowserTestUtils.removeTab(tab); + BrowserTestUtils.removeTab(tab2); +}); + +// A telemetry event should be recorded when returning to a persisted SERP via tabhistory. +add_task(async function tabhistory() { + let { tab } = await searchWithTab(SEARCH_STRING); + + let scalars = TelemetryTestUtils.getProcessScalars("parent", false, false); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 1); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, undefined); + + let browserLoadedPromise = BrowserTestUtils.browserLoaded(tab.linkedBrowser); + BrowserTestUtils.loadURIString( + tab.linkedBrowser, + "https://www.example.com/some_url" + ); + await browserLoadedPromise; + + let pageShowPromise = BrowserTestUtils.waitForContentEvent( + tab.linkedBrowser, + "pageshow" + ); + tab.linkedBrowser.goBack(); + await pageShowPromise; + + scalars = TelemetryTestUtils.getProcessScalars("parent", false, true); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 2); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, undefined); + + // Clean up. + BrowserTestUtils.removeTab(tab); +}); + +// PopupNotification's that rely on an anchor element in the urlbar should trigger +// an increment of the revert counter. +// This assumes the anchor element is present in the Urlbar during a valid pageproxystate. +add_task(async function popup_in_urlbar() { + let { tab } = await searchWithTab(SEARCH_STRING); + let promisePopupShown = BrowserTestUtils.waitForEvent( + PopupNotifications.panel, + "popupshown" + ); + PopupNotifications.show( + gBrowser.selectedBrowser, + "test-notification", + "This is a sample popup." + ); + await promisePopupShown; + + let scalars = TelemetryTestUtils.getProcessScalars("parent", false, true); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 1); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, 1); + + // Clean up. + BrowserTestUtils.removeTab(tab); +}); + +// Non-persistent PopupNotifications won't re-appear if a user switches +// tabs and returns to the tab that had the Popup. +add_task(async function non_persistent_popup_in_urlbar_switch_tab() { + let { tab } = await searchWithTab(SEARCH_STRING); + let promisePopupShown = BrowserTestUtils.waitForEvent( + PopupNotifications.panel, + "popupshown" + ); + PopupNotifications.show( + gBrowser.selectedBrowser, + "test-notification", + "This is a sample popup.", + "geo-notification-icon" + ); + await promisePopupShown; + + let scalars = TelemetryTestUtils.getProcessScalars("parent", false); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 1); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, 1); + + let tab2 = await BrowserTestUtils.openNewForegroundTab(gBrowser); + await BrowserTestUtils.switchTab(gBrowser, tab); + + scalars = TelemetryTestUtils.getProcessScalars("parent", false, true); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 2); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, 1); + + // Clean up. + BrowserTestUtils.removeTab(tab); + BrowserTestUtils.removeTab(tab2); +}); + +// Persistent PopupNotifications will constantly appear to users +// even if they switch to another tab and switch back. +add_task(async function persistent_popup_in_urlbar_switch_tab() { + let { tab } = await searchWithTab(SEARCH_STRING); + let promisePopupShown = BrowserTestUtils.waitForEvent( + PopupNotifications.panel, + "popupshown" + ); + PopupNotifications.show( + gBrowser.selectedBrowser, + "test-notification", + "This is a sample popup.", + "geo-notification-icon", + null, + null, + { persistent: true } + ); + await promisePopupShown; + + let scalars = TelemetryTestUtils.getProcessScalars("parent", false); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 1); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, 1); + + let tab2 = await BrowserTestUtils.openNewForegroundTab(gBrowser); + promisePopupShown = BrowserTestUtils.waitForEvent( + PopupNotifications.panel, + "popupshown" + ); + await BrowserTestUtils.switchTab(gBrowser, tab); + await promisePopupShown; + + scalars = TelemetryTestUtils.getProcessScalars("parent", false, true); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 2); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, 2); + + // Clean up. + BrowserTestUtils.removeTab(tab); + BrowserTestUtils.removeTab(tab2); +}); + +// If the persist feature is not enabled, a telemetry event should not be recorded +// if a PopupNotification uses an anchor in the Urlbar. +add_task(async function popup_in_urlbar_without_feature() { + await SpecialPowers.pushPrefEnv({ + set: [["browser.urlbar.showSearchTerms.featureGate", false]], + }); + + let { tab } = await searchWithTab( + SEARCH_STRING, + null, + defaultTestEngine, + false + ); + let promisePopupShown = BrowserTestUtils.waitForEvent( + PopupNotifications.panel, + "popupshown" + ); + PopupNotifications.show( + gBrowser.selectedBrowser, + "test-notification", + "This is a sample popup." + ); + await promisePopupShown; + + let scalars = TelemetryTestUtils.getProcessScalars("parent", false, true); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, undefined); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, undefined); + + // Clean up. + BrowserTestUtils.removeTab(tab); + await SpecialPowers.popPrefEnv(); +}); + +// If the anchor element for the PopupNotification is not located in the Urlbar, +// a telemetry event should not be recorded. +add_task(async function popup_not_in_urlbar() { + let { tab } = await searchWithTab(SEARCH_STRING); + + let promisePopupShown = BrowserTestUtils.waitForEvent( + PopupNotifications.panel, + "popupshown" + ); + PopupNotifications.show( + gBrowser.selectedBrowser, + "test-notification", + "This is a sample popup that uses the unified extensions button.", + gUnifiedExtensions.getPopupAnchorID(gBrowser.selectedBrowser, window) + ); + await promisePopupShown; + + let scalars = TelemetryTestUtils.getProcessScalars("parent", false, true); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_VIEWED, 1); + TelemetryTestUtils.assertScalar(scalars, PERSISTED_REVERTED, undefined); + + // Clean up. + BrowserTestUtils.removeTab(tab); +}); diff --git a/toolkit/components/telemetry/Scalars.yaml b/toolkit/components/telemetry/Scalars.yaml index d060596466f2..12aecc151232 100644 --- a/toolkit/components/telemetry/Scalars.yaml +++ b/toolkit/components/telemetry/Scalars.yaml @@ -7593,6 +7593,44 @@ urlbar.picked: record_in_processes: - main +urlbar.persistedsearchterms: + revert_by_popup_count: + bug_numbers: + - 1815971 + description: + The count of the number of times search terms were removed from the + urlbar due to a shown PopupNotification. This event can happen when + a user loads a SERP and a PopupNotification is shown, as well as when + a user switches away from a tab on a SERP showing a PopupNotification + and switches back to it. + expires: never + kind: uint + notification_emails: + - fx-search-telemetry@mozilla.com + release_channel_collection: opt-out + products: + - 'firefox' + record_in_processes: + - main + view_count: + bug_numbers: + - 1815971 + description: + The count of the number of times search terms persisted in the Urlbar. + This gets recorded after a user loads a SERP that persists search terms, + or switches back to an existing tab that should be showing the persisted + search terms in the Urlbar, regardless of whether PopupNotification + cleared the search terms from the Urlbar. + expires: never + kind: uint + notification_emails: + - fx-search-telemetry@mozilla.com + release_channel_collection: opt-out + products: + - 'firefox' + record_in_processes: + - main + urlbar.searchmode: bookmarkmenu: bug_numbers: