Bug 1384050 - Search suggestion contextual hint should only be shown when a user manually focuses the URL bar. r=Paolo
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 30 Aug 2017 18:03:00 +0200
changeset 378809 3cc38c1489e4
parent 378808 268d45f4eca3
child 378810 68b71f2b4982
push id32442
push userarchaeopteryx@coole-files.de
push dateTue, 05 Sep 2017 09:39:11 +0000
treeherdermozilla-central@35bd47b6e5ac [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersPaolo
bugs1384050
milestone57.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 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") {