From 2a055a380d641c3cf5bc2b03f07d96c97fb66967 Mon Sep 17 00:00:00 2001 From: Harry Twyford Date: Tue, 19 May 2020 13:57:54 +0000 Subject: [PATCH] Bug 1626891 - Style tail suggestions differently. r=mak Differential Revision: https://phabricator.services.mozilla.com/D74740 --- .../test/xpcshell/test_ext_urlbar.js | 2 + .../UrlbarProviderSearchSuggestions.jsm | 24 ++++++---- .../urlbar/UrlbarProviderUnifiedComplete.jsm | 2 + browser/components/urlbar/UrlbarResult.jsm | 2 +- browser/components/urlbar/UrlbarUtils.jsm | 6 +++ browser/components/urlbar/UrlbarView.jsm | 47 +++++++++++++++++++ browser/components/urlbar/tests/unit/head.js | 13 +++++ .../unit/test_search_suggestions_tail.js | 12 ++--- browser/themes/shared/urlbarView.inc.css | 19 ++++++++ .../search/SearchSuggestionController.jsm | 21 +++++++++ .../tests/xpcshell/test_searchSuggest.js | 12 +++++ 11 files changed, 145 insertions(+), 15 deletions(-) diff --git a/browser/components/extensions/test/xpcshell/test_ext_urlbar.js b/browser/components/extensions/test/xpcshell/test_ext_urlbar.js index 83bb705786e9..9d2dcd1d8a05 100644 --- a/browser/components/extensions/test/xpcshell/test_ext_urlbar.js +++ b/browser/components/extensions/test/xpcshell/test_ext_urlbar.js @@ -286,7 +286,9 @@ add_task(async function test_onProviderResultsRequested() { query: "test", engine: "Test engine", suggestion: undefined, + tailPrefix: undefined, tail: undefined, + tailOffsetIndex: -1, keyword: undefined, isSearchHistory: false, icon: "", diff --git a/browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm b/browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm index 5b02e695890d..eb8ec8f64c82 100644 --- a/browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm +++ b/browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm @@ -369,6 +369,19 @@ class ProviderSearchSuggestions extends UrlbarProvider { 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( @@ -380,14 +393,9 @@ class ProviderSearchSuggestions extends UrlbarProvider { 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, diff --git a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm b/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm index e23df550b9dc..b980a164ce32 100644 --- a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm +++ b/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm @@ -209,7 +209,9 @@ function makeUrlbarResult(tokens, info) { 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(), diff --git a/browser/components/urlbar/UrlbarResult.jsm b/browser/components/urlbar/UrlbarResult.jsm index 8c73d4e13532..58a0ece6ec25 100644 --- a/browser/components/urlbar/UrlbarResult.jsm +++ b/browser/components/urlbar/UrlbarResult.jsm @@ -128,7 +128,7 @@ class UrlbarResult { 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]; diff --git a/browser/components/urlbar/UrlbarUtils.jsm b/browser/components/urlbar/UrlbarUtils.jsm index a8932ce505e7..1090018d4c87 100644 --- a/browser/components/urlbar/UrlbarUtils.jsm +++ b/browser/components/urlbar/UrlbarUtils.jsm @@ -645,6 +645,12 @@ UrlbarUtils.RESULT_PAYLOAD_SCHEMA = { tail: { type: "string", }, + tailPrefix: { + type: "string", + }, + tailOffsetIndex: { + type: "number", + }, title: { type: "string", }, diff --git a/browser/components/urlbar/UrlbarView.jsm b/browser/components/urlbar/UrlbarView.jsm index 783438ff2393..49834e163155 100644 --- a/browser/components/urlbar/UrlbarView.jsm +++ b/browser/components/urlbar/UrlbarView.jsm @@ -759,6 +759,24 @@ class UrlbarView { 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); @@ -896,6 +914,16 @@ class UrlbarView { 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); @@ -1325,6 +1353,25 @@ class UrlbarView { } } + /** + * 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"; diff --git a/browser/components/urlbar/tests/unit/head.js b/browser/components/urlbar/tests/unit/head.js index 1377d27bde9f..cfcb82ef8a39 100644 --- a/browser/components/urlbar/tests/unit/head.js +++ b/browser/components/urlbar/tests/unit/head.js @@ -303,7 +303,9 @@ function makeSearchResult( queryContext, { suggestion, + tailPrefix, tail, + tailOffsetIndex, engineName, alias, query, @@ -323,13 +325,24 @@ function makeSearchResult( } } + // 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: [ diff --git a/browser/components/urlbar/tests/unit/test_search_suggestions_tail.js b/browser/components/urlbar/tests/unit/test_search_suggestions_tail.js index e48edd321375..ef1a4d552596 100644 --- a/browser/components/urlbar/tests/unit/test_search_suggestions_tail.js +++ b/browser/components/urlbar/tests/unit/test_search_suggestions_tail.js @@ -136,12 +136,12 @@ add_task(async function basic_tail() { makeSearchResult(context, { engineName: ENGINE_NAME, suggestion: query + "oronto", - tail: "… toronto", + tail: "toronto", }), makeSearchResult(context, { engineName: ENGINE_NAME, suggestion: query + "unisia", - tail: "… tunisia", + tail: "tunisia", }), ], }); @@ -188,12 +188,12 @@ add_task(async function mixed_results() { makeSearchResult(context, { engineName: ENGINE_NAME, suggestion: query + "oronto", - tail: "… toronto", + tail: "toronto", }), makeSearchResult(context, { engineName: ENGINE_NAME, suggestion: query + "unisia", - tail: "… tunisia", + tail: "tunisia", }), ], }); @@ -222,7 +222,7 @@ add_task(async function dedupe_local() { makeSearchResult(context, { engineName: ENGINE_NAME, suggestion: query + "unisia", - tail: "… tunisia", + tail: "tunisia", }), ], }); @@ -246,7 +246,7 @@ add_task(async function limit_results() { makeSearchResult(context, { engineName: ENGINE_NAME, suggestion: query + "oronto", - tail: "… toronto", + tail: "toronto", }), ], }); diff --git a/browser/themes/shared/urlbarView.inc.css b/browser/themes/shared/urlbarView.inc.css index 13a4595859f8..d115b87fce36 100644 --- a/browser/themes/shared/urlbarView.inc.css +++ b/browser/themes/shared/urlbarView.inc.css @@ -365,6 +365,25 @@ 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 { diff --git a/toolkit/components/search/SearchSuggestionController.jsm b/toolkit/components/search/SearchSuggestionController.jsm index c111f9c72987..f59467c01719 100644 --- a/toolkit/components/search/SearchSuggestionController.jsm +++ b/toolkit/components/search/SearchSuggestionController.jsm @@ -93,6 +93,27 @@ class SearchSuggestionEntry { 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 diff --git a/toolkit/components/search/tests/xpcshell/test_searchSuggest.js b/toolkit/components/search/tests/xpcshell/test_searchSuggest.js index 6edd342a9d8d..d55f9d9707d9 100644 --- a/toolkit/components/search/tests/xpcshell/test_searchSuggest.js +++ b/toolkit/components/search/tests/xpcshell/test_searchSuggest.js @@ -347,6 +347,18 @@ add_task(async function empty_rich_results() { 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");