Bug 1103455 - Change the events used to popup the search suggestions to avoid showing them when opening context menus. r=florian, a=lsblakk
authorDave Townsend <dtownsend@oxymoronical.com>
Mon, 22 Dec 2014 11:54:15 -0800
changeset 234344 af822dde0f7950907f78e73e9c40235ab39890dc
parent 234343 2921722843b7dfc52d17b1ea50328c4c5e8e57f7
child 234345 ffd80298cdcbd6df2d2d4c233fbd812d4155b4bf
push id4270
push userryanvm@gmail.com
push dateMon, 22 Dec 2014 23:36:11 +0000
treeherdermozilla-beta@ddb6efaa11ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersflorian, lsblakk
bugs1103455
milestone35.0
Bug 1103455 - Change the events used to popup the search suggestions to avoid showing them when opening context menus. r=florian, a=lsblakk
browser/components/search/content/search.xml
browser/components/search/test/browser.ini
browser/components/search/test/browser_searchbar_openpopup.js
browser/components/search/test/head.js
toolkit/content/widgets/textbox.xml
--- a/browser/components/search/content/search.xml
+++ b/browser/components/search/content/search.xml
@@ -637,16 +637,18 @@
     <implementation>
       <destructor><![CDATA[
         // For some reason the destructor of the base binding is called
         // automatically when the window is closed, but now when the node
         // is removed from the DOM.
         this.destroy();
       ]]></destructor>
 
+      <field name="_ignoreFocus">false</field>
+
       <method name="selectEngine">
         <body><![CDATA[
           // Override this method to avoid accidentally changing the default
           // engine using the keyboard shortcuts of the old UI.
         ]]></body>
       </method>
 
       <method name="inputChanged">
@@ -680,26 +682,52 @@
             this.setAttribute("showonlysettings", "true");
           }
         ]]></body>
       </method>
     </implementation>
     <handlers>
       <handler event="input" action="this.inputChanged();"/>
       <handler event="drop" action="this.inputChanged();"/>
+
+      <handler event="blur">
+      <![CDATA[
+        // If the input field is still focused then a different window has
+        // received focus, ignore the next focus event.
+        this._ignoreFocus = (document.activeElement == this._textbox.inputField);
+      ]]></handler>
+
       <handler event="focus">
       <![CDATA[
-        if (this._textbox.value)
-          this.openSuggestionsPanel();
+        if (this._ignoreFocus) {
+          // This window has been re-focused, don't show the suggestions
+          this._ignoreFocus = false;
+          return;
+        }
+
+        // Don't open the suggestions if there is no text in the textbox.
+        if (!this._textbox.value)
+          return;
+
+        // Don't open the suggestions if the mouse was used to focus the
+        // textbox, that will be taken care of in the click handler.
+        if (Services.focus.getLastFocusMethod(window) == Services.focus.FLAG_BYMOUSE)
+          return;
+
+        this.openSuggestionsPanel();
       ]]></handler>
 
-      <handler event="click">
+      <handler event="click" button="0">
       <![CDATA[
-        if (event.originalTarget.getAttribute("anonid") == "searchbar-search-button")
+        // Open the suggestions whenever clicking on the search icon or if there
+        // is text in the textbox.
+        if (event.originalTarget.getAttribute("anonid") == "searchbar-search-button" ||
+            this._textbox.value) {
           this.openSuggestionsPanel(true);
+        }
       ]]></handler>
 
     </handlers>
   </binding>
 
   <binding id="searchbar-textbox"
       extends="chrome://global/content/bindings/autocomplete.xml#autocomplete">
     <implementation implements="nsIObserver">
--- a/browser/components/search/test/browser.ini
+++ b/browser/components/search/test/browser.ini
@@ -30,8 +30,10 @@ skip-if = e10s # Bug ?????? - some issue
 [browser_google_behavior.js]
 skip-if = e10s # Bug ?????? - some issue with progress listeners [JavaScript Error: "req.originalURI is null" {file: "chrome://mochitests/content/browser/browser/components/search/test/browser_bing_behavior.js" line: 127}]
 [browser_healthreport.js]
 [browser_private_search_perwindowpb.js]
 skip-if = e10s # Bug ?????? - Test uses load event and checks event.target.
 [browser_yahoo.js]
 [browser_yahoo_behavior.js]
 skip-if = e10s # Bug ?????? - some issue with progress listeners [JavaScript Error: "req.originalURI is null" {file: "chrome://mochitests/content/browser/browser/components/search/test/browser_bing_behavior.js" line: 127}]
+[browser_searchbar_openpopup.js]
+skip-if = os == "linux" || e10s # Linux has different focus behaviours and e10s seems to have timing issues.
new file mode 100644
--- /dev/null
+++ b/browser/components/search/test/browser_searchbar_openpopup.js
@@ -0,0 +1,297 @@
+// Tests that the suggestion popup appears at the right times in response to
+// focus and clicks.
+
+const searchbar = document.getElementById("searchbar");
+const searchIcon = document.getAnonymousElementByAttribute(searchbar, "anonid", "searchbar-search-button");
+const textbox = searchbar._textbox;
+const searchPopup = document.getElementById("PopupSearchAutoComplete");
+
+function promiseNewEngine(basename) {
+  return new Promise((resolve, reject) => {
+    info("Waiting for engine to be added: " + basename);
+    Services.search.init({
+      onInitComplete: function() {
+        let url = getRootDirectory(gTestPath) + basename;
+        let current = Services.search.currentEngine;
+        Services.search.addEngine(url, Ci.nsISearchEngine.TYPE_MOZSEARCH, "", false, {
+          onSuccess: function (engine) {
+            info("Search engine added: " + basename);
+            Services.search.currentEngine = engine;
+            registerCleanupFunction(() => {
+              Services.search.currentEngine = current;
+              Services.search.removeEngine(engine);
+              info("Search engine removed: " + basename);
+            });
+            resolve(engine);
+          },
+          onError: function (errCode) {
+            ok(false, "addEngine failed with error code " + errCode);
+            reject();
+          }
+        });
+      }
+    });
+  });
+}
+
+add_task(function* init() {
+  yield promiseNewEngine("testEngine.xml");
+});
+
+// Adds a task that shouldn't show the search suggestions popup.
+function add_no_popup_task(task) {
+  add_task(function*() {
+    let sawPopup = false;
+    function listener() {
+      sawPopup = true;
+    }
+
+    info("Entering test " + task.name);
+    searchPopup.addEventListener("popupshowing", listener, false);
+    yield Task.spawn(task);
+    searchPopup.removeEventListener("popupshowing", listener, false);
+    ok(!sawPopup, "Shouldn't have seen the suggestions popup");
+    info("Leaving test " + task.name);
+  });
+}
+
+// Simulates the full set of events for a context click
+function context_click(target) {
+  for (let event of ["mousedown", "contextmenu", "mouseup"])
+    EventUtils.synthesizeMouseAtCenter(target, { type: event, button: 2 });
+}
+
+// Right clicking the icon should not open the popup.
+add_no_popup_task(function* open_icon_context() {
+  gURLBar.focus();
+  let toolbarPopup = document.getElementById("toolbar-context-menu");
+
+  let promise = promiseEvent(toolbarPopup, "popupshown");
+  context_click(searchIcon);
+  yield promise;
+
+  promise = promiseEvent(toolbarPopup, "popuphidden");
+  toolbarPopup.hidePopup();
+  yield promise;
+});
+
+// With no text in the search box left clicking the icon should open the popup.
+add_task(function* open_empty() {
+  gURLBar.focus();
+
+  let promise = promiseEvent(searchPopup, "popupshown");
+  info("Clicking icon");
+  EventUtils.synthesizeMouseAtCenter(searchIcon, {});
+  yield promise;
+  is(searchPopup.getAttribute("showonlysettings"), "true", "Should only show the settings");
+
+  promise = promiseEvent(searchPopup, "popuphidden");
+  info("Hiding popup");
+  searchPopup.hidePopup();
+  yield promise;
+});
+
+// With no text in the search box left clicking it should not open the popup.
+add_no_popup_task(function* click_doesnt_open_popup() {
+  gURLBar.focus();
+
+  EventUtils.synthesizeMouseAtCenter(textbox, {});
+  is(Services.focus.focusedElement, textbox.inputField, "Should have focused the search bar");
+  is(textbox.selectionStart, 0, "Should have selected all of the text");
+  is(textbox.selectionEnd, 0, "Should have selected all of the text");
+});
+
+// Left clicking in a non-empty search box when unfocused should focus it and open the popup.
+add_task(function* click_opens_popup() {
+  gURLBar.focus();
+  textbox.value = "foo";
+
+  let promise = promiseEvent(searchPopup, "popupshown");
+  EventUtils.synthesizeMouseAtCenter(textbox, {});
+  yield promise;
+  isnot(searchPopup.getAttribute("showonlysettings"), "true", "Should show the full popup");
+
+  is(Services.focus.focusedElement, textbox.inputField, "Should have focused the search bar");
+  is(textbox.selectionStart, 0, "Should have selected all of the text");
+  is(textbox.selectionEnd, 3, "Should have selected all of the text");
+
+  promise = promiseEvent(searchPopup, "popuphidden");
+  searchPopup.hidePopup();
+  yield promise;
+
+  textbox.value = "";
+});
+
+// Right clicking in a non-empty search box when unfocused should open the edit context menu.
+add_no_popup_task(function* right_click_doesnt_open_popup() {
+  gURLBar.focus();
+  textbox.value = "foo";
+
+  let contextPopup = document.getAnonymousElementByAttribute(textbox.inputField.parentNode, "anonid", "input-box-contextmenu");
+  let promise = promiseEvent(contextPopup, "popupshown");
+  context_click(textbox);
+  yield promise;
+
+  is(Services.focus.focusedElement, textbox.inputField, "Should have focused the search bar");
+  is(textbox.selectionStart, 0, "Should have selected all of the text");
+  is(textbox.selectionEnd, 3, "Should have selected all of the text");
+
+  promise = promiseEvent(contextPopup, "popuphidden");
+  contextPopup.hidePopup();
+  yield promise;
+
+  textbox.value = "";
+});
+
+// Moving focus away from the search box should close the popup
+add_task(function* focus_change_closes_popup() {
+  gURLBar.focus();
+  textbox.value = "foo";
+
+  let promise = promiseEvent(searchPopup, "popupshown");
+  EventUtils.synthesizeMouseAtCenter(textbox, {});
+  yield promise;
+  isnot(searchPopup.getAttribute("showonlysettings"), "true", "Should show the full popup");
+
+  is(Services.focus.focusedElement, textbox.inputField, "Should have focused the search bar");
+  is(textbox.selectionStart, 0, "Should have selected all of the text");
+  is(textbox.selectionEnd, 3, "Should have selected all of the text");
+
+  promise = promiseEvent(searchPopup, "popuphidden");
+  let promise2 = promiseEvent(searchbar, "blur");
+  EventUtils.synthesizeKey("VK_TAB", { shiftKey: true });
+  yield promise;
+  yield promise2;
+
+  textbox.value = "";
+});
+
+// Pressing escape should close the popup.
+add_task(function* escape_closes_popup() {
+  gURLBar.focus();
+  textbox.value = "foo";
+
+  let promise = promiseEvent(searchPopup, "popupshown");
+  EventUtils.synthesizeMouseAtCenter(textbox, {});
+  yield promise;
+  isnot(searchPopup.getAttribute("showonlysettings"), "true", "Should show the full popup");
+
+  is(Services.focus.focusedElement, textbox.inputField, "Should have focused the search bar");
+  is(textbox.selectionStart, 0, "Should have selected all of the text");
+  is(textbox.selectionEnd, 3, "Should have selected all of the text");
+
+  promise = promiseEvent(searchPopup, "popuphidden");
+  EventUtils.synthesizeKey("VK_ESCAPE", {});
+  yield promise;
+
+  textbox.value = "";
+});
+
+// Tabbing to the search box should open the popup if it contains text.
+add_task(function* tab_opens_popup() {
+  gURLBar.focus();
+  textbox.value = "foo";
+
+  let promise = promiseEvent(searchPopup, "popupshown");
+  EventUtils.synthesizeKey("VK_TAB", {});
+  yield promise;
+  isnot(searchPopup.getAttribute("showonlysettings"), "true", "Should show the full popup");
+
+  is(Services.focus.focusedElement, textbox.inputField, "Should have focused the search bar");
+  is(textbox.selectionStart, 0, "Should have selected all of the text");
+  is(textbox.selectionEnd, 3, "Should have selected all of the text");
+
+  promise = promiseEvent(searchPopup, "popuphidden");
+  searchPopup.hidePopup();
+  yield promise;
+
+  textbox.value = "";
+});
+
+// Tabbing to the search box should not open the popup if it doesn't contain text.
+add_no_popup_task(function* tab_doesnt_open_popup() {
+  gURLBar.focus();
+  textbox.value = "foo";
+
+  EventUtils.synthesizeKey("VK_TAB", {});
+
+  is(Services.focus.focusedElement, textbox.inputField, "Should have focused the search bar");
+  is(textbox.selectionStart, 0, "Should have selected all of the text");
+  is(textbox.selectionEnd, 3, "Should have selected all of the text");
+
+  textbox.value = "";
+});
+
+// Switching back to the window when the search box has focus from mouse should not open the popup.
+add_task(function* refocus_window_doesnt_open_popup_mouse() {
+  gURLBar.focus();
+  textbox.value = "foo";
+
+  let promise = promiseEvent(searchPopup, "popupshown");
+  EventUtils.synthesizeMouseAtCenter(searchbar, {});
+  yield promise;
+  isnot(searchPopup.getAttribute("showonlysettings"), "true", "Should show the full popup");
+
+  is(Services.focus.focusedElement, textbox.inputField, "Should have focused the search bar");
+  is(textbox.selectionStart, 0, "Should have selected all of the text");
+  is(textbox.selectionEnd, 3, "Should have selected all of the text");
+
+  promise = promiseEvent(searchPopup, "popuphidden");
+  let newWin = OpenBrowserWindow();
+  yield new Promise(resolve => waitForFocus(resolve, newWin));
+  yield promise;
+
+  function listener() {
+    ok(false, "Should not have shown the popup.");
+  }
+  searchPopup.addEventListener("popupshowing", listener, false);
+
+  promise = promiseEvent(searchbar, "focus");
+  newWin.close();
+  yield promise;
+
+  // Wait a few ticks to allow any focus handlers to show the popup if they are going to.
+  yield new Promise(resolve => executeSoon(resolve));
+  yield new Promise(resolve => executeSoon(resolve));
+  yield new Promise(resolve => executeSoon(resolve));
+
+  searchPopup.removeEventListener("popupshowing", listener, false);
+  textbox.value = "";
+});
+
+// Switching back to the window when the search box has focus from keyboard should not open the popup.
+add_task(function* refocus_window_doesnt_open_popup_keyboard() {
+  gURLBar.focus();
+  textbox.value = "foo";
+
+  let promise = promiseEvent(searchPopup, "popupshown");
+  EventUtils.synthesizeKey("VK_TAB", {});
+  yield promise;
+  isnot(searchPopup.getAttribute("showonlysettings"), "true", "Should show the full popup");
+
+  is(Services.focus.focusedElement, textbox.inputField, "Should have focused the search bar");
+  is(textbox.selectionStart, 0, "Should have selected all of the text");
+  is(textbox.selectionEnd, 3, "Should have selected all of the text");
+
+  promise = promiseEvent(searchPopup, "popuphidden");
+  let newWin = OpenBrowserWindow();
+  yield new Promise(resolve => waitForFocus(resolve, newWin));
+  yield promise;
+
+  function listener() {
+    ok(false, "Should not have shown the popup.");
+  }
+  searchPopup.addEventListener("popupshowing", listener, false);
+
+  promise = promiseEvent(searchbar, "focus");
+  newWin.close();
+  yield promise;
+
+  // Wait a few ticks to allow any focus handlers to show the popup if they are going to.
+  yield new Promise(resolve => executeSoon(resolve));
+  yield new Promise(resolve => executeSoon(resolve));
+  yield new Promise(resolve => executeSoon(resolve));
+
+  searchPopup.removeEventListener("popupshowing", listener, false);
+  textbox.value = "";
+});
--- a/browser/components/search/test/head.js
+++ b/browser/components/search/test/head.js
@@ -86,17 +86,17 @@ function waitForPopupShown(aPopupId, aCa
   }
   function removePopupShownListener() {
     popup.removeEventListener("popupshown", onPopupShown);
   }
   popup.addEventListener("popupshown", onPopupShown);
   registerCleanupFunction(removePopupShownListener);
 }
 
-function* promiseEvent(aTarget, aEventName, aPreventDefault) {
+function promiseEvent(aTarget, aEventName, aPreventDefault) {
   let deferred = Promise.defer();
   aTarget.addEventListener(aEventName, function onEvent(aEvent) {
     aTarget.removeEventListener(aEventName, onEvent, true);
     if (aPreventDefault) {
       aEvent.preventDefault();
     }
     deferred.resolve();
   }, true);
--- a/toolkit/content/widgets/textbox.xml
+++ b/toolkit/content/widgets/textbox.xml
@@ -236,16 +236,19 @@
           }
         ]]>
       </handler>
 
       <handler event="click" action="this._maybeSelectAll();"/>
 
 #ifndef XP_WIN
       <handler event="contextmenu">
+        // Only care about context clicks on the input field itself
+        if (event.target != this.inputField)
+          return;
         if (!event.button) // context menu opened via keyboard shortcut
           return;
         this._maybeSelectAll();
         // see bug 576135 comment 4
         let box = this.inputField.parentNode;
         let menu = document.getAnonymousElementByAttribute(box, "anonid", "input-box-contextmenu");
         box._doPopupItemEnabling(menu);
       </handler>