Bug 1561901 - Implement smart highlighting for search suggestions by adding a UrlbarUtils.HIGHLIGHT enum. r=dao

Differential Revision: https://phabricator.services.mozilla.com/D36922 --HG-- extra : moz-landing-system : lando
2024-10-25 19:25:43 +00:00 · 2019-07-11 19:05:16 +00:00 · 2019-07-11 19:05:16 +00:00 · 959aa843e5
commit 959aa843e5
parent 609a5a9af0
5 changed files with 140 additions and 53 deletions
--- a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
+++ b/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
@ -271,16 +271,21 @@ function makeUrlbarResult(tokens, info) {
          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,
            ],
          })
        );
@ -306,12 +311,12 @@ function makeUrlbarResult(tokens, info) {
          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],
          })
        );
      }
@ -320,10 +325,10 @@ function makeUrlbarResult(tokens, info) {
          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":
@ -331,10 +336,10 @@ function makeUrlbarResult(tokens, info) {
          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":
@ -342,10 +347,10 @@ function makeUrlbarResult(tokens, info) {
          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":
@ -353,9 +358,9 @@ function makeUrlbarResult(tokens, info) {
          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:
@ -369,8 +374,8 @@ function makeUrlbarResult(tokens, info) {
      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],
      })
    );
  }
@ -409,10 +414,10 @@ function makeUrlbarResult(tokens, info) {
    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
@ -138,12 +138,14 @@ class UrlbarResult {
  }

  /**
-   * 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.
@ -156,7 +158,7 @@ class UrlbarResult {
   *        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
@ -169,7 +171,7 @@ class UrlbarResult {
    // 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];
      }
    }

@ -180,7 +182,10 @@ class UrlbarResult {
    ) {
      // 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) {}
@ -215,11 +220,13 @@ class UrlbarResult {
        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
@ -128,6 +128,13 @@ var UrlbarUtils = {
  // 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.
@ -244,6 +251,8 @@ var UrlbarUtils = {
   *
   * @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],
@ -252,19 +261,25 @@ var UrlbarUtils = {
   *          ].
   *          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;
        }
      }
--- a/browser/components/urlbar/tests/browser/browser_autocomplete_a11y_label.js
+++ b/browser/components/urlbar/tests/browser/browser_autocomplete_a11y_label.js
@ -102,9 +102,9 @@ add_task(async function searchSuggestions() {
  // 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);
--- a/browser/components/urlbar/tests/unit/test_UrlbarUtils_getTokenMatches.js
+++ b/browser/components/urlbar/tests/unit/test_UrlbarUtils_getTokenMatches.js
@ -156,7 +156,67 @@ add_task(function test() {
      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}"`
    );