Bug 1498023 - Search shortcuts should hide all one-off UI when typed. r=mak,Mardak, a=RyanVM
authorDrew Willcoxon <adw@mozilla.com>
Mon, 29 Oct 2018 16:13:49 +0000
changeset 500943 3931ebbd558836838926001aef24f4c4d1120653
parent 500942 43d2c9744a7d94ef0609d6f1e099ccb19c538297
child 500944 bc7548ec328bb2ece1a2a0d519fd466cf2353ad1
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, Mardak, RyanVM
bugs1498023
milestone64.0
Bug 1498023 - Search shortcuts should hide all one-off UI when typed. r=mak,Mardak, a=RyanVM * 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.xml",
       "set_popup@chrome://browser/content/search/search.xml",
-      "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.xml",
       "set_popup@chrome://browser/content/search/search.xml",
-      "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");