Bug 1491724 - Search engine keyword should be highlighted only when the search will actually be performed on the corresponding search engine r=mak
authorDrew Willcoxon <adw@mozilla.com>
Fri, 28 Sep 2018 23:29:26 +0000
changeset 438862 7e5bf501404a
parent 438733 669ffcf8bc71
child 438863 e2d81abcb655
push id34738
push userdluca@mozilla.com
push dateSat, 29 Sep 2018 10:01:07 +0000
treeherdermozilla-central@994badb573ef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1491724, 1484737
milestone64.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 1491724 - Search engine keyword should be highlighted only when the search will actually be performed on the corresponding search engine r=mak My reasoning in https://bugzilla.mozilla.org/show_bug.cgi?id=1484737#c3 was wrong. I assumed that if a result started with a search alias then it had to be a search alias result, but that's clearly not true. This is such a pain to get right, and all the messy logic in this patch reflects that. Differential Revision: https://phabricator.services.mozilla.com/D6528
browser/base/content/urlbarBindings.xml
browser/components/search/content/search.xml
toolkit/content/widgets/autocomplete.xml
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -790,40 +790,51 @@ file, You can obtain one at http://mozil
         @return True if formatting was applied and false if not.
       -->
       <method name="_formatSearchAlias">
         <body><![CDATA[
           if (!this._formattingEnabled) {
             return false;
           }
 
-          let textNode = this.editor.rootElement.firstChild;
-          let value = textNode.textContent;
-          let trimmedValue = value.trimStart();
-
-          // If there's no alias in the results or the first word in the input
-          // value is not that alias, then there's no formatting to do.
-          if (!this.popup.searchAlias ||
-              (trimmedValue.length != this.popup.searchAlias.length &&
-               trimmedValue[this.popup.searchAlias.length] != " ") ||
-              !trimmedValue.startsWith(this.popup.searchAlias)) {
+          // There can only be an alias to highlight if the heuristic result is
+          // an alias searchengine result and it's either currently selected or
+          // was selected when the popup was closed.  We also need to check
+          // whether a one-off search button is selected because in that case
+          // there won't be a selection but the alias should not be highlighted.
+          let heuristicItem = this.popup.richlistbox.children[0] || null;
+          let alias =
+            (this.popup.selectedIndex == 0 ||
+             (this.popup.selectedIndex < 0 &&
+              this.popup._previousSelectedIndex == 0)) &&
+            !this.popup.oneOffSearchButtons.selectedButton &&
+            heuristicItem &&
+            heuristicItem.getAttribute("actiontype") == "searchengine" &&
+            this._parseActionUrl(heuristicItem.getAttribute("url")).params.alias;
+          if (!alias) {
             return false;
           }
 
-          let index = value.indexOf(this.popup.searchAlias);
+          let textNode = this.editor.rootElement.firstChild;
+          let value = textNode.textContent;
+
+          let index = value.indexOf(alias);
+          if (index < 0) {
+            return false;
+          }
 
           // We abuse the SELECTION_FIND selection type to do our highlighting.
           // It's the only type that works with Selection.setColors().
           let selection = this.editor.selectionController.getSelection(
             Ci.nsISelectionController.SELECTION_FIND
           );
 
           let range = document.createRange();
           range.setStart(textNode, index);
-          range.setEnd(textNode, index + this.popup.searchAlias.length);
+          range.setEnd(textNode, index + alias.length);
           selection.addRange(range);
 
           let fg = "#2362d7";
           let bg = "#d2e6fd";
 
           // Selection.setColors() will swap the given foreground and background
           // colors if it detects that the contrast between the background
           // color and the frame color is too low.  Normally we don't want that
@@ -2629,35 +2640,16 @@ file, You can obtain one at http://mozil
             this._invalidate();
           } else {
             this.closePopup();
           }
           ]]>
         </body>
       </method>
 
-      <method name="_selectedOneOffChanged">
-        <body><![CDATA[
-          // Update all searchengine result items to use the newly selected
-          // engine.
-          for (let item of this.richlistbox.children) {
-            if (item.collapsed) {
-              break;
-            }
-            let url = item.getAttribute("url");
-            if (url) {
-              let action = item._parseActionUrl(url);
-              if (action && action.type == "searchengine") {
-                item._adjustAcItem();
-              }
-            }
-          }
-        ]]></body>
-      </method>
-
       <!-- This handles keypress changes to the selection among the one-off
            search buttons and between the one-offs and the listbox.  It returns
            true if the keypress was consumed and false if not. -->
       <method name="handleKeyPress">
         <parameter name="aEvent"/>
         <body><![CDATA[
           this.oneOffSearchButtons.handleKeyPress(aEvent, this.matchCount,
                                                   !this._isFirstResultHeuristic,
@@ -2734,48 +2726,38 @@ file, You can obtain one at http://mozil
             Services.io.speculativeConnect2(uri, gBrowser.contentPrincipal, null);
           } catch (ex) {
             // Can't setup speculative connection for this uri string for some
             // reason, just ignore it.
           }
         ]]></body>
       </method>
 
-      <!--
-        The search alias of the first (heuristic) result in the popup, if any.
-      -->
-      <field name="searchAlias">null</field>
-
       <method name="onResultsAdded">
         <body>
           <![CDATA[
-            if (!this.input.gotResultForCurrentQuery) {
-              // This is the first result of a new search.  Cache its search
-              // alias, if any.  Do this now, when we get the first result, so
-              // that the cached alias remains available between the time the
-              // previous search ended and now.
-              //
-              // Calling _parseActionUrl and getting params.alias would be
-              // sufficient here, but as an optimization, first check whether
-              // the result is a searchengine result, which is a simple
-              // substring check, to avoid the more expensive _parseActionUrl.
-              let alias =
-                this.input.mController.getStyleAt(0).includes("searchengine") &&
-                this.input._parseActionUrl(this.input.mController.getFinalCompleteValueAt(0)).params.alias;
-              this.searchAlias = alias || null;
+            // If nothing is selected yet, select the first result if it is a
+            // pre-selected "heuristic" result.  (See UnifiedComplete.js.)
+            let selectHeuristic =
+              this.selectedIndex == -1 && this._isFirstResultHeuristic;
+            if (selectHeuristic) {
+              this.input.controller.setInitiallySelectedIndex(0);
+            }
 
-              // Format the alias or remove the formatting of the previous alias.
+            // If this is the heuristic result of a new search, format its
+            // search alias in the input or remove the formatting of the
+            // previous alias, as necessary.  We need to check selectHeuristic
+            // because the result may have already been added but only now is
+            // being selected, and we need to check gotResultForCurrentQuery
+            // because the result may be from the previous search and already
+            // selected and is now being reused.
+            if (selectHeuristic || !this.input.gotResultForCurrentQuery) {
               this.input.formatValue();
             }
 
-            // If nothing is selected yet, select the first result if it is a
-            // pre-selected "heuristic" result.  (See UnifiedComplete.js.)
-            if (this.selectedIndex == -1 && this._isFirstResultHeuristic) {
-              this.input.controller.setInitiallySelectedIndex(0);
-            }
             // If this is the first time we get the result from the current
             // search and we are not in the private context, we can speculatively
             // connect to the intended site as a performance optimization.
             if (!this.input.gotResultForCurrentQuery &&
                 this.input.speculativeConnectEnabled &&
                 !this.input.inPrivateContext &&
                 this.input.mController.matchCount > 0) {
               let firstStyle = this.input.mController.getStyleAt(0);
@@ -2917,17 +2899,43 @@ file, You can obtain one at http://mozil
           return iframe;
         ]]></body>
       </method>
 
     </implementation>
     <handlers>
 
       <handler event="SelectedOneOffButtonChanged"><![CDATA[
-        this._selectedOneOffChanged();
+        // Update all searchengine result items to use the newly selected
+        // engine.
+        for (let item of this.richlistbox.children) {
+          if (item.collapsed) {
+            break;
+          }
+          let url = item.getAttribute("url");
+          if (url) {
+            let action = item._parseActionUrl(url);
+            if (action && action.type == "searchengine") {
+              item._adjustAcItem();
+            }
+          }
+        }
+
+        // If the selection moved from the results to the one-off settings
+        // button, then call formatValue to remove the formatting of the search
+        // alias in the input, if any.  In all other cases the alias formatting
+        // is removed when the input's value setter calls formatValue, but in
+        // this specific case, at the time that formatValue is called,
+        // oneOffSearchButtons.selectedButton is still null, so the formatting
+        // is not removed.  The settings button is selected right after that.
+        if (this.oneOffSearchButtons.selectedButton ==
+              this.oneOffSearchButtons.settingsButtonCompact &&
+            (!event.detail || !event.detail.previousSelectedButton)) {
+          this.input.formatValue();
+        }
       ]]></handler>
 
       <handler event="mousedown"><![CDATA[
         // Required to make the xul:label.text-link elements in the search
         // suggestions notification work correctly when clicked on Linux.
         // This is copied from the mousedown handler in
         // browser-search-autocomplete-result-popup, which apparently had a
         // similar problem.
--- a/browser/components/search/content/search.xml
+++ b/browser/components/search/content/search.xml
@@ -733,32 +733,34 @@
         <getter><![CDATA[
           return this._selectedButton;
         ]]></getter>
         <setter><![CDATA[
           if (val && val.classList.contains("dummy")) {
             // Never select dummy buttons.
             val = null;
           }
-          if (this._selectedButton) {
-            this._selectedButton.removeAttribute("selected");
+          let previousButton = this._selectedButton;
+          if (previousButton) {
+            previousButton.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);
+          let event = new CustomEvent("SelectedOneOffButtonChanged", {
+            previousSelectedButton: previousButton,
+          });
           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">
--- a/toolkit/content/widgets/autocomplete.xml
+++ b/toolkit/content/widgets/autocomplete.xml
@@ -730,16 +730,19 @@
           this.input.controller.handleEnter(true, aEvent);
         ]]></body>
       </method>
 
       <property name="selectedIndex"
                 onget="return this.richlistbox.selectedIndex;">
         <setter>
           <![CDATA[
+          if (val != this.richlistbox.selectedIndex) {
+            this._previousSelectedIndex = this.richlistbox.selectedIndex;
+          }
           this.richlistbox.selectedIndex = val;
           // Since ensureElementIsVisible may cause an expensive Layout flush,
           // invoke it only if there may be a scrollbar, so if we could fetch
           // more results than we can show at once.
           // maxResults is the maximum number of fetched results, maxRows is the
           // maximum number of rows we show at once, without a scrollbar.
           if (this.mPopupOpen && this.maxResults > this.maxRows) {
             // when clearing the selection (val == -1, so selectedItem will be
@@ -747,16 +750,18 @@
             this.richlistbox.ensureElementIsVisible(
               this.richlistbox.selectedItem || this.richlistbox.firstElementChild);
           }
           return val;
         ]]>
         </setter>
       </property>
 
+      <field name="_previousSelectedIndex">-1</field>
+
       <method name="onSearchBegin">
         <body><![CDATA[
           this.richlistbox.mousedOverIndex = -1;
 
           if (typeof this._onSearchBegin == "function") {
             this._onSearchBegin();
           }
         ]]></body>