Bug 1756162 - Make the visibility of the "Top Pick" checkbox depend entirely on whether the best match feature is enabled. r=preferences-reviewers,Gijs
authorDrew Willcoxon <adw@mozilla.com>
Wed, 23 Feb 2022 19:03:32 +0000
changeset 608550 536fddf7da0517b43fcb92b27293bcf3ba06d889
parent 608549 419e8940bbdfa73408fd02ce1a6f434ce3769df8
child 608551 baca03867e52f1c294e9a91e92ee3ad541d56921
push id158380
push userdwillcoxon@mozilla.com
push dateWed, 23 Feb 2022 19:29:51 +0000
treeherderautoland@536fddf7da05 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspreferences-reviewers, Gijs
bugs1756162
milestone99.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1756162 - Make the visibility of the "Top Pick" checkbox depend entirely on whether the best match feature is enabled. r=preferences-reviewers,Gijs We've made a few changes to this "Top pick" checkbox recently based on shifting Product requirements, and the problem here is that the checkbox used to be inside the Firefox Suggest container, but we recently moved it outside the container. The Firefox Suggest container is properly hidden or shown depending on whether the Suggest feature is enabled, so when the checkbox was inside the container and Suggest was disabled, the checkbox properly got hidden too. Now that the checkbox is outside that container, its visibility needs to entirely depend on whether the best match feature is enabled. So all this revision does is move the checkbox's `hidden` assignment from inside the "is Suggest enabled" if-block to outside the if-block. It also sets `hidden=true` on the checkbox in the markup for good measure. I also improved the test so it checks every combination of enabled statuses between the Suggest and best match features. Depends on D138987 Differential Revision: https://phabricator.services.mozilla.com/D139161
browser/components/preferences/privacy.inc.xhtml
browser/components/preferences/privacy.js
browser/components/preferences/tests/browser_privacy_firefoxSuggest.js
--- a/browser/components/preferences/privacy.inc.xhtml
+++ b/browser/components/preferences/privacy.inc.xhtml
@@ -645,17 +645,17 @@
             preference="browser.urlbar.suggest.bookmark"/>
   <checkbox id="openpageSuggestion" data-l10n-id="addressbar-locbar-openpage-option"
             preference="browser.urlbar.suggest.openpage"/>
   <checkbox id="topSitesSuggestion"
             data-l10n-id="addressbar-locbar-shortcuts-option"
             preference="browser.urlbar.suggest.topsites"/>
   <checkbox id="enginesSuggestion" data-l10n-id="addressbar-locbar-engines-option"
             preference="browser.urlbar.suggest.engines"/>
-  <hbox id="firefoxSuggestBestMatchContainer" align="center">
+  <hbox id="firefoxSuggestBestMatchContainer" align="center" hidden="true">
     <checkbox id="firefoxSuggestBestMatch"
               class="tail-with-learn-more"
               data-l10n-id="addressbar-firefox-suggest-best-match-option"
               preference="browser.urlbar.suggest.bestmatch"/>
     <label id="firefoxSuggestBestMatchLearnMore"
            class="learnMore"
            is="text-link"
            data-l10n-id="addressbar-best-match-learn-more"/>
--- a/browser/components/preferences/privacy.js
+++ b/browser/components/preferences/privacy.js
@@ -1993,16 +1993,21 @@ var gPrivacyPane = {
   /**
    * Updates the Firefox Suggest section (in the address bar section) depending
    * on whether the user is enrolled in a Firefox Suggest rollout.
    *
    * @param {boolean} [onInit]
    *   Pass true when calling this when initializing the pane.
    */
   _updateFirefoxSuggestSection(onInit = false) {
+    // Show the best match checkbox container as appropriate.
+    document.getElementById(
+      "firefoxSuggestBestMatchContainer"
+    ).hidden = !UrlbarPrefs.get("bestMatchEnabled");
+
     let container = document.getElementById("firefoxSuggestContainer");
 
     if (UrlbarPrefs.get("quickSuggestEnabled")) {
       // Update the l10n IDs of text elements.
       let l10nIdByElementId = {
         locationBarGroupHeader: "addressbar-header-firefox-suggest",
         locationBarSuggestionLabel: "addressbar-suggest-firefox-suggest",
       };
@@ -2012,21 +2017,16 @@ var gPrivacyPane = {
         element.dataset.l10nId = l10nId;
       }
 
       // Add the extraMargin class to the engine-prefs link.
       document
         .getElementById("openSearchEnginePreferences")
         .classList.add("extraMargin");
 
-      // Show the best match checkbox container as appropriate.
-      document.getElementById(
-        "firefoxSuggestBestMatchContainer"
-      ).hidden = !UrlbarPrefs.get("bestMatchEnabled");
-
       // Show the container.
       this._updateFirefoxSuggestInfoBox();
       container.removeAttribute("hidden");
     } else if (!onInit) {
       // Firefox Suggest is not enabled. This is the default, so to avoid
       // accidentally messing anything up, only modify the doc if we're being
       // called due to a change in the rollout-enabled status (!onInit).
       container.setAttribute("hidden", "true");
--- a/browser/components/preferences/tests/browser_privacy_firefoxSuggest.js
+++ b/browser/components/preferences/tests/browser_privacy_firefoxSuggest.js
@@ -3,16 +3,17 @@
 
 // This tests the Privacy pane's Firefox Suggest UI.
 
 "use strict";
 
 XPCOMUtils.defineLazyModuleGetters(this, {
   UrlbarProviderQuickSuggest:
     "resource:///modules/UrlbarProviderQuickSuggest.jsm",
+  UrlbarQuickSuggest: "resource:///modules/UrlbarQuickSuggest.jsm",
 });
 
 XPCOMUtils.defineLazyGetter(this, "QuickSuggestTestUtils", () => {
   const { QuickSuggestTestUtils: module } = ChromeUtils.import(
     "resource://testing-common/QuickSuggestTestUtils.jsm"
   );
   module.init(this);
   registerCleanupFunction(() => module.uninit());
@@ -38,20 +39,19 @@ const EXPECTED_L10N_IDS = {
     disabled: "addressbar-header",
   },
   locationBarSuggestionLabel: {
     enabled: "addressbar-suggest-firefox-suggest",
     disabled: "addressbar-suggest",
   },
 };
 
-// Allow more time for Mac machines so they don't time out in verify mode.
-if (AppConstants.platform == "macosx") {
-  requestLongerTimeout(3);
-}
+// This test can take a while due to the many permutations some of these tasks
+// run through, so request a longer timeout.
+requestLongerTimeout(10);
 
 // The following tasks check the visibility of the Firefox Suggest UI based on
 // the value of the feature pref. See doVisibilityTest().
 
 add_task(async function historyToOffline() {
   await doVisibilityTest({
     initialScenario: "history",
     initialExpectedVisibility: false,
@@ -121,22 +121,26 @@ add_task(async function onlineToOffline(
 async function doVisibilityTest({
   initialScenario,
   initialExpectedVisibility,
   newScenario,
   newExpectedVisibility,
 }) {
   info(
     "Running visibility test: " +
-      JSON.stringify({
-        initialScenario,
-        initialExpectedVisibility,
-        newScenario,
-        newExpectedVisibility,
-      })
+      JSON.stringify(
+        {
+          initialScenario,
+          initialExpectedVisibility,
+          newScenario,
+          newExpectedVisibility,
+        },
+        null,
+        2
+      )
   );
 
   // Set the initial scenario.
   await QuickSuggestTestUtils.setScenario(initialScenario);
 
   Assert.equal(
     Services.prefs.getBoolPref("browser.urlbar.quicksuggest.enabled"),
     initialExpectedVisibility,
@@ -495,83 +499,122 @@ add_task(async function clickLearnMore()
     gBrowser.removeCurrentTab();
     gBrowser.selectedTab = prefsTab;
   }
 
   gBrowser.removeCurrentTab();
   await SpecialPowers.popPrefEnv();
 });
 
-// Tests the visibility of the best match checkbox when the best match feature is
-// initially disabled and is then enabled via preferences.
-add_task(async function bestMatchVisibility_falseToTrue() {
-  await doBestMatchVisibilityTest(false, true);
-});
-
-// Tests the visibility of the best match checkbox when the best match feature is
-// initially enabled and is then disabled via preferences.
-add_task(async function bestMatchVisibility_trueToFalse() {
-  await doBestMatchVisibilityTest(true, false);
+// Tests the visibility of the best match checkbox based on the values of
+// `browser.urlbar.quicksuggest.enabled` and `browser.urlbar.bestMatch.enabled`.
+add_task(async function bestMatchVisibility() {
+  for (let initialQuickSuggest of [false, true]) {
+    for (let initialBestMatch of [false, true]) {
+      for (let newQuickSuggest of [false, true]) {
+        for (let newBestMatch of [false, true]) {
+          await doBestMatchVisibilityTest({
+            initialQuickSuggest,
+            initialBestMatch,
+            newQuickSuggest,
+            newBestMatch,
+          });
+        }
+      }
+    }
+  }
 });
 
 /**
  * Runs a test that checks the visibility of the Firefox Suggest best match
- * checkbox. It sets the best match feature pref, opens about:preferences and
- * checks the visibility of the toggle, sets the feature pref again, and checks
- * the visibility of the toggle again.
+ * checkbox. It does the following:
+ *
+ * 1. Sets the quick suggest and best match feature prefs
+ * 2. Opens about:preferences and checks the visibility of the checkbox
+ * 3. Sets the quick suggest and best match feature prefs again
+ * 4. Checks the visibility of the checkbox again
  *
- * @param {boolean} initialEnabled
- *   Whether to enable the best match feature before about:preferences is
- *   opened.
- * @param {boolean} newEnabled
- *   Whether to enable the best match feature after about:preferences is
- *   opened.
+ * @param {boolean} initialQuickSuggest
+ *   The value to set for `browser.urlbar.quicksuggest.enabled` before
+ *   about:preferences is opened.
+ * @param {boolean} initialBestMatch
+ *   The value to set for `browser.urlbar.bestMatch.enabled` before
+ *   about:preferences is opened.
+ * @param {boolean} newQuickSuggest
+ *   The value to set for `browser.urlbar.quicksuggest.enabled` while
+ *   about:preferences is open.
+ * @param {boolean} newBestMatch
+ *   The value to set for `browser.urlbar.bestMatch.enabled` while
+ *   about:preferences is open.
  */
-async function doBestMatchVisibilityTest(initialEnabled, newEnabled) {
+async function doBestMatchVisibilityTest({
+  initialQuickSuggest,
+  initialBestMatch,
+  newQuickSuggest,
+  newBestMatch,
+}) {
   info(
     "Running best match visibility test: " +
-      JSON.stringify({ initialEnabled, newEnabled })
+      JSON.stringify(
+        {
+          initialQuickSuggest,
+          initialBestMatch,
+          newQuickSuggest,
+          newBestMatch,
+        },
+        null,
+        2
+      )
   );
 
-  // Set the initial enabled status.
-  await SpecialPowers.pushPrefEnv({
-    set: [["browser.urlbar.bestMatch.enabled", initialEnabled]],
-  });
+  // Set the initial pref values.
+  Services.prefs.setBoolPref(
+    "browser.urlbar.quicksuggest.enabled",
+    initialQuickSuggest
+  );
+  Services.prefs.setBoolPref(
+    "browser.urlbar.bestMatch.enabled",
+    initialBestMatch
+  );
+  await UrlbarQuickSuggest.readyPromise;
 
   // Open prefs and check the initial visibility.
   await openPreferencesViaOpenPreferencesAPI("privacy", { leaveOpen: true });
-
   let doc = gBrowser.selectedBrowser.contentDocument;
   let container = doc.getElementById(BEST_MATCH_CONTAINER_ID);
   Assert.equal(
     BrowserTestUtils.is_visible(container),
-    initialEnabled,
+    initialBestMatch,
     "The checkbox container has the expected initial visibility"
   );
 
-  // Set the new enabled status.
-  await SpecialPowers.pushPrefEnv({
-    set: [["browser.urlbar.bestMatch.enabled", newEnabled]],
-  });
+  // Set the new pref values.
+  Services.prefs.setBoolPref(
+    "browser.urlbar.quicksuggest.enabled",
+    newQuickSuggest
+  );
+  Services.prefs.setBoolPref("browser.urlbar.bestMatch.enabled", newBestMatch);
+  await UrlbarQuickSuggest.readyPromise;
 
   // Check visibility again.
   Assert.equal(
     BrowserTestUtils.is_visible(container),
-    newEnabled,
-    "The checkbox container has the expected visibility after setting enabled status"
+    newBestMatch,
+    "The checkbox container has the expected visibility after setting prefs"
   );
 
   // Clean up.
   gBrowser.removeCurrentTab();
-  await SpecialPowers.popPrefEnv();
-  await SpecialPowers.popPrefEnv();
+  Services.prefs.clearUserPref("browser.urlbar.quicksuggest.enabled");
+  Services.prefs.clearUserPref("browser.urlbar.bestMatch.enabled");
+  await UrlbarQuickSuggest.readyPromise;
 }
 
-// Tests the visibility of the best match checkbox container when the best match feature is
-// enabled via a Nimbus experiment before about:preferences is opened.
+// Tests the visibility of the best match checkbox when the best match feature
+// is enabled via a Nimbus experiment before about:preferences is opened.
 add_task(async function bestMatchVisibility_experiment_beforeOpen() {
   await QuickSuggestTestUtils.withExperiment({
     valueOverrides: {
       bestMatchEnabled: true,
     },
     callback: async () => {
       await openPreferencesViaOpenPreferencesAPI("privacy", {
         leaveOpen: true,
@@ -582,18 +625,18 @@ add_task(async function bestMatchVisibil
         BrowserTestUtils.is_visible(container),
         "The checkbox container is visible"
       );
       gBrowser.removeCurrentTab();
     },
   });
 });
 
-// Tests the visibility of the best match checkbox container when the best match feature is
-// enabled via a Nimbus experiment after about:preferences is opened.
+// Tests the visibility of the best match checkbox when the best match feature
+// is enabled via a Nimbus experiment after about:preferences is opened.
 add_task(async function bestMatchVisibility_experiment_afterOpen() {
   // Open prefs and check the initial visibility.
   await openPreferencesViaOpenPreferencesAPI("privacy", { leaveOpen: true });
   let doc = gBrowser.selectedBrowser.contentDocument;
   let container = doc.getElementById(BEST_MATCH_CONTAINER_ID);
   Assert.ok(
     BrowserTestUtils.is_hidden(container),
     "The checkbox container is hidden initially"
@@ -657,17 +700,18 @@ add_task(async function bestMatchToggle(
   }
 
   // Clean up.
   Services.prefs.clearUserPref("browser.urlbar.suggest.bestmatch");
   gBrowser.removeCurrentTab();
   await SpecialPowers.popPrefEnv();
 });
 
-// Clicks the learn-more link for best match and checks the help page is opened in a new tab.
+// Clicks the learn-more link for best match and checks the help page is opened
+// in a new tab.
 add_task(async function clickBestMatchLearnMore() {
   await SpecialPowers.pushPrefEnv({
     set: [["browser.urlbar.bestMatch.enabled", true]],
   });
 
   await openPreferencesViaOpenPreferencesAPI("privacy", { leaveOpen: true });
 
   const doc = gBrowser.selectedBrowser.contentDocument;