Bug 1626891 - Style tail suggestions differently. r=mak
authorHarry Twyford <htwyford@mozilla.com>
Tue, 19 May 2020 13:57:54 +0000
changeset 530825 e47fbd2bd52d12547adfd826783fc50e7ac2978c
parent 530824 e35047570e96b823d3c3c0920ae64dd8a50a7e40
child 530826 5571b75d82b66bad47fe582fb1d1f671254cd6fa
push id37433
push userdluca@mozilla.com
push dateWed, 20 May 2020 03:39:31 +0000
treeherdermozilla-central@855249e545c3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1626891
milestone78.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 1626891 - Style tail suggestions differently. r=mak Differential Revision: https://phabricator.services.mozilla.com/D74740
browser/components/extensions/test/xpcshell/test_ext_urlbar.js
browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm
browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
browser/components/urlbar/UrlbarResult.jsm
browser/components/urlbar/UrlbarUtils.jsm
browser/components/urlbar/UrlbarView.jsm
browser/components/urlbar/tests/unit/head.js
browser/components/urlbar/tests/unit/test_search_suggestions_tail.js
browser/themes/shared/urlbarView.inc.css
toolkit/components/search/SearchSuggestionController.jsm
toolkit/components/search/tests/xpcshell/test_searchSuggest.js
--- a/browser/components/extensions/test/xpcshell/test_ext_urlbar.js
+++ b/browser/components/extensions/test/xpcshell/test_ext_urlbar.js
@@ -281,17 +281,19 @@ add_task(async function test_onProviderR
       type: UrlbarUtils.RESULT_TYPE.SEARCH,
       source: UrlbarUtils.RESULT_SOURCE.SEARCH,
       title: "test",
       heuristic: true,
       payload: {
         query: "test",
         engine: "Test engine",
         suggestion: undefined,
+        tailPrefix: undefined,
         tail: undefined,
+        tailOffsetIndex: -1,
         keyword: undefined,
         isSearchHistory: false,
         icon: "",
         keywordOffer: false,
       },
     },
     // The second result should be our search suggestion result since the
     // default muxer sorts search suggestion results before other types.
--- a/browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm
+++ b/browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm
@@ -364,35 +364,43 @@ class ProviderSearchSuggestions extends 
       if (
         !suggestion ||
         suggestion.entry.value == searchString ||
         looksLikeUrl(suggestion.entry.value)
       ) {
         continue;
       }
 
+      if (suggestion.entry.tail && suggestion.entry.tailOffsetIndex < 0) {
+        Cu.reportError(
+          `Error in tail suggestion parsing. Value: ${suggestion.entry.value}, tail: ${suggestion.entry.tail}.`
+        );
+        continue;
+      }
+
+      let tail, tailPrefix;
+      if (UrlbarPrefs.get("richSuggestions.tail")) {
+        tail = suggestion.entry.tail;
+        tailPrefix = suggestion.entry.matchPrefix;
+      }
+
       try {
         results.push(
           new UrlbarResult(
             UrlbarUtils.RESULT_TYPE.SEARCH,
             UrlbarUtils.RESULT_SOURCE.SEARCH,
             ...UrlbarResult.payloadAndSimpleHighlights(queryContext.tokens, {
               engine: [engine.name, UrlbarUtils.HIGHLIGHT.TYPED],
               suggestion: [
                 suggestion.entry.value,
                 UrlbarUtils.HIGHLIGHT.SUGGESTED,
               ],
-              tail: [
-                UrlbarPrefs.get("richSuggestions.tail") &&
-                suggestion.entry.matchPrefix &&
-                suggestion.entry.tail
-                  ? suggestion.entry.matchPrefix + suggestion.entry.tail
-                  : undefined,
-                UrlbarUtils.HIGHLIGHT.SUGGESTED,
-              ],
+              tailPrefix,
+              tail: [tail, UrlbarUtils.HIGHLIGHT.SUGGESTED],
+              tailOffsetIndex: suggestion.entry.tailOffsetIndex,
               keyword: [alias ? alias : undefined, UrlbarUtils.HIGHLIGHT.TYPED],
               query: [searchString.trim(), UrlbarUtils.HIGHLIGHT.NONE],
               isSearchHistory: false,
               icon: [
                 engine.iconURI && !suggestion.entry.value
                   ? engine.iconURI.spec
                   : "",
               ],
--- a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
+++ b/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
@@ -204,17 +204,19 @@ function makeUrlbarResult(tokens, info) 
           UrlbarUtils.RESULT_SOURCE.SEARCH,
           ...UrlbarResult.payloadAndSimpleHighlights(tokens, {
             engine: [action.params.engineName, UrlbarUtils.HIGHLIGHT.TYPED],
             suggestion: [
               action.params.searchSuggestion,
               UrlbarUtils.HIGHLIGHT.SUGGESTED,
             ],
             // For test interoperabilty with UrlbarProviderSearchSuggestions.
+            tailPrefix: undefined,
             tail: undefined,
+            tailOffsetIndex: -1,
             keyword: [action.params.alias, UrlbarUtils.HIGHLIGHT.TYPED],
             query: [
               action.params.searchQuery.trim(),
               UrlbarUtils.HIGHLIGHT.NONE,
             ],
             isSearchHistory: !!action.params.isSearchHistory,
             icon: [info.icon],
             keywordOffer,
--- a/browser/components/urlbar/UrlbarResult.jsm
+++ b/browser/components/urlbar/UrlbarResult.jsm
@@ -123,17 +123,17 @@ class UrlbarResult {
           : [this.payload.url || "", this.payloadHighlights.url || []];
       case UrlbarUtils.RESULT_TYPE.SEARCH:
         switch (this.payload.keywordOffer) {
           case UrlbarUtils.KEYWORD_OFFER.SHOW:
             return [this.payload.keyword, this.payloadHighlights.keyword];
           case UrlbarUtils.KEYWORD_OFFER.HIDE:
             return ["", []];
         }
-        if (this.payload.tail) {
+        if (this.payload.tail && this.payload.tailOffsetIndex >= 0) {
           return [this.payload.tail, this.payloadHighlights.tail];
         } else if (this.payload.suggestion) {
           return [this.payload.suggestion, this.payloadHighlights.suggestion];
         }
         return [this.payload.query, this.payloadHighlights.query];
       default:
         return ["", []];
     }
--- a/browser/components/urlbar/UrlbarUtils.jsm
+++ b/browser/components/urlbar/UrlbarUtils.jsm
@@ -640,16 +640,22 @@ UrlbarUtils.RESULT_PAYLOAD_SCHEMA = {
         type: "boolean",
       },
       suggestion: {
         type: "string",
       },
       tail: {
         type: "string",
       },
+      tailPrefix: {
+        type: "string",
+      },
+      tailOffsetIndex: {
+        type: "number",
+      },
       title: {
         type: "string",
       },
       url: {
         type: "string",
       },
     },
   },
--- a/browser/components/urlbar/UrlbarView.jsm
+++ b/browser/components/urlbar/UrlbarView.jsm
@@ -754,16 +754,34 @@ class UrlbarView {
     favicon.className = "urlbarView-favicon";
     noWrap.appendChild(favicon);
     item._elements.set("favicon", favicon);
 
     let typeIcon = this._createElement("span");
     typeIcon.className = "urlbarView-type-icon";
     noWrap.appendChild(typeIcon);
 
+    let tailPrefix = this._createElement("span");
+    tailPrefix.className = "urlbarView-tail-prefix";
+    noWrap.appendChild(tailPrefix);
+    item._elements.set("tailPrefix", tailPrefix);
+    // tailPrefix holds text only for alignment purposes so it should never be
+    // read to screen readers.
+    tailPrefix.toggleAttribute("aria-hidden", true);
+
+    let tailPrefixStr = this._createElement("span");
+    tailPrefixStr.className = "urlbarView-tail-prefix-string";
+    tailPrefix.appendChild(tailPrefixStr);
+    item._elements.set("tailPrefixStr", tailPrefixStr);
+
+    let tailPrefixChar = this._createElement("span");
+    tailPrefixChar.className = "urlbarView-tail-prefix-char";
+    tailPrefix.appendChild(tailPrefixChar);
+    item._elements.set("tailPrefixChar", tailPrefixChar);
+
     let title = this._createElement("span");
     title.className = "urlbarView-title";
     noWrap.appendChild(title);
     item._elements.set("title", title);
 
     let tagsContainer = this._createElement("span");
     tagsContainer.className = "urlbarView-tags";
     noWrap.appendChild(tagsContainer);
@@ -891,16 +909,26 @@ class UrlbarView {
     }
 
     let title = item._elements.get("title");
     this._addTextContentWithHighlights(
       title,
       result.title,
       result.titleHighlights
     );
+
+    if (result.payload.tail && result.payload.tailOffsetIndex >= 0) {
+      this._fillTailSuggestionPrefix(item, result);
+      title.setAttribute("aria-label", result.payload.suggestion);
+      item.toggleAttribute("tail-suggestion", true);
+    } else {
+      item.removeAttribute("tail-suggestion");
+      title.removeAttribute("aria-label");
+    }
+
     title._tooltip = result.title;
     if (title.hasAttribute("overflow")) {
       title.setAttribute("title", title._tooltip);
     }
 
     let tagsContainer = item._elements.get("tagsContainer");
     tagsContainer.textContent = "";
     if (result.payload.tags && result.payload.tags.length) {
@@ -1320,16 +1348,35 @@ class UrlbarView {
           highlightIndex + highlightLength
         );
         parentNode.appendChild(strong);
       }
       index = highlightIndex + highlightLength;
     }
   }
 
+  /**
+   * Adds markup for a tail suggestion prefix to a row.
+   * @param {Node} item
+   *   The node for the result row.
+   * @param {UrlbarResult} result
+   *   A UrlbarResult representing a tail suggestion.
+   */
+  _fillTailSuggestionPrefix(item, result) {
+    let tailPrefixStrNode = item._elements.get("tailPrefixStr");
+    let tailPrefixStr = result.payload.suggestion.substring(
+      0,
+      result.payload.tailOffsetIndex
+    );
+    tailPrefixStrNode.textContent = tailPrefixStr;
+
+    let tailPrefixCharNode = item._elements.get("tailPrefixChar");
+    tailPrefixCharNode.textContent = result.payload.tailPrefix;
+  }
+
   _enableOrDisableOneOffSearches(enable = true) {
     if (enable) {
       this.oneOffSearchButtons.telemetryOrigin = "urlbar";
       this.oneOffSearchButtons.style.display = "";
       this.oneOffSearchButtons.textbox = this.input.inputField;
       this.oneOffSearchButtons.view = this;
     } else {
       this.oneOffSearchButtons.telemetryOrigin = null;
--- a/browser/components/urlbar/tests/unit/head.js
+++ b/browser/components/urlbar/tests/unit/head.js
@@ -298,17 +298,19 @@ async function addTestTailSuggestionsEng
  * @param {number} [options.keywordOffer]
  *   A value from UrlbarUtils.KEYWORD_OFFER.
  * @returns {UrlbarResult}
  */
 function makeSearchResult(
   queryContext,
   {
     suggestion,
+    tailPrefix,
     tail,
+    tailOffsetIndex,
     engineName,
     alias,
     query,
     engineIconUri,
     heuristic = false,
     keywordOffer,
   }
 ) {
@@ -318,23 +320,34 @@ function makeSearchResult(
     keywordOffer = UrlbarUtils.KEYWORD_OFFER.NONE;
     if (alias && !query.trim() && alias.startsWith("@")) {
       keywordOffer = heuristic
         ? UrlbarUtils.KEYWORD_OFFER.HIDE
         : UrlbarUtils.KEYWORD_OFFER.SHOW;
     }
   }
 
+  // Tail suggestion common cases, handled here to reduce verbosity in tests.
+  if (tail && !tailPrefix) {
+    tailPrefix = "… ";
+  }
+
+  if (!tailOffsetIndex) {
+    tailOffsetIndex = tail ? suggestion.indexOf(tail) : -1;
+  }
+
   let result = new UrlbarResult(
     UrlbarUtils.RESULT_TYPE.SEARCH,
     UrlbarUtils.RESULT_SOURCE.SEARCH,
     ...UrlbarResult.payloadAndSimpleHighlights(queryContext.tokens, {
       engine: [engineName, UrlbarUtils.HIGHLIGHT.TYPED],
       suggestion: [suggestion, UrlbarUtils.HIGHLIGHT.SUGGESTED],
+      tailPrefix,
       tail: [tail, UrlbarUtils.HIGHLIGHT.SUGGESTED],
+      tailOffsetIndex,
       keyword: [alias, UrlbarUtils.HIGHLIGHT.TYPED],
       // Check against undefined so consumers can pass in the empty string.
       query: [
         typeof query != "undefined" ? query : queryContext.searchString.trim(),
         UrlbarUtils.HIGHLIGHT.TYPED,
       ],
       isSearchHistory: false,
       icon: [engineIconUri ? engineIconUri : ""],
--- a/browser/components/urlbar/tests/unit/test_search_suggestions_tail.js
+++ b/browser/components/urlbar/tests/unit/test_search_suggestions_tail.js
@@ -131,22 +131,22 @@ add_task(async function basic_tail() {
   let context = createContext(query, { isPrivate: false });
   await check_results({
     context,
     matches: [
       makeSearchResult(context, { engineName: ENGINE_NAME, heuristic: true }),
       makeSearchResult(context, {
         engineName: ENGINE_NAME,
         suggestion: query + "oronto",
-        tail: "… toronto",
+        tail: "toronto",
       }),
       makeSearchResult(context, {
         engineName: ENGINE_NAME,
         suggestion: query + "unisia",
-        tail: "… tunisia",
+        tail: "tunisia",
       }),
     ],
   });
   await cleanUpSuggestions();
 });
 
 /**
  * Tests a suggestions provider that returns both normal and tail suggestions.
@@ -183,22 +183,22 @@ add_task(async function mixed_results() 
       makeSearchResult(context, {
         engineName: ENGINE_NAME,
         suggestion: "what is the time today texas",
         tail: undefined,
       }),
       makeSearchResult(context, {
         engineName: ENGINE_NAME,
         suggestion: query + "oronto",
-        tail: "… toronto",
+        tail: "toronto",
       }),
       makeSearchResult(context, {
         engineName: ENGINE_NAME,
         suggestion: query + "unisia",
-        tail: "… tunisia",
+        tail: "tunisia",
       }),
     ],
   });
   await cleanUpSuggestions();
 });
 
 /**
  * Tests that tail suggestions are deduped if their full-text form is a dupe of
@@ -217,17 +217,17 @@ add_task(async function dedupe_local() {
       makeSearchResult(context, {
         engineName: ENGINE_NAME,
         suggestion: query + "oronto",
         tail: undefined,
       }),
       makeSearchResult(context, {
         engineName: ENGINE_NAME,
         suggestion: query + "unisia",
-        tail: "… tunisia",
+        tail: "tunisia",
       }),
     ],
   });
 
   Services.prefs.clearUserPref("browser.urlbar.maxHistoricalSearchSuggestions");
   await cleanUpSuggestions();
 });
 
@@ -241,17 +241,17 @@ add_task(async function limit_results() 
   context.maxResults = 2;
   await check_results({
     context,
     matches: [
       makeSearchResult(context, { engineName: ENGINE_NAME, heuristic: true }),
       makeSearchResult(context, {
         engineName: ENGINE_NAME,
         suggestion: query + "oronto",
-        tail: "… toronto",
+        tail: "toronto",
       }),
     ],
   });
   await cleanUpSuggestions();
 });
 
 function updateSearchHistory(op, value) {
   return new Promise((resolve, reject) => {
--- a/browser/themes/shared/urlbarView.inc.css
+++ b/browser/themes/shared/urlbarView.inc.css
@@ -360,16 +360,35 @@
      tip buttons wrap around under it.  We want the icon and title to remain on
      one line.  So make the title's flex-basis the width of the parent minus the
      width of the icon. */
   flex-basis: calc(100% - /* .urlbarView-row-inner padding-inline-start */ 6px - /* .urlbarView-favicon width */ 24px - /* .urlbarView-favicon margin-inline-end */ 12px);
   flex-grow: 1;
   flex-shrink: 1;
 }
 
+/* Tail suggestions */
+.urlbarView-tail-prefix {
+  display: none;
+  justify-content: flex-end;
+  white-space: pre;
+}
+
+.urlbarView-row[tail-suggestion] > .urlbarView-row-inner > .urlbarView-no-wrap > .urlbarView-tail-prefix {
+  display: inline-flex;
+}
+
+.urlbarView-tail-prefix > .urlbarView-tail-prefix-string {
+  visibility: hidden;
+}
+
+.urlbarView-tail-prefix > .urlbarView-tail-prefix-char {
+  position: absolute;
+}
+
 /* Title separator */
 
 .urlbarView-title-separator::before {
   content: "\2014";
   margin: 0 .4em;
   opacity: 0.6;
 }
 
--- a/toolkit/components/search/SearchSuggestionController.jsm
+++ b/toolkit/components/search/SearchSuggestionController.jsm
@@ -88,16 +88,37 @@ class SearchSuggestionEntry {
 
   get matchPrefix() {
     return this._matchPrefix;
   }
 
   get tail() {
     return this._tail;
   }
+
+  get tailOffsetIndex() {
+    if (!this._tail) {
+      return -1;
+    }
+
+    let offsetIndex = this._value.lastIndexOf(this._tail);
+    if (offsetIndex + this._tail.length < this._value.length) {
+      // We might have a tail suggestion that starts with a word contained in
+      // the full-text suggestion. e.g. "london sights in l" ... "london".
+      let lastWordIndex = this._value.lastIndexOf(" ");
+      if (this._tail.startsWith(this._value.substring(lastWordIndex))) {
+        offsetIndex = lastWordIndex;
+      } else {
+        // Something's gone wrong. Consumers should not show this result.
+        offsetIndex = -1;
+      }
+    }
+
+    return offsetIndex;
+  }
 }
 
 // Maps each engine name to a unique firstPartyDomain, so that requests to
 // different engines are isolated from each other and from normal browsing.
 // This is the same for all the controllers.
 var gFirstPartyDomains = new Map();
 
 /**
--- a/toolkit/components/search/tests/xpcshell/test_searchSuggest.js
+++ b/toolkit/components/search/tests/xpcshell/test_searchSuggest.js
@@ -342,16 +342,28 @@ add_task(async function empty_rich_resul
   Assert.equal(result.remote[1].value, "richempty query tail 1");
   Assert.ok(!result.remote[1].matchPrefix);
   Assert.ok(!result.remote[1].tail);
   Assert.equal(result.remote[2].value, "richempty query tail 2");
   Assert.ok(!result.remote[2].matchPrefix);
   Assert.ok(!result.remote[2].tail);
 });
 
+add_task(async function tail_offset_index() {
+  let controller = new SearchSuggestionController();
+  let result = await controller.fetch("tail tail 1 t", false, getEngine);
+  Assert.equal(result.term, "tail tail 1 t");
+  Assert.equal(result.local.length, 0);
+  Assert.equal(result.remote.length, 3);
+  Assert.equal(result.remote[1].value, "tail tail 1 t tail 1");
+  Assert.equal(result.remote[1].matchPrefix, "… ");
+  Assert.equal(result.remote[1].tail, "tail 1");
+  Assert.equal(result.remote[1].tailOffsetIndex, 14);
+});
+
 add_task(async function fetch_twice_in_a_row() {
   // Two entries since the first will match the first fetch but not the second.
   await updateSearchHistory("bump", "delay local");
   await updateSearchHistory("bump", "delayed local");
 
   let controller = new SearchSuggestionController();
   let resultPromise1 = controller.fetch("delay", false, getEngine);