Bug 1295458 - Rework key and mouse handling for the one-off search buttons. r?florian draft
authorDrew Willcoxon <adw@mozilla.com>
Tue, 11 Apr 2017 08:40:11 -0700
changeset 560539 867eb62fed8e39a261f02c25a07fb1f89437f4b8
parent 560424 f914d40a48009c5acd1093e9939cc0ec035696dd
child 623730 7413db3df9f28749b8fd678c946e0b52438f27cd
push id53451
push userdwillcoxon@mozilla.com
push dateTue, 11 Apr 2017 15:41:03 +0000
reviewersflorian
bugs1295458
milestone55.0a1
Bug 1295458 - Rework key and mouse handling for the one-off search buttons. r?florian MozReview-Commit-ID: DKbU8r2BrA8
browser/base/content/test/urlbar/browser.ini
browser/base/content/test/urlbar/browser_urlbarOneOffs.js
browser/components/search/content/search.xml
browser/components/search/content/searchbarBindings.css
browser/components/search/test/browser_searchbar_keyboard_navigation.js
browser/themes/linux/browser.css
browser/themes/linux/searchbar.css
browser/themes/osx/browser.css
browser/themes/osx/searchbar.css
browser/themes/windows/browser.css
browser/themes/windows/searchbar.css
toolkit/content/widgets/autocomplete.xml
--- a/browser/base/content/test/urlbar/browser.ini
+++ b/browser/base/content/test/urlbar/browser.ini
@@ -64,16 +64,19 @@ support-files =
 [browser_urlbarDelete.js]
 [browser_urlbarEnter.js]
 [browser_urlbarEnterAfterMouseOver.js]
 skip-if = os == "linux" # Bug 1073339 - Investigate autocomplete test unreliability on Linux/e10s
 [browser_urlbarFocusedCmdK.js]
 [browser_urlbarHashChangeProxyState.js]
 [browser_urlbarKeepStateAcrossTabSwitches.js]
 [browser_urlbarOneOffs.js]
+support-files =
+  searchSuggestionEngine.xml
+  searchSuggestionEngine.sjs
 [browser_urlbarPrivateBrowsingWindowChange.js]
 [browser_urlbarRaceWithTabs.js]
 [browser_urlbarRevert.js]
 [browser_urlbarSearchSingleWordNotification.js]
 [browser_urlbarSearchSuggestions.js]
 support-files =
   searchSuggestionEngine.xml
   searchSuggestionEngine.sjs
--- a/browser/base/content/test/urlbar/browser_urlbarOneOffs.js
+++ b/browser/base/content/test/urlbar/browser_urlbarOneOffs.js
@@ -68,16 +68,28 @@ add_task(function* history() {
   assertState(-1, -1, "");
 
   // Key up through each one-off.
   for (let i = numButtons - 1; i >= 0; i--) {
     EventUtils.synthesizeKey("VK_UP", {})
     assertState(-1, i, "");
   }
 
+  // Key right through each one-off.
+  for (let i = 1; i < numButtons; i++) {
+    EventUtils.synthesizeKey("VK_RIGHT", {})
+    assertState(-1, i, "");
+  }
+
+  // Key left through each one-off.
+  for (let i = numButtons - 2; i >= 0; i--) {
+    EventUtils.synthesizeKey("VK_LEFT", {})
+    assertState(-1, i, "");
+  }
+
   // Key up through each result.
   for (let i = gMaxResults - 1; i >= 0; i--) {
     EventUtils.synthesizeKey("VK_UP", {})
     assertState(i, -1,
       "example.com/browser_urlbarOneOffs.js/?" + (gMaxResults - i - 1));
   }
 
   // Key up once more.  Nothing should be selected.
--- a/browser/components/search/content/search.xml
+++ b/browser/components/search/content/search.xml
@@ -921,17 +921,17 @@
                    role="presentation"/>
       </xul:hbox>
       <xul:tree anonid="tree" flex="1"
                 class="autocomplete-tree plain search-panel-tree"
                 hidecolumnpicker="true" seltype="single">
         <xul:treecols anonid="treecols">
           <xul:treecol id="treecolAutoCompleteValue" class="autocomplete-treecol" flex="1" overflow="true"/>
         </xul:treecols>
-        <xul:treechildren class="autocomplete-treebody"/>
+        <xul:treechildren class="autocomplete-treebody searchbar-treebody"/>
       </xul:tree>
       <xul:vbox anonid="search-one-off-buttons" class="search-one-offs"/>
     </content>
     <implementation>
       <field name="AppConstants" readonly="true">
         (Components.utils.import("resource://gre/modules/AppConstants.jsm", {})).AppConstants;
       </field>
 
@@ -1134,16 +1134,29 @@
           return;
         }
         this.oneOffButtons.handleSearchCommand(event, engine);
       ]]></handler>
     </handlers>
 
   </binding>
 
+
+  <!-- This is the same as the autocomplete-treebody binding except it does not
+       select rows on mousemove. -->
+  <binding id="searchbar-treebody"
+           extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-treebody">
+    <handlers>
+      <handler event="mousemove"><![CDATA[
+        // Cancel the event so that the base binding doesn't select the row.
+        event.preventDefault();
+      ]]></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"/>
     </content>
   </binding>
 
@@ -1302,17 +1315,37 @@
 
       <!-- The selected one-off, a xul:button, including the add-engine button
            and the search-settings button.  Null if no one-off is selected. -->
       <property name="selectedButton">
         <getter><![CDATA[
           return this._selectedButton;
         ]]></getter>
         <setter><![CDATA[
-          this._changeVisuallySelectedButton(val, true);
+          if (val && val.classList.contains("dummy")) {
+            // Never select dummy buttons.
+            val = null;
+          }
+          if (this._selectedButton) {
+            this._selectedButton.removeAttribute("selected");
+          }
+          if (val) {
+            val.setAttribute("selected", "true");
+          }
+          this._selectedButton = val;
+          this._updateStateForButton(null);
+          if (val && !val.engine) {
+            // If the button doesn't have an engine, then clear the popup's
+            // selection to indicate that pressing Return while the button is
+            // selected will do the button's command, not search.
+            this.popup.selectedIndex = -1;
+          }
+          let event = document.createEvent("Events");
+          event.initEvent("SelectedOneOffButtonChanged", true, false);
+          this.dispatchEvent(event);
           return val;
         ]]></setter>
       </property>
 
       <!-- The index of the selected one-off, including the add-engine button
            and the search-settings button.  -1 if no one-off is selected. -->
       <property name="selectedButtonIndex">
         <getter><![CDATA[
@@ -1326,30 +1359,16 @@
         ]]></getter>
         <setter><![CDATA[
           let buttons = this.getSelectableButtons(true);
           this.selectedButton = buttons[val];
           return val;
         ]]></setter>
       </property>
 
-      <!-- The visually selected one-off is the same as the selected one-off
-           unless a one-off is moused over.  In that case, the visually selected
-           one-off is the moused-over one-off, which may be different from the
-           selected one-off.  The visually selected one-off is always the one
-           that is visually highlighted.  Includes the add-engine button and the
-           search-settings button.  A xul:button. -->
-      <property name="visuallySelectedButton" readonly="true">
-        <getter><![CDATA[
-          return this.getSelectableButtons(true).find(button => {
-            return button.getAttribute("selected") == "true";
-          });
-        ]]></getter>
-      </property>
-
       <property name="compact" readonly="true">
         <getter><![CDATA[
           return this.getAttribute("compact") == "true";
         ]]></getter>
       </property>
 
       <property name="settingsButton" readonly="true">
         <getter><![CDATA[
@@ -1733,62 +1752,60 @@
 
       <method name="_buttonForEngine">
         <parameter name="engine"/>
         <body><![CDATA[
           return document.getElementById(this._buttonIDForEngine(engine));
         ]]></body>
       </method>
 
-      <method name="_changeVisuallySelectedButton">
-        <parameter name="val"/>
-        <parameter name="aUpdateLogicallySelectedButton"/>
+      <!--
+        Updates the popup and textbox for the currently selected or moused-over
+        button.
+
+        @param mousedOverButton
+               The currently moused-over button, or null if there isn't one.
+      -->
+      <method name="_updateStateForButton">
+        <parameter name="mousedOverButton"/>
         <body><![CDATA[
-          let visuallySelectedButton = this.visuallySelectedButton;
-          if (visuallySelectedButton)
-            visuallySelectedButton.removeAttribute("selected");
-
+          let button = mousedOverButton;
           let header =
             document.getAnonymousElementByAttribute(this, "anonid",
                                                     "search-panel-one-offs-header");
-          // Avoid selecting dummy buttons.
-          if (val && !val.classList.contains("dummy")) {
-            val.setAttribute("selected", "true");
-            if (val.classList.contains("searchbar-engine-one-off-item") &&
-                val.engine) {
-              let headerEngineText =
-                document.getAnonymousElementByAttribute(this, "anonid",
-                                                        "searchbar-oneoffheader-engine");
-              header.selectedIndex = 2;
-              headerEngineText.value = val.engine.name;
-            } else {
-              header.selectedIndex = this.query ? 1 : 0;
-            }
-            if (this.textbox) {
-              this.textbox.setAttribute("aria-activedescendant", val.id);
-            }
-          } else {
-            val = null;
+
+          // Ignore dummy buttons.
+          if (button && button.classList.contains("dummy")) {
+            button = null;
+          }
+
+          // If there's no moused-over button, then the one-offs should reflect
+          // the selected button, if any.
+          button = button || this.selectedButton;
+
+          if (!button) {
             header.selectedIndex = this.query ? 1 : 0;
             if (this.textbox) {
               this.textbox.removeAttribute("aria-activedescendant");
             }
+            return;
           }
 
-          if (aUpdateLogicallySelectedButton) {
-            this._selectedButton = val;
-            if (val && !val.engine) {
-              // If the button doesn't have an engine, then clear the popup's
-              // selection to indicate that pressing Return while the button is
-              // selected will do the button's command, not search.
-              this.popup.selectedIndex = -1;
-            }
-            let event = document.createEvent("Events");
-            event.initEvent("SelectedOneOffButtonChanged", true, false);
-            this.dispatchEvent(event);
+          if (button.classList.contains("searchbar-engine-one-off-item") &&
+              button.engine) {
+            let headerEngineText =
+              document.getAnonymousElementByAttribute(this, "anonid",
+                                                      "searchbar-oneoffheader-engine");
+            header.selectedIndex = 2;
+            headerEngineText.value = button.engine.name;
+          } else {
+            header.selectedIndex = this.query ? 1 : 0;
+          }
+          if (this.textbox) {
+            this.textbox.setAttribute("aria-activedescendant", button.id);
           }
         ]]></body>
       </method>
 
       <method name="getSelectableButtons">
         <parameter name="aIncludeNonEngineButtons"/>
         <body><![CDATA[
           let buttons = [];
@@ -1854,86 +1871,66 @@
       </method>
 
       <!--
         Increments or decrements the index of the currently selected one-off.
 
         @param aForward
                If true, the index is incremented, and if false, the index is
                decremented.
+        @param aIncludeNonEngineButtons
+               If true, non-dummy buttons that do not have engines are included.
+               These buttons include the OpenSearch and settings buttons.  For
+               example, if the currently selected button is an engine button,
+               the next button is the settings button, and you pass true for
+               aForward, then passing true for this value would cause the
+               settings to be selected.  Passing false for this value would
+               cause the selection to clear or wrap around, depending on what
+               value you passed for the aWrapAround parameter.
         @param aWrapAround
-               This has a couple of effects, depending on whether there is
-               currently a selection.
-               (1) If true and the last one-off is currently selected,
-               incrementing the index will cause the selection to be cleared and
-               this method to return true.  Calling advanceSelection again after
-               that (again with aForward=true) will select the first one-off.
-               Likewise if decrementing the index when the first one-off is
-               selected, except in the opposite direction of course.
-               (2) If true and there currently is no selection, decrementing the
-               index will cause the last one-off to become selected and this
-               method to return true.  Only the aForward=false case is affected
-               because it is always the case that if aForward=true and there
-               currently is no selection, the first one-off becomes selected and
-               this method returns true.
-        @param aCycleEngines
-               If true, only engine buttons are included.
+               If true, the selection wraps around between the first and last
+               buttons.
         @return True if the selection can continue to advance after this method
                 returns and false if not.
       -->
       <method name="advanceSelection">
         <parameter name="aForward"/>
+        <parameter name="aIncludeNonEngineButtons"/>
         <parameter name="aWrapAround"/>
-        <parameter name="aCycleEngines"/>
         <body><![CDATA[
-          let selectedButton = this.selectedButton;
-          let buttons = this.getSelectableButtons(aCycleEngines);
-
-          if (selectedButton) {
-            // cycle through one-off buttons.
-            let index = buttons.indexOf(selectedButton);
-            if (aForward)
-              ++index;
-            else
-              --index;
-
-            if (index >= 0 && index < buttons.length)
-              this.selectedButton = buttons[index];
-            else
-              this.selectedButton = null;
-
-            if (this.selectedButton || aWrapAround)
-              return true;
-
-            return false;
+          let buttons = this.getSelectableButtons(aIncludeNonEngineButtons);
+          let index;
+          if (this.selectedButton) {
+            let inc = aForward ? 1 : -1;
+            let oldIndex = buttons.indexOf(this.selectedButton);
+            index = ((oldIndex + inc) + buttons.length) % buttons.length;
+            if (!aWrapAround &&
+                ((aForward && index <= oldIndex) ||
+                 (!aForward && oldIndex <= index))) {
+              // The index has wrapped around, but wrapping around isn't
+              // allowed.
+              index = -1;
+            }
+          } else {
+            index = aForward ? 0 : buttons.length - 1;
           }
-
-          // If no selection, select the first button or ...
-          if (aForward) {
-            this.selectedButton = buttons[0];
-            return true;
-          }
-
-          if (!aForward && aWrapAround) {
-            // the last button.
-            this.selectedButton = buttons[buttons.length - 1];
-            return true;
-          }
-
-          return false;
+          this.selectedButton = index < 0 ? null : buttons[index];
         ]]></body>
       </method>
 
       <!--
         This handles key presses specific to the one-off buttons like Tab and
-        Alt-Up/Down, and Up/Down keys within the buttons.  Since one-off buttons
+        Alt+Up/Down, and Up/Down keys within the buttons.  Since one-off buttons
         are always used in conjunction with a list of some sort (in this.popup),
         it also handles Up/Down keys that cross the boundaries between list
         items and the one-off buttons.
 
+        If this method handles the key press, then event.defaultPrevented will
+        be true when it returns.
+
         @param event
                The key event.
         @param numListItems
                The number of items in the list.  The reason that this is a
                parameter at all is that the list may contain items at the end
                that should be ignored, depending on the consumer.  That's true
                for the urlbar for example.
         @param allowEmptySelection
@@ -1941,134 +1938,206 @@
                buttons contains a selection.  Pass false if either the list or
                the one-off buttons (or both) should always contain a selection.
         @param textboxUserValue
                When the last list item is selected and the user presses Down,
                the first one-off becomes selected and the textbox value is
                restored to the value that the user typed.  Pass that value here.
                However, if you pass true for allowEmptySelection, you don't need
                to pass anything for this parameter.  (Pass undefined or null.)
-        @return True if this method handled the keypress and false if not.  If
-                false, then you should let the autocomplete controller handle
-                the keypress.  The value of event.defaultPrevented will be the
-                same as this return value.
       -->
       <method name="handleKeyPress">
         <parameter name="event"/>
         <parameter name="numListItems"/>
         <parameter name="allowEmptySelection"/>
         <parameter name="textboxUserValue"/>
         <body><![CDATA[
           if (!this.popup) {
-            return false;
+            return;
           }
-
-          let stopEvent = false;
+          let handled = this._handleKeyPress(event, numListItems,
+                                             allowEmptySelection,
+                                             textboxUserValue);
+          if (handled) {
+            event.preventDefault();
+            event.stopPropagation();
+          }
+        ]]></body>
+      </method>
 
-          // Tab cycles through the one-offs and moves the focus out at the end.
-          // But only if non-Shift modifiers aren't also pressed, to avoid
-          // clobbering other shortcuts.
-          if (event.keyCode == KeyEvent.DOM_VK_TAB &&
-              !event.altKey &&
-              !event.ctrlKey &&
-              !event.metaKey &&
-              this.getAttribute("disabletab") != "true") {
-            stopEvent = this.advanceSelection(!event.shiftKey, false, true);
-          } else if (event.altKey &&
-                     (event.keyCode == KeyEvent.DOM_VK_DOWN ||
-                      event.keyCode == KeyEvent.DOM_VK_UP)) {
-            // Alt + up/down is very similar to (shift +) tab but differs in that
-            // it loops through the list, whereas tab will move the focus out.
-            stopEvent =
-              this.advanceSelection(event.keyCode == KeyEvent.DOM_VK_DOWN,
-                                    true, false);
-          } else if (event.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_UP) {
-            if (numListItems > 0) {
-              if (this.popup.selectedIndex > 0) {
-                // The autocomplete controller should handle this case.
-              } else if (this.popup.selectedIndex == 0) {
-                if (!allowEmptySelection) {
-                  // Wrap around the selection to the last one-off.
-                  this.selectedButton = null;
-                  this.popup.selectedIndex = -1;
-                  // Call advanceSelection after setting selectedIndex so that
-                  // screen readers see the newly selected one-off. Both trigger
-                  // accessibility events.
-                  this.advanceSelection(false, true, true);
-                  stopEvent = true;
-                }
-              } else {
-                let firstButtonSelected =
-                  this.selectedButton &&
-                  this.selectedButton == this.getSelectableButtons(true)[0];
-                if (firstButtonSelected) {
-                  this.selectedButton = null;
-                } else {
-                  stopEvent = this.advanceSelection(false, true, true);
-                }
-              }
-            } else {
-              stopEvent = this.advanceSelection(false, true, true);
-            }
-          } else if (event.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_DOWN) {
-            if (numListItems > 0) {
-              if (this.popup.selectedIndex >= 0 &&
-                  this.popup.selectedIndex < numListItems - 1) {
-                // The autocomplete controller should handle this case.
-              } else if (this.popup.selectedIndex == numListItems - 1) {
-                this.selectedButton = null;
-                if (!allowEmptySelection) {
-                  this.popup.selectedIndex = -1;
-                  stopEvent = true;
-                }
-                if (this.textbox && typeof(textboxUserValue) == "string") {
-                  this.textbox.value = textboxUserValue;
-                }
-                // Call advanceSelection after setting selectedIndex so that
-                // screen readers see the newly selected one-off. Both trigger
-                // accessibility events.
-                this.advanceSelection(true, true, true);
-              } else {
-                let buttons = this.getSelectableButtons(true);
-                let lastButtonSelected =
-                  this.selectedButton &&
-                  this.selectedButton == buttons[buttons.length - 1];
-                if (lastButtonSelected) {
-                  this.selectedButton = null;
-                  stopEvent = allowEmptySelection;
-                } else if (this.selectedButton) {
-                  stopEvent = this.advanceSelection(true, true, true);
-                } else {
-                  // The autocomplete controller should handle this case.
-                }
-              }
-            } else {
-              stopEvent = this.advanceSelection(true, true, true);
-            }
-          } else if (this.selectedButton &&
-                     this.selectedButton.getAttribute("anonid") ==
-                       "addengine-menu-button" &&
-                     event.keyCode == KeyEvent.DOM_VK_RIGHT) {
+      <method name="_handleKeyPress">
+        <parameter name="event"/>
+        <parameter name="numListItems"/>
+        <parameter name="allowEmptySelection"/>
+        <parameter name="textboxUserValue"/>
+        <body><![CDATA[
+          if (event.keyCode == KeyEvent.DOM_VK_RIGHT &&
+              this.selectedButton &&
+              this.selectedButton.getAttribute("anonid") ==
+                "addengine-menu-button") {
             // If the add-engine overflow menu item is selected and the user
             // presses the right arrow key, open the submenu.  Unfortunately
             // handling the left arrow key -- to close the popup -- isn't
             // straightforward.  Once the popup is open, it consumes all key
             // events.  Setting ignorekeys=handled on it doesn't help, since the
             // popup handles all arrow keys.  Setting ignorekeys=true on it does
-            // mean that the popup no longer consumes the left arrow key, but then
-            // it no longer handles up/down keys to select items in the popup.
+            // mean that the popup no longer consumes the left arrow key, but
+            // then it no longer handles up/down keys to select items in the
+            // popup.
             this.selectedButton.open = true;
-            stopEvent = true;
+            return true;
+          }
+
+          // Handle the Tab key, but only if non-Shift modifiers aren't also
+          // pressed to avoid clobbering other shortcuts (like the Alt+Tab
+          // browser tab switcher).  The reason this uses getModifierState() and
+          // checks for "AltGraph" is that when you press Shift-Alt-Tab,
+          // event.altKey is actually false for some reason, at least on macOS.
+          // getModifierState("Alt") is also false, but "AltGraph" is true.
+          if (event.keyCode == KeyEvent.DOM_VK_TAB &&
+              !event.getModifierState("Alt") &&
+              !event.getModifierState("AltGraph") &&
+              !event.getModifierState("Control") &&
+              !event.getModifierState("Meta")) {
+            if (this.getAttribute("disabletab") == "true" ||
+                (event.shiftKey &&
+                  this.selectedButtonIndex <= 0) ||
+                (!event.shiftKey &&
+                 this.selectedButtonIndex ==
+                   this.getSelectableButtons(true).length - 1)) {
+              this.selectedButton = null;
+              return false;
+            }
+            this.popup.selectedIndex = -1;
+            this.advanceSelection(!event.shiftKey, true, false);
+            return !!this.selectedButton;
           }
 
-          if (stopEvent) {
-            event.preventDefault();
-            event.stopPropagation();
+          if (event.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_UP) {
+            if (event.altKey) {
+              // Keep the currently selected result in the list (if any) as a
+              // secondary "alt" selection and move the selection up within the
+              // buttons.
+              this.advanceSelection(false, false, false);
+              return true;
+            }
+            if (numListItems == 0) {
+              this.advanceSelection(false, true, false);
+              return true;
+            }
+            if (this.popup.selectedIndex > 0) {
+              // Moving up within the list.  The autocomplete controller should
+              // handle this case.  A button may be selected, so null it.
+              this.selectedButton = null;
+              return false;
+            }
+            if (this.popup.selectedIndex == 0) {
+              // Moving up from the top of the list.
+              if (allowEmptySelection) {
+                // Let the autocomplete controller remove selection in the list
+                // and revert the typed text in the textbox.
+                return false;
+              }
+              // Wrap selection around to the last button.
+              if (this.textbox && typeof(textboxUserValue) == "string") {
+                this.textbox.value = textboxUserValue;
+              }
+              this.advanceSelection(false, true, true);
+              return true;
+            }
+            if (!this.selectedButton) {
+              // Moving up from no selection in the list or the buttons, back
+              // down to the last button.
+              this.advanceSelection(false, true, true);
+              return true;
+            }
+            if (this.selectedButtonIndex == 0) {
+              // Moving up from the buttons to the bottom of the list.
+              this.selectedButton = null;
+              return false;
+            }
+            // Moving up/left within the buttons.
+            this.advanceSelection(false, true, false);
             return true;
           }
+
+          if (event.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_DOWN) {
+            if (event.altKey) {
+              // Keep the currently selected result in the list (if any) as a
+              // secondary "alt" selection and move the selection down within
+              // the buttons.
+              this.advanceSelection(true, false, false);
+              return true;
+            }
+            if (numListItems == 0) {
+              this.advanceSelection(true, true, false);
+              return true;
+            }
+            if (this.popup.selectedIndex >= 0 &&
+                this.popup.selectedIndex < numListItems - 1) {
+              // Moving down within the list.  The autocomplete controller
+              // should handle this case.  A button may be selected, so null it.
+              this.selectedButton = null;
+              return false;
+            }
+            if (this.popup.selectedIndex == numListItems - 1) {
+              // Moving down from the last item in the list to the buttons.
+              this.selectedButtonIndex = 0;
+              if (allowEmptySelection) {
+                // Let the autocomplete controller remove selection in the list
+                // and revert the typed text in the textbox.
+                return false;
+              }
+              if (this.textbox && typeof(textboxUserValue) == "string") {
+                this.textbox.value = textboxUserValue;
+              }
+              this.popup.selectedIndex = -1;
+              return true;
+            }
+            if (this.selectedButton) {
+              let buttons = this.getSelectableButtons(true);
+              if (this.selectedButtonIndex == buttons.length - 1) {
+                // Moving down from the buttons back up to the top of the list.
+                this.selectedButton = null;
+                if (allowEmptySelection) {
+                  // Prevent the selection from wrapping around to the top of
+                  // the list by returning true, since the list currently has no
+                  // selection.  Nothing should be selected after handling this
+                  // Down key.
+                  return true;
+                }
+                return false;
+              }
+              // Moving down/right within the buttons.
+              this.advanceSelection(true, true, false);
+              return true;
+            }
+            return false;
+          }
+
+          if (event.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_LEFT) {
+            if (this.selectedButton &&
+                (this.compact || this.selectedButton.engine)) {
+              // Moving left within the buttons.
+              this.advanceSelection(false, this.compact, true);
+              return true;
+            }
+            return false;
+          }
+
+          if (event.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_RIGHT) {
+            if (this.selectedButton &&
+                (this.compact || this.selectedButton.engine)) {
+              // Moving right within the buttons.
+              this.advanceSelection(true, this.compact, true);
+              return true;
+            }
+            return false;
+          }
+
           return false;
         ]]></body>
       </method>
 
       <!--
         If the given event is related to the one-offs, this method records
         one-off telemetry for it.  this.telemetryOrigin will be appended to the
         computed source, so make sure you set that first.
@@ -2169,24 +2238,20 @@
 
       <handler event="mousemove"><![CDATA[
         let target = event.originalTarget;
 
         // Handle mouseover on the add-engine menu button and its popup items.
         if (target.getAttribute("anonid") == "addengine-menu-button" ||
             (target.localName == "menuitem" &&
              target.classList.contains("addengine-item"))) {
-          // Make the menu button visually selected.  It's highlighted in the
-          // CSS when the popup is open, but the popup doesn't open until a
-          // short timeout has elapsed.  Making the button visually selected now
-          // provides better feedback to the user.
           let menuButton = document.getAnonymousElementByAttribute(
             this, "anonid", "addengine-menu-button"
           );
-          this._changeVisuallySelectedButton(menuButton);
+          this._updateStateForButton(menuButton);
           this._addEngineMenuShouldBeOpen = true;
           this._resetAddEngineMenuTimeout();
           return;
         }
 
         if (target.localName != "button")
           return;
 
@@ -2195,53 +2260,43 @@
            return;
 
         let isOneOff =
           target.classList.contains("searchbar-engine-one-off-item") &&
           !target.classList.contains("dummy");
         if (isOneOff ||
             target.classList.contains("addengine-item") ||
             target.classList.contains("search-setting-button")) {
-          this._changeVisuallySelectedButton(target);
+          this._updateStateForButton(target);
         }
       ]]></handler>
 
       <handler event="mouseout"><![CDATA[
 
         let target = event.originalTarget;
 
         // Handle mouseout on the add-engine menu button and its popup items.
         if (target.getAttribute("anonid") == "addengine-menu-button" ||
             (target.localName == "menuitem" &&
              target.classList.contains("addengine-item"))) {
-          // The menu button will appear selected since the mouse is either over
-          // it or over one of the menu items in the popup.  Make it unselected.
-          this._changeVisuallySelectedButton(null);
+          this._updateStateForButton(null);
           this._addEngineMenuShouldBeOpen = false;
           this._resetAddEngineMenuTimeout();
           return;
         }
 
         if (target.localName != "button") {
           return;
         }
 
-        // Don't deselect the current button if the context menu is open.
+        // Don't update the mouseover state if the context menu is open.
         if (this._ignoreMouseEvents)
           return;
 
-        // Unfortunately this will fire before mouseover hits another item.
-        // If this button is selected, we replace that selection only if
-        // we're not moving to a different one-off item:
-        if (target.getAttribute("selected") == "true" &&
-            (!event.relatedTarget ||
-             !event.relatedTarget.classList.contains("searchbar-engine-one-off-item") ||
-             event.relatedTarget.classList.contains("dummy"))) {
-          this._changeVisuallySelectedButton(this.selectedButton);
-        }
+        this._updateStateForButton(null);
       ]]></handler>
 
       <handler event="click"><![CDATA[
         if (event.button == 2)
           return; // ignore right clicks.
 
         let button = event.originalTarget;
         let engine = button.engine;
--- a/browser/components/search/content/searchbarBindings.css
+++ b/browser/components/search/content/searchbarBindings.css
@@ -3,16 +3,20 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
 
 .searchbar-textbox {
   -moz-binding: url("chrome://browser/content/search/search.xml#searchbar-textbox");
 }
 
+.searchbar-treebody {
+  -moz-binding: url("chrome://browser/content/search/search.xml#searchbar-treebody");
+}
+
 .search-one-offs {
   -moz-binding: url("chrome://browser/content/search/search.xml#search-one-offs");
 }
 
 .search-setting-button[compact=true],
 .search-setting-button-compact:not([compact=true]) {
   display: none;
 }
--- a/browser/components/search/test/browser_searchbar_keyboard_navigation.js
+++ b/browser/components/search/test/browser_searchbar_keyboard_navigation.js
@@ -323,52 +323,63 @@ add_task(function* test_tab_and_arrows()
 
   // After pressing down, the first sugggestion should be selected.
   EventUtils.synthesizeKey("VK_DOWN", {});
   is(searchPopup.selectedIndex, 0, "first suggestion should be selected");
   is(textbox.value, kValues[0], "the textfield value should have changed");
   ok(!textbox.selectedButton, "no one-off button should be selected");
 
   // After pressing tab, the first one-off should be selected,
-  // and the first suggestion still selected.
+  // and no suggestion should be selected.
   let oneOffs = getOneOffs();
   EventUtils.synthesizeKey("VK_TAB", {});
   is(textbox.selectedButton, oneOffs[0],
      "the first one-off button should be selected");
-  is(searchPopup.selectedIndex, 0, "first suggestion should still be selected");
+  is(searchPopup.selectedIndex, -1, "no suggestion should be selected");
 
-  // After pressing down, the second suggestion should be selected,
-  // and the first one-off still selected.
+  // After pressing down, the second one-off should be selected.
   EventUtils.synthesizeKey("VK_DOWN", {});
-  is(textbox.selectedButton, oneOffs[0],
-     "the first one-off button should still be selected");
-  is(searchPopup.selectedIndex, 1, "second suggestion should be selected");
+  is(textbox.selectedButton, oneOffs[1],
+     "the second one-off button should be selected");
+  is(searchPopup.selectedIndex, -1, "no suggestion should be selected");
 
-  // After pressing up, the first suggestion should be selected again,
-  // and the first one-off still selected.
+  // After pressing right, the third one-off should be selected.
+  EventUtils.synthesizeKey("VK_RIGHT", {});
+  is(textbox.selectedButton, oneOffs[2],
+     "the third one-off button should be selected");
+  is(searchPopup.selectedIndex, -1, "no suggestion should be selected");
+
+  // After pressing left, the second one-off should be selected again.
+  EventUtils.synthesizeKey("VK_LEFT", {});
+  is(textbox.selectedButton, oneOffs[1],
+     "the second one-off button should be selected again");
+  is(searchPopup.selectedIndex, -1, "no suggestion should be selected");
+
+  // After pressing up, the first one-off should be selected again.
   EventUtils.synthesizeKey("VK_UP", {});
   is(textbox.selectedButton, oneOffs[0],
-     "the first one-off button should still be selected");
-  is(searchPopup.selectedIndex, 0, "second suggestion should be selected again");
+     "the first one-off button should be selected again");
+  is(searchPopup.selectedIndex, -1, "no suggestion should be selected");
 
-  // After pressing up again, we should have no suggestion selected anymore,
+  // After pressing up again, the last suggestion should be selected.
   // the textfield value back to the user-typed value, and still the first one-off
   // selected.
   EventUtils.synthesizeKey("VK_UP", {});
-  is(searchPopup.selectedIndex, -1, "no suggestion should be selected");
-  is(textbox.value, kUserValue,
-     "the textfield value should be back to user typed value");
-  is(textbox.selectedButton, oneOffs[0],
-     "the first one-off button should still be selected");
+  is(searchPopup.selectedIndex, kValues.length - 1,
+     "last suggestion should be selected");
+  is(textbox.value, kValues[kValues.length - 1],
+     "the textfield value should match the suggestion");
+  is(textbox.selectedButton, null,
+     "no one-off button should be selected");
 
-  // Now pressing down should select the second one-off.
+  // Now pressing down should select the first one-off.
   EventUtils.synthesizeKey("VK_DOWN", {});
-  is(textbox.selectedButton, oneOffs[1],
-     "the second one-off button should be selected");
-  is(searchPopup.selectedIndex, -1, "there should still be no selected suggestion");
+  is(textbox.selectedButton, oneOffs[0],
+     "the first one-off button should be selected");
+  is(searchPopup.selectedIndex, -1, "there should be no selected suggestion");
 
   // Finally close the panel.
   let promise = promiseEvent(searchPopup, "popuphidden");
   searchPopup.hidePopup();
   yield promise;
 });
 
 add_task(function* test_open_search() {
--- a/browser/themes/linux/browser.css
+++ b/browser/themes/linux/browser.css
@@ -916,16 +916,27 @@ notification[value="translation"] menuli
 .autocomplete-richlistitem {
   height: 30px;
   min-height: 30px;
   font: message-box;
   border-radius: 2px;
   border: 1px solid transparent;
 }
 
+.autocomplete-richlistitem:hover,
+treechildren.searchbar-treebody::-moz-tree-row(hover) {
+  background-color: hsla(0, 0%, 0%, 0.06);
+  border-color: hsla(0, 0%, 0%, 0.1);
+}
+
+.autocomplete-richlistitem[selected],
+treechildren.searchbar-treebody::-moz-tree-row(selected) {
+  background-color: Highlight;
+}
+
 .ac-title {
   font-size: 1.05em;
 }
 
 .ac-separator,
 .ac-url,
 .ac-action,
 .ac-tags {
--- a/browser/themes/linux/searchbar.css
+++ b/browser/themes/linux/searchbar.css
@@ -216,16 +216,23 @@ menuitem[cmd="cmd_clearhistory"][disable
 .search-panel-one-offs:not([compact=true]) > .searchbar-engine-one-off-item.last-of-row,
 .search-panel-one-offs[compact=true] > .searchbar-engine-one-off-item.last-of-row:not(.dummy),
 .search-panel-one-offs[compact=true] > .searchbar-engine-one-off-item.dummy:not(.last-of-row),
 .search-panel-one-offs[compact=true] > .searchbar-engine-one-off-item.last-engine,
 .search-setting-button-compact {
   background-image: none;
 }
 
+.searchbar-engine-one-off-item:not([selected]):not(.dummy):hover,
+.search-setting-button:hover,
+.addengine-item:hover {
+  background-color: hsla(0, 0%, 0%, 0.06);
+  color: inherit;
+}
+
 .searchbar-engine-one-off-item[selected] {
   background-color: Highlight;
   background-image: none;
   color: HighlightText;
 }
 
 .searchbar-engine-one-off-item > .button-box {
   padding: 0;
--- a/browser/themes/osx/browser.css
+++ b/browser/themes/osx/browser.css
@@ -1693,21 +1693,31 @@ toolbar .toolbarbutton-1 > .toolbarbutto
 .autocomplete-richlistitem {
   height: 30px;
   min-height: 30px;
   font: message-box;
   border-radius: 2px;
   border: 1px solid transparent;
 }
 
+.autocomplete-richlistitem:hover,
+treechildren.searchbar-treebody::-moz-tree-row(hover) {
+  background-color: hsla(0, 0%, 0%, 0.06);
+  border-color: hsla(0, 0%, 0%, 0.1);
+}
+
 .autocomplete-richlistitem[selected] {
   background-color: hsl(210, 80%, 52%);
   color: hsl(0, 0%, 100%);
 }
 
+treechildren.searchbar-treebody::-moz-tree-row(selected) {
+  background-color: Highlight;
+}
+
 .ac-title {
   font-size: 14px;
 }
 
 .ac-separator,
 .ac-url,
 .ac-action,
 .ac-tags {
--- a/browser/themes/osx/searchbar.css
+++ b/browser/themes/osx/searchbar.css
@@ -205,16 +205,23 @@
 .search-panel-one-offs:not([compact=true]) > .searchbar-engine-one-off-item.last-of-row,
 .search-panel-one-offs[compact=true] > .searchbar-engine-one-off-item.last-of-row:not(.dummy),
 .search-panel-one-offs[compact=true] > .searchbar-engine-one-off-item.dummy:not(.last-of-row),
 .search-panel-one-offs[compact=true] > .searchbar-engine-one-off-item.last-engine,
 .search-setting-button-compact {
   background-image: none;
 }
 
+.searchbar-engine-one-off-item:not([selected]):not(.dummy):hover,
+.search-setting-button:hover,
+.addengine-item:hover {
+  background-color: hsla(0, 0%, 0%, 0.06);
+  color: inherit;
+}
+
 .searchbar-engine-one-off-item[selected] {
   background-color: Highlight;
   background-image: none;
   color: HighlightText;
 }
 
 .searchbar-engine-one-off-item > .button-box > .button-text {
   display: none;
--- a/browser/themes/windows/browser.css
+++ b/browser/themes/windows/browser.css
@@ -1433,21 +1433,31 @@ html|span.ac-tag {
 
 html|span.ac-emphasize-text-title,
 html|span.ac-emphasize-text-tag,
 html|span.ac-emphasize-text-url {
   font-weight: 600;
 }
 
 @media (-moz-windows-default-theme) {
-  .autocomplete-richlistitem[selected=true] {
+  .autocomplete-richlistitem:hover,
+  treechildren.searchbar-treebody::-moz-tree-row(hover) {
+    background-color: hsla(0, 0%, 0%, 0.06);
+    border-color: hsla(0, 0%, 0%, 0.1);
+  }
+
+  .autocomplete-richlistitem[selected] {
     background-color: hsl(210, 80%, 52%);
     color: hsl(0, 0%, 100%);
   }
 
+  treechildren.searchbar-treebody::-moz-tree-row(selected) {
+    background-color: Highlight;
+  }
+
   .ac-title:not([selected=true]) {
     color: hsl(0, 0%, 0%);
   }
 
   .ac-separator:not([selected=true]) {
     color: hsl(0, 0%, 50%);
   }
 
--- a/browser/themes/windows/searchbar.css
+++ b/browser/themes/windows/searchbar.css
@@ -217,16 +217,23 @@
 .search-panel-one-offs:not([compact=true]) > .searchbar-engine-one-off-item.last-of-row,
 .search-panel-one-offs[compact=true] > .searchbar-engine-one-off-item.last-of-row:not(.dummy),
 .search-panel-one-offs[compact=true] > .searchbar-engine-one-off-item.dummy:not(.last-of-row),
 .search-panel-one-offs[compact=true] > .searchbar-engine-one-off-item.last-engine,
 .search-setting-button-compact {
   background-image: none;
 }
 
+.searchbar-engine-one-off-item:not([selected]):not(.dummy):hover,
+.search-setting-button:hover,
+.addengine-item:hover {
+  background-color: hsla(0, 0%, 0%, 0.06);
+  color: inherit;
+}
+
 .searchbar-engine-one-off-item[selected] {
   background-color: Highlight;
   background-image: none;
   color: HighlightText;
 }
 
 .searchbar-engine-one-off-item > .button-box {
   padding: 0;
--- a/toolkit/content/widgets/autocomplete.xml
+++ b/toolkit/content/widgets/autocomplete.xml
@@ -1099,17 +1099,17 @@ extends="chrome://global/content/binding
           }
           return val;
         ]]>
         </setter>
       </property>
 
       <method name="onSearchBegin">
         <body><![CDATA[
-          this.richlistbox.mouseSelectedIndex = -1;
+          this.richlistbox.mousedOverIndex = -1;
 
           if (typeof this._onSearchBegin == "function") {
             this._onSearchBegin();
           }
         ]]></body>
       </method>
 
       <method name="openAutocompletePopup">
@@ -1346,24 +1346,24 @@ extends="chrome://global/content/binding
             item.setAttribute("ac-image", image);
             item.setAttribute("ac-value", value);
             item.setAttribute("ac-label", label);
             item.setAttribute("ac-comment", comment);
             item.setAttribute("ac-text", trimmedSearchString);
 
             // Completely reuse the existing richlistitem for invalidation
             // due to new results, but only when: the item is the same, *OR*
-            // we are about to replace the currently mouse-selected item, to
+            // we are about to replace the currently moused-over item, to
             // avoid surprising the user.
             let iface = Components.interfaces.nsIAutoCompletePopup;
             if (reusable &&
                 originalText == trimmedSearchString &&
                 invalidateReason == iface.INVALIDATE_REASON_NEW_RESULT &&
                 (originalValue == value ||
-                 this.richlistbox.mouseSelectedIndex === this._currentIndex)) {
+                 this.richlistbox.mousedOverIndex === this._currentIndex)) {
 
               // try to re-use the existing item
               let reused = item._reuseAcItem();
               if (reused) {
                 this._currentIndex++;
                 continue;
               }
             } else {
@@ -2502,17 +2502,17 @@ extends="chrome://global/content/binding
         <children/>
       </xul:treerows>
     </content>
   </binding>
 
   <binding id="autocomplete-richlistbox" extends="chrome://global/content/bindings/richlistbox.xml#richlistbox">
     <implementation>
       <field name="mLastMoveTime">Date.now()</field>
-      <field name="mouseSelectedIndex">-1</field>
+      <field name="mousedOverIndex">-1</field>
     </implementation>
     <handlers>
       <handler event="mouseup">
         <![CDATA[
         // don't call onPopupClick for the scrollbar buttons, thumb, slider, etc.
         let item = event.originalTarget;
         while (item && item.localName != "richlistitem") {
           item = item.parentNode;
@@ -2522,32 +2522,31 @@ extends="chrome://global/content/binding
           return;
 
         this.parentNode.onPopupClick(event);
       ]]>
       </handler>
 
       <handler event="mousemove">
         <![CDATA[
-        if (Date.now() - this.mLastMoveTime > 30) {
-         let item = event.target;
-         while (item && item.localName != "richlistitem") {
-           item = item.parentNode;
-         }
+        if (Date.now() - this.mLastMoveTime <= 30) {
+          return;
+        }
 
-         if (!item)
-           return;
+        let item = event.target;
+        while (item && item.localName != "richlistitem") {
+          item = item.parentNode;
+        }
 
-         let index = this.getIndexOfItem(item);
-         if (index != this.selectedIndex) {
-            this.mouseSelectedIndex = this.selectedIndex = index;
-         }
+        if (!item) {
+          return;
+        }
 
-         this.mLastMoveTime = Date.now();
-        }
+        this.mousedOverIndex = this.getIndexOfItem(item);
+        this.mLastMoveTime = Date.now();
       ]]>
       </handler>
     </handlers>
   </binding>
 
   <binding id="autocomplete-treebody">
     <implementation>
       <field name="mLastMoveTime">Date.now()</field>
@@ -2558,16 +2557,21 @@ extends="chrome://global/content/binding
 
       <handler event="mousedown"><![CDATA[
          var rc = this.parentNode.treeBoxObject.getRowAt(event.clientX, event.clientY);
          if (rc != this.parentNode.currentIndex)
             this.parentNode.view.selection.select(rc);
       ]]></handler>
 
       <handler event="mousemove"><![CDATA[
+        if (event.defaultPrevented) {
+          // Allow bindings that extend this one to cancel the event so that
+          // nothing is selected.
+          return;
+        }
         if (Date.now() - this.mLastMoveTime > 30) {
          var rc = this.parentNode.treeBoxObject.getRowAt(event.clientX, event.clientY);
          if (rc != this.parentNode.currentIndex)
             this.parentNode.view.selection.select(rc);
          this.mLastMoveTime = Date.now();
         }
       ]]></handler>
     </handlers>