Bug 1384050 - Search suggestion contextual hint should only be shown when a user manually focuses the URL bar. r=paolo draft
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 30 Aug 2017 18:03:00 +0200
changeset 658807 0d79e6fdc3c0a6904b3b49272fedbdced8f63116
parent 658578 632e42dca494ec3d90b70325d9c359f80cb3f38a
child 729753 dae5329a78686fedad31bd353f3b0481222e409a
push id77875
push usermak77@bonardo.net
push dateMon, 04 Sep 2017 21:54:15 +0000
reviewerspaolo
bugs1384050
milestone57.0a1
Bug 1384050 - Search suggestion contextual hint should only be shown when a user manually focuses the URL bar. r=paolo MozReview-Commit-ID: IVbHYdtYJgx
browser/base/content/browser.js
browser/base/content/test/urlbar/browser_urlbarSearchSuggestions_opt-out.js
browser/base/content/urlbarBindings.xml
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -2234,43 +2234,56 @@ function loadOneOrMoreURIs(aURIString, a
       inBackground: false,
       replace: true,
       triggeringPrincipal: aTriggeringPrincipal,
     });
   } catch (e) {
   }
 }
 
-function focusAndSelectUrlBar() {
+/**
+ * Focuses the location bar input field and selects its contents.
+ *
+ * @param [optional] userInitiatedFocus
+ *        Whether this focus is caused by an user interaction whose intention
+ *        was to use the location bar. For example, using a shortcut to go to
+ *        the location bar, or a contextual menu to search from it.
+ *        The default is false and should be used in all those cases where the
+ *        code focuses the location bar but that's not the primary user
+ *        intention, like when opening a new tab.
+ */
+function focusAndSelectUrlBar(userInitiatedFocus = false) {
   // In customize mode, the url bar is disabled. If a new tab is opened or the
   // user switches to a different tab, this function gets called before we've
   // finished leaving customize mode, and the url bar will still be disabled.
   // We can't focus it when it's disabled, so we need to re-run ourselves when
   // we've finished leaving customize mode.
   if (CustomizationHandler.isExitingCustomizeMode) {
     gNavToolbox.addEventListener("aftercustomization", function() {
-      focusAndSelectUrlBar();
+      focusAndSelectUrlBar(userInitiatedFocus);
     }, {once: true});
 
     return true;
   }
 
   if (gURLBar) {
     if (window.fullScreen)
       FullScreen.showNavToolbox();
 
+    gURLBar.userInitiatedFocus = userInitiatedFocus;
     gURLBar.select();
+    gURLBar.userInitiatedFocus = false;
     if (document.activeElement == gURLBar.inputField)
       return true;
   }
   return false;
 }
 
 function openLocation() {
-  if (focusAndSelectUrlBar())
+  if (focusAndSelectUrlBar(true))
     return;
 
   if (window.location.href != getBrowserURL()) {
     var win = getTopWin();
     if (win) {
       // If there's an open browser window, it should handle this command
       win.focus()
       win.openLocation();
@@ -3810,17 +3823,17 @@ const BrowserSearch = {
                                 "chrome,all,dialog=no", "about:blank");
         Services.obs.addObserver(observer, "browser-delayed-startup-finished");
       }
       return;
     }
 
     let focusUrlBarIfSearchFieldIsNotActive = function(aSearchBar) {
       if (!aSearchBar || document.activeElement != aSearchBar.textbox.inputField) {
-        focusAndSelectUrlBar();
+        focusAndSelectUrlBar(true);
       }
     };
 
     let searchBar = this.searchBar;
     let placement = CustomizableUI.getPlacementOfWidget("search-container");
     let focusSearchBar = () => {
       searchBar = this.searchBar;
       searchBar.select();
--- a/browser/base/content/test/urlbar/browser_urlbarSearchSuggestions_opt-out.js
+++ b/browser/base/content/test/urlbar/browser_urlbarSearchSuggestions_opt-out.js
@@ -1,8 +1,9 @@
+/* eslint-disable mozilla/no-arbitrary-setTimeout */
 // The order of the tests here matters!
 
 const SUGGEST_ALL_PREF = "browser.search.suggest.enabled";
 const SUGGEST_URLBAR_PREF = "browser.urlbar.suggest.searches";
 const CHOICE_PREF = "browser.urlbar.userMadeSearchSuggestionsChoice";
 const TIMES_PREF = "browser.urlbar.timesBeforeHidingSuggestionsHint";
 const TEST_ENGINE_BASENAME = "searchSuggestionEngine.xml";
 const ONEOFF_PREF = "browser.urlbar.oneOffSearches";
@@ -33,17 +34,17 @@ add_task(async function prepare() {
 });
 
 add_task(async function focus() {
   // Focusing the urlbar should open the popup in order to show the
   // notification.
   setupVisibleHint();
   gURLBar.blur();
   let popupPromise = promisePopupShown(gURLBar.popup);
-  gURLBar.focus();
+  focusAndSelectUrlBar(true);
   await popupPromise;
   Assert.ok(gURLBar.popup.popupOpen, "popup should be open");
   assertVisible(true);
   assertFooterVisible(false);
   Assert.equal(gURLBar.popup._matchCount, 0, "popup should have no results");
 
   // Start searching.
   EventUtils.synthesizeKey("r", {});
@@ -59,31 +60,48 @@ add_task(async function focus() {
   let prefsPromise = BrowserTestUtils.waitForLocationChange(gBrowser, "about:preferences#search");
   changeOptionsLink.click();
   await prefsPromise;
   Assert.ok(!gURLBar.popup.popupOpen, "popup should be closed");
   // The preferences page does fancy stuff with focus, ensure to unload it.
   await BrowserTestUtils.loadURI(gBrowser.selectedBrowser, "about:blank");
 });
 
-add_task(async function new_tab() {
-  // Opening a new tab when the urlbar is unfocused, should focusing it and thus
-  // open the popup in order to show the notification.
+add_task(async function click_on_focused() {
+  // Even if the location bar is already focused, we should still show the popup
+  // and the notification on click.
   setupVisibleHint();
   gURLBar.blur();
+  // Won't show the hint since it's not user initiated.
+  gURLBar.focus();
+  await new Promise(resolve => setTimeout(resolve, 500));
+  Assert.ok(!gURLBar.popup.popupOpen, "popup should be closed");
+
   let popupPromise = promisePopupShown(gURLBar.popup);
-  // openNewForegroundTab doesn't focus the urlbar.
-  await BrowserTestUtils.synthesizeKey("t", { accelKey: true }, gBrowser.selectedBrowser);
+  EventUtils.synthesizeMouseAtCenter(gURLBar.inputField, { button: 0, type: "mousedown" });
   await popupPromise;
+
   Assert.ok(gURLBar.popup.popupOpen, "popup should be open");
   assertVisible(true);
   assertFooterVisible(false);
   Assert.equal(gURLBar.popup._matchCount, 0, "popup should have no results");
+  gURLBar.blur();
+  Assert.ok(!gURLBar.popup.popupOpen, "popup should be closed");
+});
+
+add_task(async function new_tab() {
+  // Opening a new tab when the urlbar is unfocused, should focus it but not
+  // open the popup.
+  setupVisibleHint();
+  gURLBar.blur();
+  // openNewForegroundTab doesn't focus the urlbar.
+  await BrowserTestUtils.synthesizeKey("t", { accelKey: true }, gBrowser.selectedBrowser);
+  await new Promise(resolve => setTimeout(resolve, 500));
+  Assert.ok(!gURLBar.popup.popupOpen, "popup should be closed");
   await BrowserTestUtils.removeTab(gBrowser.selectedTab);
-  Assert.ok(!gURLBar.popup.popupOpen, "popup should be closed");
 });
 
 add_task(async function privateWindow() {
   // Since suggestions are disabled in private windows, the notification should
   // not appear even when suggestions are otherwise enabled.
   setupVisibleHint();
   let win = await BrowserTestUtils.openNewBrowserWindow({ private: true });
   await promiseAutocompleteResultPopup("foo", win);
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -183,16 +183,22 @@ file, You can obtain one at http://mozil
 
       <!--
         Since we never want scrollbars, we always use the maxResults value.
       -->
       <property name="maxRows"
                 onget="return this.popup.maxResults;"/>
 
       <!--
+        Set by focusAndSelectUrlBar to indicate whether the next focus event was
+        initiated by an explicit user action. See the "focus" handler below.
+      -->
+      <field name="userInitiatedFocus">false</field>
+
+      <!--
         onBeforeValueGet is called by the base-binding's .value getter.
         It can return an object with a "value" property, to override the
         return value of the getter.
       -->
       <method name="onBeforeValueGet">
         <body><![CDATA[
           return { value: this._value };
         ]]></body>
@@ -1448,16 +1454,37 @@ file, You can obtain one at http://mozil
           let previousDate = this._prefs.getIntPref("lastSuggestionsPromptDate");
           if (!useDays || previousDate != date) {
             this._prefs.setIntPref(prefName, remaining - 1);
           }
           this._prefs.setIntPref("lastSuggestionsPromptDate", date);
         ]]></body>
       </method>
 
+      <method name="maybeShowSearchSuggestionsNotificationOnFocus">
+        <parameter name="mouseFocused"/>
+        <body><![CDATA[
+          let whichNotification = this.whichSearchSuggestionsNotification;
+          if (this._showSearchSuggestionNotificationOnMouseFocus &&
+              mouseFocused) {
+            // Force showing the opt-out notification.
+            this._whichSearchSuggestionsNotification = whichNotification = "opt-out";
+          }
+          if (whichNotification == "opt-out") {
+            try {
+              this.popup.openAutocompletePopup(this, this);
+            } finally {
+              if (mouseFocused) {
+                delete this._whichSearchSuggestionsNotification;
+                this._showSearchSuggestionNotificationOnMouseFocus = false;
+              }
+            }
+          }
+        ]]></body>
+      </method>
     </implementation>
 
     <handlers>
       <handler event="keydown"><![CDATA[
         if (this._noActionKeys.has(event.keyCode) &&
             this.popup.selectedIndex >= 0 &&
             !this._pressedNoActionKeys.has(event.keyCode)) {
           if (this._pressedNoActionKeys.size == 0) {
@@ -1472,52 +1499,51 @@ file, You can obtain one at http://mozil
         if (this._noActionKeys.has(event.keyCode) &&
             this._pressedNoActionKeys.has(event.keyCode)) {
           this._pressedNoActionKeys.delete(event.keyCode);
           if (this._pressedNoActionKeys.size == 0)
             this._clearNoActions();
         }
       ]]></handler>
 
+      <handler event="mousedown"><![CDATA[
+        // Eventually show the opt-out notification even if the location bar is
+        // empty, focused, and the user clicks on it.
+        if (event.button == 0 && this.focused && this.textValue == "") {
+          this.maybeShowSearchSuggestionsNotificationOnFocus(true);
+        }
+      ]]></handler>
+
       <handler event="focus"><![CDATA[
         if (event.originalTarget == this.inputField) {
           this._hideURLTooltip();
           this.formatValue();
           if (this.getAttribute("pageproxystate") != "valid") {
             UpdatePopupNotificationsVisibility();
           }
 
-          // We show the opt-out notification on every kind of focus to the urlbar
-          // included opening a new tab, but we want to enforce at least one
+          // We show the opt-out notification when the mouse/keyboard focus the
+          // urlbar, but in any case we want to enforce at least one
           // notification when the user focuses it with the mouse.
           let whichNotification = this.whichSearchSuggestionsNotification;
           if (whichNotification == "opt-out" &&
               this._showSearchSuggestionNotificationOnMouseFocus === undefined) {
             this._showSearchSuggestionNotificationOnMouseFocus = true;
           }
 
-          // Check whether the focus change came from a user mouse action.
+          // Check whether the focus change came from a keyboard/mouse action.
           let focusMethod = Services.focus.getLastFocusMethod(window);
-          let mouseFocused = !!(focusMethod & Services.focus.FLAG_BYMOUSE);
-          if (this._showSearchSuggestionNotificationOnMouseFocus &&
-              mouseFocused) {
-            // Force showing the opt-out notification.
-            this._whichSearchSuggestionsNotification = whichNotification = "opt-out";
+          // If it's a focus started by code and the primary user intention was
+          // not to go to the location bar, don't show a notification.
+          if (!focusMethod && !this.userInitiatedFocus) {
+            return;
           }
 
-          if (whichNotification == "opt-out") {
-            try {
-              this.popup.openAutocompletePopup(this, this);
-            } finally {
-              if (mouseFocused) {
-                delete this._whichSearchSuggestionsNotification;
-                this._showSearchSuggestionNotificationOnMouseFocus = false;
-              }
-            }
-          }
+          let mouseFocused = !!(focusMethod & Services.focus.FLAG_BYMOUSE);
+          this.maybeShowSearchSuggestionsNotificationOnFocus(mouseFocused);
         }
       ]]></handler>
 
       <handler event="blur"><![CDATA[
         if (event.originalTarget == this.inputField) {
           this._clearNoActions();
           this.formatValue();
           if (this.getAttribute("pageproxystate") != "valid") {