Bug 1484737 - Improve the handling of search alias highlighting in the urlbar. r=Mardak,mak a=pascalc
authorDrew Willcoxon <adw@mozilla.com>
Thu, 06 Sep 2018 00:20:45 +0000
changeset 492458 c365b95955141b6336da5656e4406186e457ddb1
parent 492457 b9d0f86adcd418361d58938427128a95fecea7e4
child 492459 0daf051f7fc7c5fc5c9dacb33f4d1b49e755c952
push id1815
push userffxbld-merge
push dateMon, 15 Oct 2018 10:40:45 +0000
treeherdermozilla-release@18d4c09e9378 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMardak, mak, pascalc
bugs1484737
milestone63.0
Bug 1484737 - Improve the handling of search alias highlighting in the urlbar. r=Mardak,mak a=pascalc This has two parts: (1) urlbar already had a formatValue method. Right now, it only does the URL formatting (domain highlighting, crossing out https for mixed content pages) that we do when the urlbar is not focused. This patch generalizes that method into a kind of "any formatting you want to do, do it here" method, and it adds alias formatting. formatValue is called by the base autocomplete binding when `value` is set. So it's called when the selection in the popup changes and the autocomplete controller subsequently sets the input value. (It's also called by urlbar on focus and blur.) And if anyone else sets the value directly, it'll be called then too of course. But it's not called when you're just typing in the input, so I added a call in urlbar.onResultsAdded, where we were calling highlightSearchAlias, to handle the first heuristic result being added or modified as a result of what you type. So I think that should cover all possible times we need to highlight the alias? (2) Just looking at the selected result to get the alias in the input doesn't work all the time. If you click a search tile on newtab and then key around in the popup, sometimes when you key down to the one-off buttons, the input value reverts to the alias (it's the user-typed value I guess?), but at the time that the value setter is called during the revert, the popup's selected index is still the last selection in the popup. IOW the selected index doesn't match up with what's in the input. Rather than deal with that, it seems safer to call PlacesSearchAutocompleteProvider.findMatchByAlias() on the first word in the input. But that has a couple of problems. It's async, and I noticed there can be a slight delay in the highlighting appearing. Also, we've already gotten the information returned by that method, when we generated the results in the first place, so it seems inelegant to call it again. So what I've done instead is to cache aliases in the popup when results are added, and then just look up the first word in the input in these aliases. We shouldn't reset this cache until the first result of a new search comes in. Differential Revision: https://phabricator.services.mozilla.com/D3850
browser/base/content/urlbarBindings.xml
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -510,132 +510,170 @@ file, You can obtain one at http://mozil
           // nsIURIFixup::createFixupURI with the result will produce a different URI.
           return this._mayTrimURLs ? trimURL(aURL) : aURL;
         ]]></body>
       </method>
 
       <field name="_formattingEnabled">true</field>
 
       <!--
-        If the input value is a URL, the input is not focused, and formatting is
-        enabled, this method highlights the domain, and if mixed content is
-        present, it crosses out the https scheme.  It also ensures that the host
-        is visible (not scrolled out of sight).  Otherwise it removes formatting.
+        This is used only as an optimization to avoid removing formatting in
+        the _remove* format methods when no formatting is actually applied.
+      -->
+      <field name="_formattingApplied">false</field>
+
+      <!--
+        This method tries to apply styling to the text in the input, depending
+        on the text.  See the _format* methods.
+      -->
+      <method name="formatValue">
+        <body><![CDATA[
+          if (!this.editor || !this.editor.rootElement.firstChild.textContent) {
+            return;
+          }
+
+          // Remove the current formatting.
+          this._removeURLFormat();
+          this._removeSearchAliasFormat();
+          this._formattingApplied = false;
+
+          // Apply new formatting.  Formatter methods should return true if they
+          // successfully formatted the value and false if not.  We apply only
+          // one formatter at a time, so we stop at the first successful one.
+          let formatterMethods = [
+            "_formatURL",
+            "_formatSearchAlias",
+          ];
+          this._formattingApplied = formatterMethods.some(m => this[m]());
+        ]]></body>
+      </method>
+
+      <method name="_removeURLFormat">
+        <body><![CDATA[
+          this.scheme.value = "";
+          if (!this._formattingApplied) {
+            return;
+          }
+          let controller = this.editor.selectionController;
+          let strikeOut =
+            controller.getSelection(controller.SELECTION_URLSTRIKEOUT);
+          strikeOut.removeAllRanges();
+          let selection =
+            controller.getSelection(controller.SELECTION_URLSECONDARY);
+          selection.removeAllRanges();
+          this._formatScheme(controller.SELECTION_URLSTRIKEOUT, true);
+          this._formatScheme(controller.SELECTION_URLSECONDARY, true);
+          this.inputField.style.setProperty("--urlbar-scheme-size", "0px");
+        ]]></body>
+      </method>
+
+      <!--
+        If the input value is a URL and the input is not focused, this
+        formatter method highlights the domain, and if mixed content is present,
+        it crosses out the https scheme.  It also ensures that the host is
+        visible (not scrolled out of sight).
 
         @param  onlyEnsureFormattedHostVisible
                 Pass true to skip formatting and instead only ensure that the
                 host is visible.
+        @return True if formatting was applied and false if not.
       -->
-      <method name="formatValue">
+      <method name="_formatURL">
         <parameter name="onlyEnsureFormattedHostVisible"/>
         <body><![CDATA[
-          // Used to avoid re-entrance in async callbacks.
-          let instance = this._formattingInstance = {};
-
-          if (!this.editor)
-            return;
-
-          let controller, strikeOut, selection;
-
-          if (!onlyEnsureFormattedHostVisible) {
-            // Cleanup previously set styles.
-            this.scheme.value = "";
-            if (this._formattingEnabled) {
-              controller = this.editor.selectionController;
-              strikeOut = controller.getSelection(controller.SELECTION_URLSTRIKEOUT);
-              strikeOut.removeAllRanges();
-              selection = controller.getSelection(controller.SELECTION_URLSECONDARY);
-              selection.removeAllRanges();
-              this.formatScheme(controller.SELECTION_URLSTRIKEOUT, true);
-              this.formatScheme(controller.SELECTION_URLSECONDARY, true);
-              this.inputField.style.setProperty("--urlbar-scheme-size", "0px");
-            }
+          if (this.focused) {
+            return false;
           }
 
           let textNode = this.editor.rootElement.firstChild;
           let value = textNode.textContent;
-          if (!value)
-            return;
-
-          if (this.focused)
-            return;
 
           // Get the URL from the fixup service:
           let flags = Services.uriFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS |
                       Services.uriFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP;
           let uriInfo;
           try {
             uriInfo = Services.uriFixup.getFixupURIInfo(value, flags);
           } catch (ex) {}
           // Ignore if we couldn't make a URI out of this, the URI resulted in a search,
           // or the URI has a non-http(s)/ftp protocol.
           if (!uriInfo ||
               !uriInfo.fixedURI ||
               uriInfo.keywordProviderName ||
               !["http", "https", "ftp"].includes(uriInfo.fixedURI.scheme)) {
-            return;
+            return false;
           }
 
           // If we trimmed off the http scheme, ensure we stick it back on before
           // trying to figure out what domain we're accessing, so we don't get
           // confused by user:pass@host http URLs. We later use
           // trimmedLength to ensure we don't count the length of a trimmed protocol
           // when determining which parts of the URL to highlight as "preDomain".
           let trimmedLength = 0;
           if (uriInfo.fixedURI.scheme == "http" && !value.startsWith("http://")) {
             value = "http://" + value;
             trimmedLength = "http://".length;
           }
 
           let matchedURL = value.match(/^(([a-z]+:\/\/)(?:[^\/#?]+@)?)(\S+?)(?::\d+)?\s*(?:[\/#?]|$)/);
-          if (!matchedURL)
-            return;
+          if (!matchedURL) {
+            return false;
+          }
 
           let [, preDomain, schemeWSlashes, domain] = matchedURL;
           // We strip http, so we should not show the scheme box for it.
           if (!this._mayTrimURLs || schemeWSlashes != "http://") {
             this.scheme.value = schemeWSlashes;
             this.inputField.style.setProperty("--urlbar-scheme-size",
                                               schemeWSlashes.length + "ch");
           }
 
+          // Used to avoid re-entrance in the requestAnimationFrame callback.
+          let instance = this._formatURLInstance = {};
+
           // Make sure the host is always visible. Since it is aligned on
           // the first strong directional character, we set scrollLeft
           // appropriately to ensure the domain stays visible in case of an
           // overflow.
           window.requestAnimationFrame(() => {
             // Check for re-entrance. On focus change this formatting code is
             // invoked regardless, thus this should be enough.
-            if (this._formattingInstance != instance)
+            if (this._formatURLInstance != instance) {
               return;
+            }
             let directionality = window.windowUtils.getDirectionFromText(domain);
             // In the future, for example in bug 525831, we may add a forceRTL
             // char just after the domain, and in such a case we should not
             // scroll to the left.
             if (directionality == window.windowUtils.DIRECTION_RTL &&
                 value[preDomain.length + domain.length] != "\u200E") {
               this.inputField.scrollLeft = this.inputField.scrollLeftMax;
             }
           });
 
-          if (onlyEnsureFormattedHostVisible || !this._formattingEnabled)
-            return;
+          if (onlyEnsureFormattedHostVisible || !this._formattingEnabled) {
+            return false;
+          }
 
-          this.formatScheme(controller.SELECTION_URLSECONDARY);
+          let controller = this.editor.selectionController;
+
+          this._formatScheme(controller.SELECTION_URLSECONDARY);
 
           // Strike out the "https" part if mixed active content is loaded.
           if (this.getAttribute("pageproxystate") == "valid" &&
               value.startsWith("https:") &&
               gBrowser.securityUI.state &
                 Ci.nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT) {
             let range = document.createRange();
             range.setStart(textNode, 0);
             range.setEnd(textNode, 5);
+            let strikeOut =
+              controller.getSelection(controller.SELECTION_URLSTRIKEOUT);
             strikeOut.addRange(range);
-            this.formatScheme(controller.SELECTION_URLSTRIKEOUT);
+            this._formatScheme(controller.SELECTION_URLSTRIKEOUT);
           }
 
           let baseDomain = domain;
           let subDomain = "";
           try {
             baseDomain = Services.eTLD.getBaseDomainFromHost(uriInfo.fixedURI.host);
             if (!domain.endsWith(baseDomain)) {
               // getBaseDomainFromHost converts its resultant to ACE.
@@ -643,35 +681,40 @@ file, You can obtain one at http://mozil
                                .getService(Ci.nsIIDNService);
               baseDomain = IDNService.convertACEtoUTF8(baseDomain);
             }
           } catch (e) {}
           if (baseDomain != domain) {
             subDomain = domain.slice(0, -baseDomain.length);
           }
 
+          let selection =
+            controller.getSelection(controller.SELECTION_URLSECONDARY);
+
           let rangeLength = preDomain.length + subDomain.length - trimmedLength;
           if (rangeLength) {
             let range = document.createRange();
             range.setStart(textNode, 0);
             range.setEnd(textNode, rangeLength);
             selection.addRange(range);
           }
 
           let startRest = preDomain.length + domain.length - trimmedLength;
           if (startRest < value.length - trimmedLength) {
             let range = document.createRange();
             range.setStart(textNode, startRest);
             range.setEnd(textNode, value.length - trimmedLength);
             selection.addRange(range);
           }
+
+          return true;
         ]]></body>
       </method>
 
-      <method name="formatScheme">
+      <method name="_formatScheme">
         <parameter name="selectionType"/>
         <parameter name="clear"/>
         <body><![CDATA[
           let editor = this.scheme.editor;
           let controller = editor.selectionController;
           let textNode = editor.rootElement.firstChild;
           let selection = controller.getSelection(selectionType);
           if (clear) {
@@ -680,16 +723,94 @@ file, You can obtain one at http://mozil
             let r = document.createRange();
             r.setStart(textNode, 0);
             r.setEnd(textNode, textNode.textContent.length);
             selection.addRange(r);
           }
         ]]></body>
       </method>
 
+      <method name="_removeSearchAliasFormat">
+        <body><![CDATA[
+          if (!this._formattingApplied) {
+            return;
+          }
+          let selection = this.editor.selectionController.getSelection(
+            Ci.nsISelectionController.SELECTION_FIND
+          );
+          selection.removeAllRanges();
+        ]]></body>
+      </method>
+
+      <!--
+        If the input value starts with a search alias, this formatter method
+        highlights it.
+
+        @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)) {
+            return false;
+          }
+
+          let index = value.indexOf(this.popup.searchAlias);
+
+          // 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);
+          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
+          // to happen; we want it to use our colors as given (even if setColors
+          // thinks the contrast is too low).  But it's a nice feature for non-
+          // default themes, where the contrast between our background color and
+          // the input's frame color might actually be too low.  We can
+          // (hackily) force setColors to use our colors as given by passing
+          // them as the alternate colors.  Otherwise, allow setColors to swap
+          // them, which we can do by passing "currentColor".  See
+          // nsTextPaintStyle::GetHighlightColors for details.
+          if (this.querySelector(":-moz-lwtheme") ||
+              (AppConstants.platform == "win" &&
+               window.matchMedia("(-moz-windows-default-theme: 0)").matches)) {
+            // non-default theme(s)
+            selection.setColors(fg, bg, "currentColor", "currentColor");
+          } else {
+            // default themes
+            selection.setColors(fg, bg, fg, bg);
+          }
+
+          return true;
+        ]]></body>
+      </method>
+
       <method name="handleRevert">
         <body><![CDATA[
           var isScrolling = this.popupOpen;
 
           gBrowser.userTypedValue = null;
 
           // don't revert to last valid url unless page is NOT loading
           // and user is NOT key-scrolling through autocomplete list
@@ -1459,25 +1580,25 @@ file, You can obtain one at http://mozil
             case "resize":
               if (aEvent.target == window) {
                 // Close the popup since it would be wrongly sized, we'll
                 // recalculate a proper size on reopening. For example, this may
                 // happen when using special OS resize functions like Win+Arrow.
                 this.closePopup();
 
                 // Make sure the host remains visible in the input field (via
-                // formatValue) when the window is resized.  We don't want to
+                // _formatURL) when the window is resized.  We don't want to
                 // hurt resize performance though, so do this only after resize
                 // events have stopped and a small timeout has elapsed.
                 if (this._resizeThrottleTimeout) {
                   clearTimeout(this._resizeThrottleTimeout);
                 }
                 this._resizeThrottleTimeout = setTimeout(() => {
                   this._resizeThrottleTimeout = null;
-                  this.formatValue(true);
+                  this._formatURL(true);
                 }, 100);
               }
               break;
           }
         ]]></body>
       </method>
 
       <method name="updateTextOverflow">
@@ -1799,92 +1920,16 @@ file, You can obtain one at http://mozil
               this._whichSearchSuggestionsNotification = which;
             }, {once: true});
           }
 
           this.gotResultForCurrentQuery = false;
           this.controller.startSearch(value);
         ]]></body>
       </method>
-
-      <!--
-        Highlights the search alias in the input, or clears the highlight if
-        there is no alias.  To determine in an efficient manner whether the
-        input contains an alias, this method looks at the first (heuristic)
-        result.  If it's a searchengine result with an alias, then it looks for
-        that alias in the input.  Otherwise it clears the highlight.  That means
-        that if the input contains an alias but the alias result is not first,
-        the alias will not be highlighted.
-      -->
-      <method name="highlightSearchAlias">
-        <body><![CDATA[
-          if (!this.editor) {
-            return;
-          }
-
-          // 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
-          );
-          selection.removeAllRanges();
-
-          let textNode = this.editor.rootElement.firstChild;
-          let value = textNode.textContent;
-          if (!value) {
-            return;
-          }
-
-          // Alias results have the searchengine style.  Check that first to
-          // avoid the more expensive regexp in _parseActionUrl when possible
-          // since this method is called by the popup every time you start a
-          // search.
-          let alias =
-            this.mController.matchCount &&
-            this.mController.getStyleAt(0).includes("searchengine") &&
-            this._parseActionUrl(this.mController.getFinalCompleteValueAt(0)).params.alias;
-          if (!alias) {
-            return;
-          }
-
-          let index = value.indexOf(alias);
-          if (index < 0) {
-            return;
-          }
-
-          let range = document.createRange();
-          range.setStart(textNode, index);
-          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
-          // to happen; we want it to use our colors as given (even if setColors
-          // thinks the contrast is too low).  But it's a nice feature for non-
-          // default themes, where the contrast between our background color and
-          // the input's frame color might actually be too low.  We can
-          // (hackily) force setColors to use our colors as given by passing
-          // them as the alternate colors.  Otherwise, allow setColors to swap
-          // them, which we can do by passing "currentColor".  See
-          // nsTextPaintStyle::GetHighlightColors for details.
-          if (this.querySelector(":-moz-lwtheme") ||
-              (AppConstants.platform == "win" &&
-               window.matchMedia("(-moz-windows-default-theme: 0)").matches)) {
-            // non-default theme(s)
-            selection.setColors(fg, bg, "currentColor", "currentColor");
-          } else {
-            // default themes
-            selection.setColors(fg, bg, fg, bg);
-          }
-        ]]></body>
-      </method>
     </implementation>
 
     <handlers>
       <handler event="keydown"><![CDATA[
         if (this._noActionKeys.has(event.keyCode) &&
             this.popup.selectedIndex >= 0 &&
             !this._pressedNoActionKeys.has(event.keyCode)) {
           if (this._pressedNoActionKeys.size == 0) {
@@ -2624,25 +2669,41 @@ 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[
-            // Highlight the search alias in the input if one is present, or
-            // clear the highlight otherwise.  highlightSearchAlias examines
-            // only the first result, so as an optimzation, call it only when
-            // this is the first result.
             if (!this.input.gotResultForCurrentQuery) {
-              this.input.highlightSearchAlias();
+              // 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;
+
+              // Format the alias or remove the formatting of the previous alias.
+              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) {
               // Don't fire DOMMenuItemActive so that screen readers still see
               // the input as being focused.
               this.richlistbox.suppressMenuItemEvent = true;