Bug 1498023 - Search shortcuts should hide all one-off UI when typed r=mak,Mardak
authorDrew Willcoxon <adw@mozilla.com>
Mon, 29 Oct 2018 16:13:49 +0000
changeset 443341 0cd201908c7d8d409478c5b95a66fb3ab42378c8
parent 443340 479efb379f5439b2b0f0e541fa71d9911237a904
child 443342 629bcd4ed798247b6d63f80cf0d1c16d179e2b84
push id34954
push userrgurzau@mozilla.com
push dateMon, 29 Oct 2018 22:00:12 +0000
treeherdermozilla-central@b851d42e2620 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, Mardak
bugs1498023
milestone65.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 1498023 - Search shortcuts should hide all one-off UI when typed r=mak,Mardak * Disable or enable the one-offs per each new search based on whether the first char is "@". The patch does this in `onResultsAdded`, where other per-search initialization happens. * Remove the `disableOneOffButtons` option from the urlbar `search` method. It's not necessary anymore now that one-offs are automatically hidden for the only caller that uses this option (new tab with the "@engine" tiles). * Make the `oneOffSearchesEnabled` getter return the actual status of the one-off UI instead of relying on an `_oneOffSearchesEnabled` property. Differential Revision: https://phabricator.services.mozilla.com/D9883
browser/base/content/test/performance/browser_urlbar_keyed_search.js
browser/base/content/test/performance/browser_urlbar_search.js
browser/base/content/test/urlbar/browser_urlbarOneOffs.js
browser/base/content/test/urlbar/browser_urlbarSearchFunction.js
browser/base/content/urlbarBindings.xml
browser/components/newtab/lib/PlacesFeed.jsm
browser/components/newtab/test/unit/lib/PlacesFeed.test.js
--- a/browser/base/content/test/performance/browser_urlbar_keyed_search.js
+++ b/browser/base/content/test/performance/browser_urlbar_keyed_search.js
@@ -18,17 +18,18 @@ requestLongerTimeout(5);
 const EXPECTED_REFLOWS_FIRST_OPEN = [];
 if (AppConstants.DEBUG ||
     AppConstants.platform == "win" ||
     AppConstants.platform == "macosx") {
   EXPECTED_REFLOWS_FIRST_OPEN.push({
     stack: [
       "_rebuild@chrome://browser/content/search/search-one-offs.js",
       "set popup@chrome://browser/content/search/search-one-offs.js",
-      "set_oneOffSearchesEnabled@chrome://browser/content/urlbarBindings.xml",
+      "_syncOneOffSearchesEnabled@chrome://browser/content/urlbarBindings.xml",
+      "toggleOneOffSearches@chrome://browser/content/urlbarBindings.xml",
       "_enableOrDisableOneOffSearches@chrome://browser/content/urlbarBindings.xml",
       "@chrome://browser/content/urlbarBindings.xml",
       "_openAutocompletePopup@chrome://browser/content/urlbarBindings.xml",
       "openAutocompletePopup@chrome://browser/content/urlbarBindings.xml",
       "openPopup@chrome://global/content/bindings/autocomplete.xml",
       "set_popupOpen@chrome://global/content/bindings/autocomplete.xml",
     ],
   });
--- a/browser/base/content/test/performance/browser_urlbar_search.js
+++ b/browser/base/content/test/performance/browser_urlbar_search.js
@@ -19,17 +19,18 @@ const EXPECTED_REFLOWS_FIRST_OPEN = [];
 if (AppConstants.DEBUG ||
     AppConstants.platform == "linux" ||
     AppConstants.platform == "macosx" ||
     AppConstants.isPlatformAndVersionAtLeast("win", "10")) {
   EXPECTED_REFLOWS_FIRST_OPEN.push({
     stack: [
       "_rebuild@chrome://browser/content/search/search-one-offs.js",
       "set popup@chrome://browser/content/search/search-one-offs.js",
-      "set_oneOffSearchesEnabled@chrome://browser/content/urlbarBindings.xml",
+      "_syncOneOffSearchesEnabled@chrome://browser/content/urlbarBindings.xml",
+      "toggleOneOffSearches@chrome://browser/content/urlbarBindings.xml",
       "_enableOrDisableOneOffSearches@chrome://browser/content/urlbarBindings.xml",
       "@chrome://browser/content/urlbarBindings.xml",
       "_openAutocompletePopup@chrome://browser/content/urlbarBindings.xml",
       "openAutocompletePopup@chrome://browser/content/urlbarBindings.xml",
       "openPopup@chrome://global/content/bindings/autocomplete.xml",
       "set_popupOpen@chrome://global/content/bindings/autocomplete.xml",
     ],
   });
--- a/browser/base/content/test/urlbar/browser_urlbarOneOffs.js
+++ b/browser/base/content/test/urlbar/browser_urlbarOneOffs.js
@@ -29,16 +29,18 @@ add_task(async function init() {
     });
   }
   await PlacesTestUtils.addVisits(visits);
 });
 
 // Keys up and down through the history panel, i.e., the panel that's shown when
 // there's no text in the textbox.
 add_task(async function history() {
+  gURLBar.popup.toggleOneOffSearches(true);
+
   gURLBar.focus();
   EventUtils.synthesizeKey("KEY_ArrowDown");
   await promisePopupShown(gURLBar.popup);
   await waitForAutocompleteResultAt(gMaxResults - 1);
 
   assertState(-1, -1, "");
 
   // Key down through each result.
@@ -244,16 +246,42 @@ add_task(async function collapsedOneOffs
   assertState(0, -1);
   Assert.ok(gURLBar.popup.oneOffSearchButtons.buttons.collapsed,
     "The one-off buttons should be collapsed");
   EventUtils.synthesizeKey("KEY_ArrowUp");
   assertState(1, -1);
   await hidePopup();
 });
 
+
+// The one-offs should be hidden when searching with an "@engine" search engine
+// alias.
+add_task(async function hiddenWhenUsingSearchAlias() {
+  let typedValue = "@example";
+  await promiseAutocompleteResultPopup(typedValue, window, true);
+  await waitForAutocompleteResultAt(0);
+  Assert.equal(gURLBar.popup.oneOffSearchesEnabled, false);
+  Assert.equal(
+    window.getComputedStyle(gURLBar.popup.oneOffSearchButtons).display,
+    "none"
+  );
+  await hidePopup();
+
+  typedValue = "not an engine alias";
+  await promiseAutocompleteResultPopup(typedValue, window, true);
+  await waitForAutocompleteResultAt(0);
+  Assert.equal(gURLBar.popup.oneOffSearchesEnabled, true);
+  Assert.equal(
+    window.getComputedStyle(gURLBar.popup.oneOffSearchButtons).display,
+    "-moz-box"
+  );
+  await hidePopup();
+});
+
+
 function assertState(result, oneOff, textValue = undefined) {
   Assert.equal(gURLBar.popup.selectedIndex, result,
                "Expected result should be selected");
   Assert.equal(gURLBar.popup.oneOffSearchButtons.selectedButtonIndex, oneOff,
                "Expected one-off should be selected");
   if (textValue !== undefined) {
     Assert.equal(gURLBar.textValue, textValue, "Expected textValue");
   }
--- a/browser/base/content/test/urlbar/browser_urlbarSearchFunction.js
+++ b/browser/base/content/test/urlbar/browser_urlbarSearchFunction.js
@@ -23,17 +23,17 @@ add_task(async function init() {
     // Make sure the popup is closed for the next test.
     gURLBar.handleRevert();
     gURLBar.blur();
     Assert.ok(!gURLBar.popup.popupOpen, "popup should be closed");
   });
 });
 
 
-// Calls search() with only a search-string argument.
+// Calls search() with a normal, non-"@engine" search-string argument.
 add_task(async function basic() {
   let resetNotification = enableSearchSuggestionsNotification();
 
   gURLBar.search("basic");
   await promiseSearchComplete();
   await waitForAutocompleteResultAt(0);
   assertUrlbarValue("basic");
 
@@ -42,109 +42,39 @@ add_task(async function basic() {
 
   EventUtils.synthesizeKey("KEY_Escape");
   await promisePopupHidden(gURLBar.popup);
 
   resetNotification();
 });
 
 
-// Calls search() with the search-suggestions notification disabled.
-add_task(async function disableSearchSuggestionsNotification() {
-  let resetNotification = enableSearchSuggestionsNotification();
-
-  gURLBar.search("disableSearchSuggestionsNotification", {
-    disableSearchSuggestionsNotification: true,
-  });
-  await promiseSearchComplete();
-  await waitForAutocompleteResultAt(0);
-  assertUrlbarValue("disableSearchSuggestionsNotification");
-
-  assertSearchSuggestionsNotificationVisible(false);
-  assertOneOffButtonsVisible(true);
-
-  EventUtils.synthesizeKey("KEY_Escape");
-  await promisePopupHidden(gURLBar.popup);
-
-  // Open the popup again (by doing another search) to make sure the
-  // notification is shown -- i.e., that we didn't accidentally break it if
-  // it should continue being shown.
-  gURLBar.search("disableSearchSuggestionsNotification again");
-  await promiseSearchComplete();
-  await waitForAutocompleteResultAt(0);
-  assertUrlbarValue("disableSearchSuggestionsNotification again");
-  assertSearchSuggestionsNotificationVisible(true);
-  assertOneOffButtonsVisible(true);
-
-  EventUtils.synthesizeKey("KEY_Escape");
-  await promisePopupHidden(gURLBar.popup);
-
-  resetNotification();
-});
-
-
-// Calls search() with the one-off search buttons disabled.
-add_task(async function disableOneOffButtons() {
+// Calls search() with an "@engine" search engine alias so that the one-off
+// search buttons and search-suggestions notification are disabled.
+add_task(async function searchEngineAlias() {
   let resetNotification = enableSearchSuggestionsNotification();
 
-  gURLBar.search("disableOneOffButtons", {
-    disableOneOffButtons: true,
-  });
-  await promiseSearchComplete();
-  await waitForAutocompleteResultAt(0);
-  assertUrlbarValue("disableOneOffButtons");
-
-  assertSearchSuggestionsNotificationVisible(true);
-  assertOneOffButtonsVisible(false);
-
-  EventUtils.synthesizeKey("KEY_Escape");
-  await promisePopupHidden(gURLBar.popup);
-
-  // Open the popup again (by doing another search) to make sure the one-off
-  // buttons are shown -- i.e., that we didn't accidentally break them.
-  gURLBar.search("disableOneOffButtons again");
+  gURLBar.search("@example");
   await promiseSearchComplete();
   await waitForAutocompleteResultAt(0);
-  assertUrlbarValue("disableOneOffButtons again");
-  assertSearchSuggestionsNotificationVisible(true);
-  assertOneOffButtonsVisible(true);
-
-  EventUtils.synthesizeKey("KEY_Escape");
-  await promisePopupHidden(gURLBar.popup);
-
-  resetNotification();
-});
-
-
-// Calls search() with both the search-suggestions notification and the one-off
-// search buttons disabled.
-add_task(async function disableSearchSuggestionsNotificationAndOneOffButtons() {
-  let resetNotification = enableSearchSuggestionsNotification();
-
-  gURLBar.search("disableSearchSuggestionsNotificationAndOneOffButtons", {
-    disableSearchSuggestionsNotification: true,
-    disableOneOffButtons: true,
-  });
-  await promiseSearchComplete();
-  await waitForAutocompleteResultAt(0);
-  assertUrlbarValue("disableSearchSuggestionsNotificationAndOneOffButtons");
+  assertUrlbarValue("@example");
 
   assertSearchSuggestionsNotificationVisible(false);
   assertOneOffButtonsVisible(false);
 
   EventUtils.synthesizeKey("KEY_Escape");
   await promisePopupHidden(gURLBar.popup);
 
   // Open the popup again (by doing another search) to make sure the
   // notification and one-off buttons are shown -- i.e., that we didn't
   // accidentally break them.
-  gURLBar.search("disableSearchSuggestionsNotificationAndOneOffButtons again");
+  gURLBar.search("not an engine alias");
   await promiseSearchComplete();
   await waitForAutocompleteResultAt(0);
-  assertUrlbarValue("disableSearchSuggestionsNotificationAndOneOffButtons again");
+  assertUrlbarValue("not an engine alias");
   assertSearchSuggestionsNotificationVisible(true);
   assertOneOffButtonsVisible(true);
 
   EventUtils.synthesizeKey("KEY_Escape");
   await promisePopupHidden(gURLBar.popup);
 
   resetNotification();
 });
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -1237,18 +1237,20 @@ file, You can obtain one at http://mozil
                 break;
             }
           }
         ]]></body>
       </method>
 
       <method name="_enableOrDisableOneOffSearches">
         <body><![CDATA[
-          this.popup.oneOffSearchesEnabled =
-            this._prefs.getBoolPref("oneOffSearches");
+          this.popup.toggleOneOffSearches(
+            this._prefs.getBoolPref("oneOffSearches"),
+            "pref"
+          );
         ]]></body>
       </method>
 
       <method name="handleEvent">
         <parameter name="aEvent"/>
         <body><![CDATA[
           switch (aEvent.type) {
             case "paste":
@@ -1637,42 +1639,23 @@ file, You can obtain one at http://mozil
       </method>
 
       <!--
         Sets the input's value, starts a search, and opens the popup.
 
         @param  value
                 The input's value will be set to this value, and the search will
                 use it as its query.
-        @param  options
-                An optional object with the following optional properties:
-                * disableOneOffButtons: Set to true to hide the one-off search
-                  buttons.
-                * disableSearchSuggestionsNotification: Set to true to hide the
-                  onboarding opt-out search suggestions notification.
       -->
       <method name="search">
         <parameter name="value"/>
-        <parameter name="options"/>
         <body><![CDATA[
-          options = options || {};
-
-          if (options.disableOneOffButtons) {
-            this.popup.addEventListener("popupshowing", () => {
-              if (this.popup.oneOffSearchesEnabled) {
-                this.popup.oneOffSearchesEnabled = false;
-                this.popup.addEventListener("popuphidden", () => {
-                  this.popup.oneOffSearchesEnabled = true;
-                }, {once: true});
-              }
-            }, {once: true});
-          }
-
-          if (options.disableSearchSuggestionsNotification &&
-              this.whichSearchSuggestionsNotification != "none") {
+          // Hide the suggestions notification if the search uses an "@engine"
+          // search engine alias.
+          if (value.trim()[0] == "@") {
             let which = this.whichSearchSuggestionsNotification;
             this._whichSearchSuggestionsNotification = "none";
             this.popup.addEventListener("popuphidden", () => {
               this._whichSearchSuggestionsNotification = which;
             }, {once: true});
           }
 
           // We want the value to be treated as text that the user typed.  It
@@ -1914,18 +1897,16 @@ file, You can obtain one at http://mozil
         250
       </field>
 
       <field name="oneOffSearchButtons" readonly="true">
         document.getAnonymousElementByAttribute(this, "anonid",
                                                 "one-off-search-buttons");
       </field>
 
-      <field name="_oneOffSearchesEnabled">false</field>
-
       <field name="_overrideValue">null</field>
       <property name="overrideValue"
                 onget="return this._overrideValue;"
                 onset="this._overrideValue = val; return val;"/>
 
       <method name="onPopupClick">
         <parameter name="aEvent"/>
         <body><![CDATA[
@@ -1933,37 +1914,60 @@ file, You can obtain one at http://mozil
             // Ignore right-clicks.
             return;
           }
           // Otherwise "call super" -- do what autocomplete-base-popup does.
           this.input.controller.handleEnter(true, aEvent);
         ]]></body>
       </method>
 
-      <property name="oneOffSearchesEnabled">
-        <getter><![CDATA[
-          return this._oneOffSearchesEnabled;
-        ]]></getter>
-        <setter><![CDATA[
-          this._oneOffSearchesEnabled = !!val;
-          if (val) {
+      <field name="_oneOffSearchesEnabledByReason">new Map()</field>
+
+      <method name="toggleOneOffSearches">
+        <parameter name="enable"/>
+        <parameter name="reason"/>
+        <body><![CDATA[
+          this._oneOffSearchesEnabledByReason.set(reason || "runtime", enable);
+          this._syncOneOffSearchesEnabled();
+        ]]></body>
+      </method>
+
+      <method name="_syncOneOffSearchesEnabled">
+        <body><![CDATA[
+          // If the popup hasn't ever been opened yet, then don't actually do
+          // anything.  (The popup will still be hidden in that case.)  The
+          // input adds a popupshowing listener that will call this method back
+          // and lazily initialize the one-off buttons the first time the popup
+          // opens.  There are performance tests that fail if we don't do this.
+          if (this.hidden) {
+            return;
+          }
+
+          let enable = Array.from(this._oneOffSearchesEnabledByReason.values())
+                            .every(v => v);
+          if (enable) {
             this.oneOffSearchButtons.telemetryOrigin = "urlbar";
             this.oneOffSearchButtons.style.display = "-moz-box";
             // Set .textbox first, since the popup setter will cause
             // a _rebuild call that uses it.
             this.oneOffSearchButtons.textbox = this.input;
             this.oneOffSearchButtons.popup = this;
           } else {
             this.oneOffSearchButtons.telemetryOrigin = null;
             this.oneOffSearchButtons.style.display = "none";
             this.oneOffSearchButtons.textbox = null;
             this.oneOffSearchButtons.popup = null;
           }
-          return this._oneOffSearchesEnabled;
-        ]]></setter>
+        ]]></body>
+      </method>
+
+      <property name="oneOffSearchesEnabled" readonly="true">
+        <getter><![CDATA[
+          return this.oneOffSearchButtons.style.display != "none";
+        ]]></getter>
       </property>
 
       <!-- Override this so that navigating between items results in an item
            always being selected. -->
       <method name="getNextIndex">
         <parameter name="reverse"/>
         <parameter name="amount"/>
         <parameter name="index"/>
@@ -2456,16 +2460,20 @@ file, You can obtain one at http://mozil
             // search alias in the input or remove the formatting of the
             // previous alias, as necessary.  We need to check selectHeuristic
             // because the result may have already been added but only now is
             // being selected, and we need to check gotResultForCurrentQuery
             // because the result may be from the previous search and already
             // selected and is now being reused.
             if (selectHeuristic || !this.input.gotResultForCurrentQuery) {
               this.input.formatValue();
+
+              // Also, hide the one-off search buttons if the user is using, or
+              // starting to use, an "@engine" search engine alias.
+              this.toggleOneOffSearches(this.input.value.trim()[0] != "@");
             }
 
             // If this is the first time we get the result from the current
             // search and we are not in the private context, we can speculatively
             // connect to the intended site as a performance optimization.
             if (!this.input.gotResultForCurrentQuery &&
                 this.input.speculativeConnectEnabled &&
                 !this.input.inPrivateContext &&
--- a/browser/components/newtab/lib/PlacesFeed.jsm
+++ b/browser/components/newtab/lib/PlacesFeed.jsm
@@ -275,17 +275,17 @@ class PlacesFeed {
         }));
       }
     } catch (err) {
       Cu.reportError(err);
     }
   }
 
   fillSearchTopSiteTerm({_target, data}) {
-    _target.browser.ownerGlobal.gURLBar.search(`${data.label} `, {disableOneOffButtons: true, disableSearchSuggestionsNotification: true});
+    _target.browser.ownerGlobal.gURLBar.search(`${data.label} `);
   }
 
   onAction(action) {
     switch (action.type) {
       case at.INIT:
         // Briefly avoid loading services for observing for better startup timing
         Services.tm.dispatchToMainThread(() => this.addObservers());
         break;
--- a/browser/components/newtab/test/unit/lib/PlacesFeed.test.js
+++ b/browser/components/newtab/test/unit/lib/PlacesFeed.test.js
@@ -245,17 +245,17 @@ describe("PlacesFeed", () => {
         type: at.FILL_SEARCH_TERM,
         data: {label: "@Foo"},
         _target: {browser: {ownerGlobal: {gURLBar: locationBar}}},
       };
 
       feed.fillSearchTopSiteTerm(action);
 
       assert.calledOnce(locationBar.search);
-      assert.calledWithExactly(locationBar.search, "@Foo ", {disableOneOffButtons: true, disableSearchSuggestionsNotification: true});
+      assert.calledWithExactly(locationBar.search, "@Foo ");
     });
     it("should call saveToPocket on SAVE_TO_POCKET", () => {
       const action = {
         type: at.SAVE_TO_POCKET,
         data: {site: {url: "raspberry.com", title: "raspberry"}},
         _target: {browser: {}},
       };
       sinon.stub(feed, "saveToPocket");