Bug 1561901 - Implement smart highlighting for search suggestions by adding a UrlbarUtils.HIGHLIGHT enum. r=dao
authorharry <htwyford@mozilla.com>
Thu, 11 Jul 2019 19:05:16 +0000
changeset 482458 75685314e8e28f6d615c0dfa1fa620f18770e03a
parent 482457 13b62d64e105c6bba7ebd89c10068c323d7e81bd
child 482459 bacec7b3bfde7f19c8ff08b882cf70bf2b54dfe1
push id89787
push userhtwyford@mozilla.com
push dateThu, 11 Jul 2019 20:58:12 +0000
treeherderautoland@75685314e8e2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdao
bugs1561901
milestone70.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 1561901 - Implement smart highlighting for search suggestions by adding a UrlbarUtils.HIGHLIGHT enum. r=dao Differential Revision: https://phabricator.services.mozilla.com/D36922
browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
browser/components/urlbar/UrlbarResult.jsm
browser/components/urlbar/UrlbarUtils.jsm
browser/components/urlbar/tests/browser/browser_autocomplete_a11y_label.js
browser/components/urlbar/tests/unit/test_UrlbarUtils_getTokenMatches.js
--- a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
+++ b/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
@@ -266,26 +266,31 @@ function makeUrlbarResult(tokens, info) 
   let action = PlacesUtils.parseActionUrl(info.url);
   if (action) {
     switch (action.type) {
       case "searchengine":
         return new UrlbarResult(
           UrlbarUtils.RESULT_TYPE.SEARCH,
           UrlbarUtils.RESULT_SOURCE.SEARCH,
           ...UrlbarResult.payloadAndSimpleHighlights(tokens, {
-            engine: [action.params.engineName, true],
-            suggestion: [action.params.searchSuggestion, true],
-            keyword: [action.params.alias, true],
-            query: [action.params.searchQuery.trim(), true],
-            icon: [info.icon, false],
+            engine: [action.params.engineName, UrlbarUtils.HIGHLIGHT.TYPED],
+            suggestion: [
+              action.params.searchSuggestion,
+              UrlbarUtils.HIGHLIGHT.SUGGESTED,
+            ],
+            keyword: [action.params.alias, UrlbarUtils.HIGHLIGHT.TYPED],
+            query: [
+              action.params.searchQuery.trim(),
+              UrlbarUtils.HIGHLIGHT.TYPED,
+            ],
+            icon: [info.icon],
             isKeywordOffer: [
               action.params.alias &&
                 !action.params.searchQuery.trim() &&
                 action.params.alias.startsWith("@"),
-              false,
             ],
           })
         );
       case "keyword": {
         let title = info.comment;
         if (!title) {
           // If the url doesn't have an host (e.g. javascript urls), comment
           // will be empty, and we can't build the usual title. Thus use the url.
@@ -301,81 +306,81 @@ function makeUrlbarResult(tokens, info) 
               .map(t => t.value)
               .join(" "),
           ]);
         }
         return new UrlbarResult(
           UrlbarUtils.RESULT_TYPE.KEYWORD,
           UrlbarUtils.RESULT_SOURCE.BOOKMARKS,
           ...UrlbarResult.payloadAndSimpleHighlights(tokens, {
-            title: [title, true],
-            url: [action.params.url, true],
-            keyword: [info.firstToken.value, true],
-            input: [action.params.input, false],
-            postData: [action.params.postData, false],
-            icon: [info.icon, false],
+            title: [title, UrlbarUtils.HIGHLIGHT.TYPED],
+            url: [action.params.url, UrlbarUtils.HIGHLIGHT.TYPED],
+            keyword: [info.firstToken.value, UrlbarUtils.HIGHLIGHT.TYPED],
+            input: [action.params.input],
+            postData: [action.params.postData],
+            icon: [info.icon],
           })
         );
       }
       case "extension":
         return new UrlbarResult(
           UrlbarUtils.RESULT_TYPE.OMNIBOX,
           UrlbarUtils.RESULT_SOURCE.OTHER_NETWORK,
           ...UrlbarResult.payloadAndSimpleHighlights(tokens, {
-            title: [info.comment, true],
-            content: [action.params.content, true],
-            keyword: [action.params.keyword, true],
-            icon: [info.icon, false],
+            title: [info.comment, UrlbarUtils.HIGHLIGHT.TYPED],
+            content: [action.params.content, UrlbarUtils.HIGHLIGHT.TYPED],
+            keyword: [action.params.keyword, UrlbarUtils.HIGHLIGHT.TYPED],
+            icon: [info.icon],
           })
         );
       case "remotetab":
         return new UrlbarResult(
           UrlbarUtils.RESULT_TYPE.REMOTE_TAB,
           UrlbarUtils.RESULT_SOURCE.TABS,
           ...UrlbarResult.payloadAndSimpleHighlights(tokens, {
-            url: [action.params.url, true],
-            title: [info.comment, true],
-            device: [action.params.deviceName, true],
-            icon: [info.icon, false],
+            url: [action.params.url, UrlbarUtils.HIGHLIGHT.TYPED],
+            title: [info.comment, UrlbarUtils.HIGHLIGHT.TYPED],
+            device: [action.params.deviceName, UrlbarUtils.HIGHLIGHT.TYPED],
+            icon: [info.icon],
           })
         );
       case "switchtab":
         return new UrlbarResult(
           UrlbarUtils.RESULT_TYPE.TAB_SWITCH,
           UrlbarUtils.RESULT_SOURCE.TABS,
           ...UrlbarResult.payloadAndSimpleHighlights(tokens, {
-            url: [action.params.url, true],
-            title: [info.comment, true],
-            device: [action.params.deviceName, true],
-            icon: [info.icon, false],
+            url: [action.params.url, UrlbarUtils.HIGHLIGHT.TYPED],
+            title: [info.comment, UrlbarUtils.HIGHLIGHT.TYPED],
+            device: [action.params.deviceName, UrlbarUtils.HIGHLIGHT.TYPED],
+            icon: [info.icon],
           })
         );
       case "visiturl":
         return new UrlbarResult(
           UrlbarUtils.RESULT_TYPE.URL,
           UrlbarUtils.RESULT_SOURCE.OTHER_LOCAL,
           ...UrlbarResult.payloadAndSimpleHighlights(tokens, {
-            title: [info.comment, true],
-            url: [action.params.url, true],
-            icon: [info.icon, false],
+            title: [info.comment, UrlbarUtils.HIGHLIGHT.TYPED],
+            url: [action.params.url, UrlbarUtils.HIGHLIGHT.TYPED],
+            icon: [info.icon],
           })
         );
       default:
         Cu.reportError(`Unexpected action type: ${action.type}`);
         return null;
     }
   }
 
   if (info.style.includes("priority-search")) {
     return new UrlbarResult(
       UrlbarUtils.RESULT_TYPE.SEARCH,
       UrlbarUtils.RESULT_SOURCE.SEARCH,
       ...UrlbarResult.payloadAndSimpleHighlights(tokens, {
-        engine: [info.comment, true],
-        icon: [info.icon, false],
+        engine: [info.comment, UrlbarUtils.HIGHLIGHT.TYPED],
+        icon: [info.icon],
       })
     );
   }
 
   // This is a normal url/title tuple.
   let source;
   let tags = [];
   let comment = info.comment;
@@ -404,15 +409,15 @@ function makeUrlbarResult(tokens, info) 
     source = UrlbarUtils.RESULT_SOURCE.OTHER_LOCAL;
   } else {
     source = UrlbarUtils.RESULT_SOURCE.HISTORY;
   }
   return new UrlbarResult(
     UrlbarUtils.RESULT_TYPE.URL,
     source,
     ...UrlbarResult.payloadAndSimpleHighlights(tokens, {
-      url: [info.url, true],
-      icon: [info.icon, false],
-      title: [comment, true],
-      tags: [tags, true],
+      url: [info.url, UrlbarUtils.HIGHLIGHT.TYPED],
+      icon: [info.icon],
+      title: [comment, UrlbarUtils.HIGHLIGHT.TYPED],
+      tags: [tags, UrlbarUtils.HIGHLIGHT.TYPED],
     })
   );
 }
--- a/browser/components/urlbar/UrlbarResult.jsm
+++ b/browser/components/urlbar/UrlbarResult.jsm
@@ -133,59 +133,64 @@ class UrlbarResult {
    * Returns an icon url.
    * @returns {string} url of the icon.
    */
   get icon() {
     return this.payload.icon;
   }
 
   /**
-   * A convenience function that takes a payload annotated with should-highlight
-   * bools and returns the payload and the payload's highlights.  Use this
-   * function when the highlighting required by your payload is based on simple
-   * substring matching, as done by UrlbarUtils.getTokenMatches().  Pass the
-   * return values as the `payload` and `payloadHighlights` params of the
-   * UrlbarResult constructor.
+   * A convenience function that takes a payload annotated with
+   * UrlbarUtils.HIGHLIGHT enums and returns the payload and the payload's
+   * highlights. Use this function when the highlighting required by your
+   * payload is based on simple substring matching, as done by
+   * UrlbarUtils.getTokenMatches(). Pass the return values as the `payload` and
+   * `payloadHighlights` params of the UrlbarResult constructor.
+   * `payloadHighlights` is optional. If omitted, payload will not be
+   * highlighted.
    *
    * If the payload doesn't have a title or has an empty title, and it also has
    * a URL, then this function also sets the title to the URL's domain.
    *
    * @param {array} tokens The tokens that should be highlighted in each of the
    *        payload properties.
    * @param {object} payloadInfo An object that looks like this:
    *        { payloadPropertyName: payloadPropertyInfo }
    *
    *        Each payloadPropertyInfo may be either a string or an array.  If
    *        it's a string, then the property value will be that string, and no
    *        highlighting will be applied to it.  If it's an array, then it
-   *        should look like this: [payloadPropertyValue, shouldHighlight].
+   *        should look like this: [payloadPropertyValue, highlightType].
    *        payloadPropertyValue may be a string or an array of strings.  If
    *        it's a string, then the payloadHighlights in the return value will
    *        be an array of match highlights as described in
    *        UrlbarUtils.getTokenMatches().  If it's an array, then
    *        payloadHighlights will be an array of arrays of match highlights,
    *        one element per element in payloadPropertyValue.
    * @returns {array} An array [payload, payloadHighlights].
    */
   static payloadAndSimpleHighlights(tokens, payloadInfo) {
     // Convert string values in payloadInfo to [value, false] arrays.
     for (let [name, info] of Object.entries(payloadInfo)) {
       if (typeof info == "string") {
-        payloadInfo[name] = [info, false];
+        payloadInfo[name] = [info];
       }
     }
 
     if (
       (!payloadInfo.title || !payloadInfo.title[0]) &&
       payloadInfo.url &&
       typeof payloadInfo.url[0] == "string"
     ) {
       // If there's no title, show the domain as the title.  Not all valid URLs
       // have a domain.
-      payloadInfo.title = payloadInfo.title || ["", true];
+      payloadInfo.title = payloadInfo.title || [
+        "",
+        UrlbarUtils.HIGHLIGHT.TYPED,
+      ];
       try {
         payloadInfo.title[0] = new URL(payloadInfo.url[0]).host;
       } catch (e) {}
     }
 
     if (payloadInfo.url) {
       // For display purposes we need to unescape the url.
       payloadInfo.displayUrl = [...payloadInfo.url];
@@ -210,19 +215,21 @@ class UrlbarResult {
     }
 
     let entries = Object.entries(payloadInfo);
     return [
       entries.reduce((payload, [name, [val, _]]) => {
         payload[name] = val;
         return payload;
       }, {}),
-      entries.reduce((highlights, [name, [val, shouldHighlight]]) => {
-        if (shouldHighlight) {
+      entries.reduce((highlights, [name, [val, highlightType]]) => {
+        if (highlightType) {
           highlights[name] = !Array.isArray(val)
-            ? UrlbarUtils.getTokenMatches(tokens, val || "")
-            : val.map(subval => UrlbarUtils.getTokenMatches(tokens, subval));
+            ? UrlbarUtils.getTokenMatches(tokens, val || "", highlightType)
+            : val.map(subval =>
+                UrlbarUtils.getTokenMatches(tokens, subval, highlightType)
+              );
         }
         return highlights;
       }, {}),
     ];
   }
 }
--- a/browser/components/urlbar/UrlbarUtils.jsm
+++ b/browser/components/urlbar/UrlbarUtils.jsm
@@ -123,16 +123,23 @@ var UrlbarUtils = {
     COMMIT: 3,
     CANCELED: 4,
   },
 
   // Limit the length of titles and URLs we display so layout doesn't spend too
   // much time building text runs.
   MAX_TEXT_LENGTH: 255,
 
+  // Whether a result should be highlighted up to the point the user has typed
+  // or after that point.
+  HIGHLIGHT: {
+    TYPED: 1,
+    SUGGESTED: 2,
+  },
+
   /**
    * Adds a url to history as long as it isn't in a private browsing window,
    * and it is valid.
    *
    * @param {string} url The url to add to history.
    * @param {nsIDomWindow} window The window from where the url is being added.
    */
   addToUrlbarHistory(url, window) {
@@ -239,37 +246,45 @@ var UrlbarUtils = {
   /**
    * Returns a list of all the token substring matches in a string.  Matching is
    * case insensitive.  Each match in the returned list is a tuple: [matchIndex,
    * matchLength].  matchIndex is the index in the string of the match, and
    * matchLength is the length of the match.
    *
    * @param {array} tokens The tokens to search for.
    * @param {string} str The string to match against.
+   * @param {boolean} highlightType If HIGHLIGHT.SUGGESTED, return a list of all
+   *   the token string non-matches. Otherwise, return matches.
    * @returns {array} An array: [
    *            [matchIndex_0, matchLength_0],
    *            [matchIndex_1, matchLength_1],
    *            ...
    *            [matchIndex_n, matchLength_n]
    *          ].
    *          The array is sorted by match indexes ascending.
    */
-  getTokenMatches(tokens, str) {
+  getTokenMatches(tokens, str, highlightType) {
     str = str.toLocaleLowerCase();
     // To generate non-overlapping ranges, we start from a 0-filled array with
     // the same length of the string, and use it as a collision marker, setting
     // 1 where a token matches.
-    let hits = new Array(str.length).fill(0);
+    let hits = new Array(str.length).fill(
+      highlightType == this.HIGHLIGHT.SUGGESTED ? 1 : 0
+    );
     for (let { lowerCaseValue } of tokens) {
       // Ideally we should never hit the empty token case, but just in case
       // the `needle` check protects us from an infinite loop.
       for (let index = 0, needle = lowerCaseValue; index >= 0 && needle; ) {
         index = str.indexOf(needle, index);
         if (index >= 0) {
-          hits.fill(1, index, index + needle.length);
+          hits.fill(
+            highlightType == this.HIGHLIGHT.SUGGESTED ? 0 : 1,
+            index,
+            index + needle.length
+          );
           index += needle.length;
         }
       }
     }
     // Starting from the collision array, generate [start, len] tuples
     // representing the ranges to be highlighted.
     let ranges = [];
     for (let index = hits.indexOf(1); index >= 0 && index < hits.length; ) {
--- a/browser/components/urlbar/tests/browser/browser_autocomplete_a11y_label.js
+++ b/browser/components/urlbar/tests/browser/browser_autocomplete_a11y_label.js
@@ -97,19 +97,19 @@ add_task(async function searchSuggestion
     length,
     3,
     "Should get at least heuristic result + two search suggestions"
   );
   // The first expected search is the search term itself since the heuristic
   // result will come before the search suggestions.
   let expectedSearches = [
     "foo",
-    "foofoo",
-    // The extra spaces is here due to bug 1550644.
-    "foo bar ",
+    // The extra space is here due to bug 1550644.
+    "foofoo ",
+    "foo bar",
   ];
   for (let i = 0; i < length; i++) {
     let result = await UrlbarTestUtils.getDetailsOfResultAt(window, i);
     if (result.type === UrlbarUtils.RESULT_TYPE.SEARCH) {
       Assert.greaterOrEqual(
         expectedSearches.length,
         0,
         "Should still have expected searches remaining"
--- a/browser/components/urlbar/tests/unit/test_UrlbarUtils_getTokenMatches.js
+++ b/browser/components/urlbar/tests/unit/test_UrlbarUtils_getTokenMatches.js
@@ -151,14 +151,74 @@ add_task(function test() {
     },
   ];
   for (let { tokens, phrase, expected } of tests) {
     tokens = tokens.map(t => ({
       value: t,
       lowerCaseValue: t.toLocaleLowerCase(),
     }));
     Assert.deepEqual(
-      UrlbarUtils.getTokenMatches(tokens, phrase),
+      UrlbarUtils.getTokenMatches(tokens, phrase, UrlbarUtils.HIGHLIGHT.TYPED),
       expected,
       `Match "${tokens.map(t => t.value).join(", ")}" on "${phrase}"`
     );
   }
 });
+
+add_task(function testReverse() {
+  const tests = [
+    {
+      tokens: ["mozilla", "is", "i"],
+      phrase: "mozilla is for the Open Web",
+      expected: [[7, 1], [10, 17]],
+    },
+    {
+      tokens: ["\u9996"],
+      phrase: "Test \u9996\u9875 Test",
+      expected: [[0, 5], [6, 6]],
+    },
+    {
+      tokens: ["mo", "zilla"],
+      phrase: "mOzIlLa",
+      expected: [],
+    },
+    {
+      tokens: ["MO", "ZILLA"],
+      phrase: "mozilla",
+      expected: [],
+    },
+    {
+      tokens: [""], // Should never happen in practice.
+      phrase: "mozilla",
+      expected: [[0, 7]],
+    },
+    {
+      tokens: ["mo", "om"],
+      phrase: "mozilla mozzarella momo",
+      expected: [[2, 6], [10, 9]],
+    },
+    {
+      tokens: ["mo", "om"],
+      phrase: "MOZILLA MOZZARELLA MOMO",
+      expected: [[2, 6], [10, 9]],
+    },
+    {
+      tokens: ["MO", "OM"],
+      phrase: "mozilla mozzarella momo",
+      expected: [[2, 6], [10, 9]],
+    },
+  ];
+  for (let { tokens, phrase, expected } of tests) {
+    tokens = tokens.map(t => ({
+      value: t,
+      lowerCaseValue: t.toLocaleLowerCase(),
+    }));
+    Assert.deepEqual(
+      UrlbarUtils.getTokenMatches(
+        tokens,
+        phrase,
+        UrlbarUtils.HIGHLIGHT.SUGGESTED
+      ),
+      expected,
+      `Match "${tokens.map(t => t.value).join(", ")}" on "${phrase}"`
+    );
+  }
+});