Bug 1118135 - Clicking the magnifying glass while the suggestions are open should close the popup and not re-open it. r=felipe, a=sledru
authorDave Townsend <dtownsend@oxymoronical.com>
Tue, 06 Jan 2015 11:05:32 -0800
changeset 242881 ee9df2674663
parent 242880 2fd253435fe4
child 242882 f82a118e1064
push id4328
push userryanvm@gmail.com
push date2015-01-15 22:26 +0000
treeherdermozilla-beta@c8031be76a86 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfelipe, sledru
bugs1118135
milestone36.0
Bug 1118135 - Clicking the magnifying glass while the suggestions are open should close the popup and not re-open it. r=felipe, a=sledru The popup gets closed when native events trigger rollup. This is before the any mousedown event reaches the DOM so it is difficult to detect the case where it was a click on the magnifying glass icon that closed the popup. Here a field is added to the popup that is set to true when rollup is triggered and false on the next tick of the event loop which will be after the mousedown event reaches the DOM. This allows us to detect the case and ignore the click on the magnifying glass.
browser/base/content/urlbarBindings.xml
browser/components/search/content/search.xml
browser/components/search/test/browser_searchbar_openpopup.js
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -996,16 +996,22 @@
       <xul:vbox anonid="add-engines"/>
       <xul:button anonid="search-settings"
                   xbl:inherits="showonlysettings"
                   oncommand="openPreferences('paneSearch')"
                   class="search-setting-button search-panel-header"
                   label="&changeSearchSettings.button;"/>
     </content>
     <implementation>
+      <!-- Popup rollup is triggered by native events before the mousedown event
+           reaches the DOM. The will be set to true by the popuphiding event and
+           false after the mousedown event has been triggered to detect what
+           caused rollup. -->
+      <field name="_isHiding">false</field>
+
       <method name="updateHeader">
         <body><![CDATA[
           let currentEngine = Services.search.currentEngine;
           let uri = currentEngine.iconURI;
           if (uri) {
             uri = uri.spec;
             this.setAttribute("src", PlacesUtils.getImageURLForResolution(window, uri));
           }
@@ -1241,16 +1247,23 @@
             }
           }
           Services.search.addEngine(target.getAttribute("uri"),
                                     Ci.nsISearchEngine.DATA_XML,
                                     target.getAttribute("src"), false,
                                     installCallback);
         }
       ]]></handler>
+
+      <handler event="popuphiding"><![CDATA[
+        this._isHiding = true;
+        setTimeout(() => {
+          this._isHiding = false;
+        }, 0);
+      ]]></handler>
     </handlers>
   </binding>
 
   <!-- Used for additional open search providers in the search panel. -->
   <binding id="addengine-icon" extends="xul:box">
     <content>
       <xul:image class="addengine-icon" xbl:inherits="src"/>
       <xul:image class="addengine-badge"/>
--- a/browser/components/search/content/search.xml
+++ b/browser/components/search/content/search.xml
@@ -644,16 +644,17 @@
       <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>
+      <field name="_clickClosedPopup">false</field>
 
       <method name="rebuildPopup">
         <body><![CDATA[
           this._textbox.popup.updateHeader();
         ]]></body>
       </method>
 
       <method name="inputChanged">
@@ -715,27 +716,40 @@
         // 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="mousedown" phase="capturing">
+      <![CDATA[
+        if (event.originalTarget.getAttribute("anonid") == "searchbar-search-button") {
+          this._clickClosedPopup = this._textbox.popup._isHiding;
+        }
+      ]]></handler>
+
       <handler event="click" button="0">
       <![CDATA[
         // Ignore clicks on the search go button.
         if (event.originalTarget.getAttribute("anonid") == "search-go-button") {
           return;
         }
 
+        let isIconClick = event.originalTarget.getAttribute("anonid") == "searchbar-search-button";
+
+        // Ignore clicks on the icon if they were made to close the popup
+        if (isIconClick && this._clickClosedPopup) {
+          return;
+        }
+
         // 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) {
+        if (isIconClick || this._textbox.value) {
           this.openSuggestionsPanel(true);
         }
       ]]></handler>
 
     </handlers>
   </binding>
 
   <binding id="searchbar-textbox"
--- a/browser/components/search/test/browser_searchbar_openpopup.js
+++ b/browser/components/search/test/browser_searchbar_openpopup.js
@@ -2,16 +2,33 @@
 // focus and clicks.
 
 const searchbar = document.getElementById("searchbar");
 const searchIcon = document.getAnonymousElementByAttribute(searchbar, "anonid", "searchbar-search-button");
 const goButton = document.getAnonymousElementByAttribute(searchbar, "anonid", "search-go-button");
 const textbox = searchbar._textbox;
 const searchPopup = document.getElementById("PopupSearchAutoComplete");
 
+const isWindows = Services.appinfo.OS == "WINNT";
+const mouseDown = isWindows ? 2 : 1;
+const mouseUp = isWindows ? 4 : 2;
+const utils = window.QueryInterface(Ci.nsIInterfaceRequestor)
+                    .getInterface(Ci.nsIDOMWindowUtils);
+const scale = utils.screenPixelsPerCSSPixel;
+
+function synthesizeNativeMouseClick(aElement) {
+  let rect = aElement.getBoundingClientRect();
+  let win = aElement.ownerDocument.defaultView;
+  let x = win.mozInnerScreenX + (rect.left + rect.right) / 2;
+  let y = win.mozInnerScreenY + (rect.top + rect.bottom) / 2;
+
+  utils.sendNativeMouseEvent(x * scale, y * scale, mouseDown, 0, null);
+  utils.sendNativeMouseEvent(x * scale, y * scale, mouseUp, 0, null);
+}
+
 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, {
@@ -72,29 +89,46 @@ add_no_popup_task(function* open_icon_co
   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.
+// Clicking the icon again should hide the popup and not show it again.
 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");
+  is(textbox.mController.searchString, "", "Should be an empty search string");
+
+  // By giving the textbox some text any next attempt to open the search popup
+  // from the click handler will try to search for this text.
+  textbox.value = "foo";
 
   promise = promiseEvent(searchPopup, "popuphidden");
+  let clickPromise = promiseEvent(searchIcon, "click");
+
   info("Hiding popup");
-  searchPopup.hidePopup();
+  synthesizeNativeMouseClick(searchIcon);
   yield promise;
+
+  is(textbox.mController.searchString, "", "Should not have started to search for the new text");
+
+  // Cancel the search if it started.
+  if (textbox.mController.searchString != "") {
+    textbox.mController.stopSearch();
+  }
+
+  textbox.value = "";
 });
 
 // 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");
@@ -218,39 +252,16 @@ add_no_popup_task(function* tab_doesnt_o
 
   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 = "";
 });
 
-// Clicks outside the search popup should close the popup but not consume the click.
-add_task(function* dont_consume_clicks() {
-  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.synthesizeMouseAtCenter(gURLBar, {});
-  yield promise;
-
-  is(Services.focus.focusedElement, gURLBar.inputField, "Should have focused the URL bar");
-
-  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;
@@ -330,8 +341,31 @@ add_no_popup_task(function* search_go_do
 
   let promise = promiseOnLoad();
   EventUtils.synthesizeMouseAtCenter(goButton, {});
   yield promise;
 
   textbox.value = "";
   gBrowser.removeCurrentTab();
 });
+
+// Clicks outside the search popup should close the popup but not consume the click.
+add_task(function* dont_consume_clicks() {
+  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");
+  synthesizeNativeMouseClick(gURLBar);
+  yield promise;
+
+  is(Services.focus.focusedElement, gURLBar.inputField, "Should have focused the URL bar");
+
+  textbox.value = "";
+});