mirror of
https://github.com/mozilla/gecko-dev.git
synced 2024-10-13 05:15:45 +00:00
Bug 1500516 - Search aliases: @engine text should remain in the urlbar when highlighting search suggestion results, and modified suggestions should search with the @ engine r=mak
* Slightly rework the logic that makes `searchSuggestionsCompletePromise` so that it checks `this.hasBehavior("searches")` and `this._inPrivateWindow` earlier so that it can avoid getting the query string and truncating it (along with the pref accesss)
* Get rid of the `input` param to `_addSearchEngineMatch`. It's only used for forcing a trailing space for alias results that don't have a query, but `_addSearchEngineMatch` can detect that case on its own -- no need for an `input` param.
* A slightly unrelated change: I noticed that when the spec shows a search for "@amazon telescopes", the first suggestion is not "telescopes", like it actually is in Firefox, but "telescopes for adults". That makes sense. There's no point in having the first suggestion echo back the heuristic result. It's better not to because (1) there's no visual dupe and (2) you don't have to press the down arrow key twice to get to non-dupe suggestions. So I added some logic to the suggestions fetching to ignore suggestions that are duplicates of the search string. I changed `_searchEngineAliasMatch` to `_searchEngineHeuristicMatch` because of course you can do searches without using an alias, and this new logic needs the query string in that case.
* Slightly rework `_addSearchEngineMatch` to be a little more straightforward
* Fix `head_autocomplete.js` so it intelligently compares moz-action results instead of a simple string comparison (and hope that the object is stringified the same way)
Differential Revision: https://phabricator.services.mozilla.com/D9472
--HG--
extra : moz-landing-system : lando
This commit is contained in:
Drew Willcoxon
parent
8e116ab9bc
commit
5292e4e7aa
@ -22,9 +22,18 @@ add_task(async function() {
|
||||
await promiseAutocompleteResultPopup("open a search");
|
||||
let result = await waitForAutocompleteResultAt(0);
|
||||
isnot(result, null, "Should have a result");
|
||||
is(result.getAttribute("url"),
|
||||
`moz-action:searchengine,{"engineName":"MozSearch","input":"open%20a%20search","searchQuery":"open%20a%20search"}`,
|
||||
"Result should be a moz-action: for the correct search engine");
|
||||
Assert.deepEqual(
|
||||
PlacesUtils.parseActionUrl(result.getAttribute("url")),
|
||||
{
|
||||
type: "searchengine",
|
||||
params: {
|
||||
engineName: "MozSearch",
|
||||
input: "open a search",
|
||||
searchQuery: "open a search",
|
||||
},
|
||||
},
|
||||
"Result should be a moz-action: for the correct search engine"
|
||||
);
|
||||
is(result.hasAttribute("image"), false, "Result shouldn't have an image attribute");
|
||||
|
||||
let tabPromise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
|
||||
|
||||
@ -106,9 +106,18 @@ add_task(async function() {
|
||||
// Check the heuristic result.
|
||||
let result = promiseValues[2];
|
||||
let engineName = Services.search.currentEngine.name;
|
||||
Assert.equal(result.url,
|
||||
`moz-action:searchengine,{"engineName":"${engineName}","input":"test","searchQuery":"test"}`,
|
||||
"result.url");
|
||||
Assert.deepEqual(
|
||||
PlacesUtils.parseActionUrl(result.url),
|
||||
{
|
||||
type: "searchengine",
|
||||
params: {
|
||||
engineName,
|
||||
input: "test",
|
||||
searchQuery: "test",
|
||||
},
|
||||
},
|
||||
"result.url"
|
||||
);
|
||||
Assert.ok("action" in result, "result.action");
|
||||
Assert.equal(result.action.type, "searchengine", "result.action.type");
|
||||
Assert.ok("params" in result.action, "result.action.params");
|
||||
|
||||
@ -51,8 +51,15 @@ async function promiseTestResult(test) {
|
||||
is(result.getAttribute("type"), test.resultListType,
|
||||
`Autocomplete result should have searchengine for the type for search '${test.search}'`);
|
||||
|
||||
is(gURLBar.mController.getFinalCompleteValueAt(0), test.finalCompleteValue,
|
||||
`Autocomplete item should go to the expected final value for search '${test.search}'`);
|
||||
let actualValue = gURLBar.mController.getFinalCompleteValueAt(0);
|
||||
let actualAction = PlacesUtils.parseActionUrl(actualValue);
|
||||
let expectedAction = PlacesUtils.parseActionUrl(test.finalCompleteValue);
|
||||
Assert.equal(!!actualAction, !!expectedAction);
|
||||
if (actualAction) {
|
||||
Assert.deepEqual(actualAction, expectedAction);
|
||||
} else {
|
||||
Assert.equal(actualValue, test.finalCompleteValue);
|
||||
}
|
||||
}
|
||||
|
||||
const tests = [{
|
||||
|
||||
@ -330,6 +330,7 @@ XPCOMUtils.defineLazyModuleGetters(this, {
|
||||
AboutPagesUtils: "resource://gre/modules/AboutPagesUtils.jsm",
|
||||
BrowserUtils: "resource://gre/modules/BrowserUtils.jsm",
|
||||
ExtensionSearchHandler: "resource://gre/modules/ExtensionSearchHandler.jsm",
|
||||
ObjectUtils: "resource://gre/modules/ObjectUtils.jsm",
|
||||
OS: "resource://gre/modules/osfile.jsm",
|
||||
PlacesRemoteTabsAutocompleteProvider: "resource://gre/modules/PlacesRemoteTabsAutocompleteProvider.jsm",
|
||||
PlacesSearchAutocompleteProvider: "resource://gre/modules/PlacesSearchAutocompleteProvider.jsm",
|
||||
@ -474,23 +475,47 @@ function stripHttpAndTrim(spec, trimSlash = true) {
|
||||
|
||||
/**
|
||||
* Returns the key to be used for a match in a map for the purposes of removing
|
||||
* duplicate entries - any 2 URLs that should be considered the same should
|
||||
* return the same key. For some moz-action URLs this will unwrap the params
|
||||
* and return a key based on the wrapped URL.
|
||||
* duplicate entries - any 2 matches that should be considered the same should
|
||||
* return the same key. The type of the returned key depends on the type of the
|
||||
* match, so don't assume you can compare keys using ==. Instead, use
|
||||
* ObjectUtils.deepEqual().
|
||||
*
|
||||
* @param {object} match
|
||||
* The match object.
|
||||
* @returns {value} Some opaque key object. Use ObjectUtils.deepEqual() to
|
||||
* compare keys.
|
||||
*/
|
||||
function makeKeyForURL(match) {
|
||||
let url = match.value;
|
||||
let action = PlacesUtils.parseActionUrl(url);
|
||||
// At this stage we only consider moz-action URLs.
|
||||
if (!action || !("url" in action.params)) {
|
||||
function makeKeyForMatch(match) {
|
||||
// For autofill entries, we need to have a key based on the comment rather
|
||||
// than the value field, because the latter may have been trimmed.
|
||||
if (match.hasOwnProperty("style") && match.style.includes("autofill")) {
|
||||
url = match.comment;
|
||||
return [stripHttpAndTrim(match.comment), null];
|
||||
}
|
||||
return [stripHttpAndTrim(url), null];
|
||||
|
||||
let action = PlacesUtils.parseActionUrl(match.value);
|
||||
if (!action) {
|
||||
return [stripHttpAndTrim(match.value), null];
|
||||
}
|
||||
return [stripHttpAndTrim(action.params.url), action];
|
||||
|
||||
let key;
|
||||
switch (action.type) {
|
||||
case "searchengine":
|
||||
// We want to exclude search suggestion matches that simply echo back the
|
||||
// query string in the heuristic result. For example, if the user types
|
||||
// "@engine test", we want to exclude a "test" suggestion match.
|
||||
key = [
|
||||
action.type,
|
||||
action.params.engineName,
|
||||
(action.params.searchSuggestion || action.params.searchQuery)
|
||||
.toLocaleLowerCase(),
|
||||
];
|
||||
break;
|
||||
default:
|
||||
key = stripHttpAndTrim(action.params.url || match.value);
|
||||
break;
|
||||
}
|
||||
|
||||
return [key, action];
|
||||
}
|
||||
|
||||
/**
|
||||
@ -806,7 +831,7 @@ Search.prototype = {
|
||||
// If the query is simply "@", then the results should be a list of all the
|
||||
// search engines with "@" aliases, without a hueristic result.
|
||||
if (this._trimmedOriginalSearchString == "@") {
|
||||
let added = await this._addSearchEngineTokenAliasResults();
|
||||
let added = await this._addSearchEngineTokenAliasMatches();
|
||||
if (added) {
|
||||
this._cleanUpNonCurrentMatches(null);
|
||||
this._autocompleteSearch.finishSearch(true);
|
||||
@ -858,19 +883,24 @@ Search.prototype = {
|
||||
ExtensionSearchHandler.handleInputCancelled();
|
||||
}
|
||||
|
||||
// Start adding search suggestions, unless they aren't required or the
|
||||
// window is private.
|
||||
let searchSuggestionsCompletePromise = Promise.resolve();
|
||||
if (this._enableActions) {
|
||||
if (this._enableActions &&
|
||||
this.hasBehavior("search") &&
|
||||
!this._inPrivateWindow) {
|
||||
// Get the query string stripped of any engine alias or restriction token.
|
||||
// In the former case, _searchTokens[0] will be the alias, so use
|
||||
// this._searchEngineHeuristicMatch.query. In the latter case, the token
|
||||
// has been removed from _searchTokens, so use _searchTokens.join().
|
||||
let query =
|
||||
this._searchEngineAliasMatch ? this._searchEngineAliasMatch.query :
|
||||
this._searchTokens.join(" ");
|
||||
if (query) {
|
||||
// Limit the string sent for search suggestions to a maximum length.
|
||||
query = query.substr(0, UrlbarPrefs.get("maxCharsForSearchSuggestions"));
|
||||
// Avoid fetching suggestions if they are not required, private browsing
|
||||
// mode is enabled, or the query may expose sensitive information.
|
||||
if (this.hasBehavior("search") &&
|
||||
!this._inPrivateWindow &&
|
||||
!this._prohibitSearchSuggestionsFor(query)) {
|
||||
// Don't add suggestions if the query may expose sensitive information.
|
||||
if (!this._prohibitSearchSuggestionsFor(query)) {
|
||||
let engine;
|
||||
if (this._searchEngineAliasMatch) {
|
||||
engine = this._searchEngineAliasMatch.engine;
|
||||
@ -880,12 +910,16 @@ Search.prototype = {
|
||||
return;
|
||||
}
|
||||
}
|
||||
let alias =
|
||||
this._searchEngineAliasMatch &&
|
||||
this._searchEngineAliasMatch.alias ||
|
||||
"";
|
||||
searchSuggestionsCompletePromise =
|
||||
this._matchSearchSuggestions(engine, query);
|
||||
this._matchSearchSuggestions(engine, query, alias);
|
||||
// If the user has used a search engine alias, then the only results
|
||||
// we want to show are suggestions from that engine, so we're done.
|
||||
// We're also done if we're restricting results to suggestions.
|
||||
if (this._searchEngineAliasMatch || this.hasBehavior("restrict")) {
|
||||
if (alias || this.hasBehavior("restrict")) {
|
||||
// Wait for the suggestions to be added.
|
||||
await searchSuggestionsCompletePromise;
|
||||
this._cleanUpNonCurrentMatches(null);
|
||||
@ -1079,20 +1113,15 @@ Search.prototype = {
|
||||
*
|
||||
* @returns {bool} True if any results were added, false if not.
|
||||
*/
|
||||
async _addSearchEngineTokenAliasResults() {
|
||||
async _addSearchEngineTokenAliasMatches() {
|
||||
let engines = await PlacesSearchAutocompleteProvider.tokenAliasEngines();
|
||||
if (!engines || !engines.length) {
|
||||
return false;
|
||||
}
|
||||
for (let { engine, tokenAliases } of engines) {
|
||||
let alias = tokenAliases[0];
|
||||
// `input` should have a trailing space so that when the user selects the
|
||||
// result, they can start typing their query without first having to enter
|
||||
// a space between the alias and query.
|
||||
this._addSearchEngineMatch({
|
||||
engine,
|
||||
alias,
|
||||
input: alias + " ",
|
||||
alias: tokenAliases[0],
|
||||
});
|
||||
}
|
||||
return true;
|
||||
@ -1253,7 +1282,7 @@ Search.prototype = {
|
||||
return false;
|
||||
},
|
||||
|
||||
_matchSearchSuggestions(engine, searchString) {
|
||||
_matchSearchSuggestions(engine, searchString, alias) {
|
||||
this._suggestionsFetch =
|
||||
PlacesSearchAutocompleteProvider.newSuggestionsFetch(
|
||||
engine,
|
||||
@ -1281,6 +1310,7 @@ Search.prototype = {
|
||||
if (!looksLikeUrl(suggestion)) {
|
||||
this._addSearchEngineMatch({
|
||||
engine,
|
||||
alias,
|
||||
query: searchString,
|
||||
suggestion,
|
||||
historical,
|
||||
@ -1536,40 +1566,47 @@ Search.prototype = {
|
||||
* @param {bool} [historical]
|
||||
* True if you're adding a suggestion match and the suggestion is from
|
||||
* the user's local history (and not the search engine).
|
||||
* @param {string} [input]
|
||||
* Use this value to override the action.params.input that this method
|
||||
* would otherwise compute.
|
||||
*/
|
||||
_addSearchEngineMatch({engine,
|
||||
query = "",
|
||||
alias = undefined,
|
||||
suggestion = undefined,
|
||||
historical = false,
|
||||
input = undefined}) {
|
||||
historical = false}) {
|
||||
let actionURLParams = {
|
||||
engineName: engine.name,
|
||||
input: input || suggestion || this._originalSearchString,
|
||||
searchQuery: query,
|
||||
};
|
||||
if (alias) {
|
||||
actionURLParams.alias = alias;
|
||||
}
|
||||
|
||||
if (suggestion) {
|
||||
actionURLParams.searchSuggestion = suggestion;
|
||||
// `input` should include the alias.
|
||||
actionURLParams.input = (alias ? `${alias} ` : "") + suggestion;
|
||||
} else if (alias && !query) {
|
||||
// `input` should have a trailing space so that when the user selects the
|
||||
// result, they can start typing their query without first having to enter
|
||||
// a space between the alias and query.
|
||||
actionURLParams.input = `${alias} `;
|
||||
} else {
|
||||
actionURLParams.input = this._originalSearchString;
|
||||
}
|
||||
let value = PlacesUtils.mozActionURI("searchengine", actionURLParams);
|
||||
|
||||
let match = {
|
||||
value,
|
||||
comment: engine.name,
|
||||
icon: engine.iconURI ? engine.iconURI.spec : null,
|
||||
icon: engine.iconURI && !suggestion ? engine.iconURI.spec : null,
|
||||
style: "action searchengine",
|
||||
frecency: FRECENCY_DEFAULT,
|
||||
};
|
||||
|
||||
if (alias) {
|
||||
actionURLParams.alias = alias;
|
||||
match.style += " alias";
|
||||
}
|
||||
if (suggestion) {
|
||||
actionURLParams.searchSuggestion = suggestion;
|
||||
match.style += " suggestion";
|
||||
match.type = UrlbarUtils.MATCH_GROUP.SUGGESTION;
|
||||
}
|
||||
|
||||
match.value = PlacesUtils.mozActionURI("searchengine", actionURLParams);
|
||||
this._addMatch(match);
|
||||
},
|
||||
|
||||
@ -1827,16 +1864,16 @@ Search.prototype = {
|
||||
// Note: this partially fixes Bug 1222435, but not if the urls differ more
|
||||
// than just by "http://". We should still evaluate www and other schemes
|
||||
// equivalences.
|
||||
let [urlMapKey, action] = makeKeyForURL(match);
|
||||
let [urlMapKey, action] = makeKeyForMatch(match);
|
||||
if ((match.placeId && this._usedPlaceIds.has(match.placeId)) ||
|
||||
this._usedURLs.map(e => e.key).includes(urlMapKey)) {
|
||||
this._usedURLs.some(e => ObjectUtils.deepEqual(e.key, urlMapKey))) {
|
||||
let isDupe = true;
|
||||
if (action && ["switchtab", "remotetab"].includes(action.type)) {
|
||||
// The new entry is a switch/remote tab entry, look for the duplicate
|
||||
// among current matches.
|
||||
for (let i = 0; i < this._usedURLs.length; ++i) {
|
||||
let {key: matchKey, action: matchAction, type: matchType} = this._usedURLs[i];
|
||||
if (matchKey == urlMapKey) {
|
||||
if (ObjectUtils.deepEqual(matchKey, urlMapKey)) {
|
||||
isDupe = true;
|
||||
// Don't replace the match if the existing one is heuristic and the
|
||||
// new one is a switchtab, instead also add the switchtab match.
|
||||
|
||||
@ -4,6 +4,7 @@
|
||||
|
||||
const FRECENCY_DEFAULT = 10000;
|
||||
|
||||
ChromeUtils.import("resource://gre/modules/ObjectUtils.jsm");
|
||||
ChromeUtils.import("resource://gre/modules/Services.jsm");
|
||||
ChromeUtils.import("resource://testing-common/httpd.js");
|
||||
|
||||
@ -143,7 +144,18 @@ async function _check_autocomplete_matches(match, result) {
|
||||
info(`Checking match: ` +
|
||||
`actual=${JSON.stringify(actual)} ... ` +
|
||||
`expected=${JSON.stringify(expected)}`);
|
||||
if (actual.value != expected.value || actual.comment != expected.comment) {
|
||||
|
||||
let actualAction = PlacesUtils.parseActionUrl(actual.value);
|
||||
let expectedAction = PlacesUtils.parseActionUrl(expected.value);
|
||||
if (actualAction && expectedAction) {
|
||||
if (!ObjectUtils.deepEqual(actualAction, expectedAction)) {
|
||||
return false;
|
||||
}
|
||||
} else if (actual.value != expected.value) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if (actual.comment != expected.comment) {
|
||||
return false;
|
||||
}
|
||||
|
||||
@ -363,19 +375,11 @@ function makeActionURI(action, params) {
|
||||
// Creates a full "match" entry for a search result, suitable for passing as
|
||||
// an entry to check_autocomplete.
|
||||
function makeSearchMatch(input, extra = {}) {
|
||||
// Note that counter-intuitively, the order the object properties are defined
|
||||
// in the object passed to makeActionURI is important for check_autocomplete
|
||||
// to match them :(
|
||||
let params = {
|
||||
engineName: extra.engineName || "MozSearch",
|
||||
input,
|
||||
searchQuery: "searchQuery" in extra ? extra.searchQuery : input,
|
||||
};
|
||||
if ("alias" in extra) {
|
||||
// May be undefined, which is expected, but in that case make sure it's not
|
||||
// included in the params of the moz-action URL.
|
||||
params.alias = extra.alias;
|
||||
}
|
||||
let style = [ "action", "searchengine" ];
|
||||
if ("style" in extra && Array.isArray(extra.style)) {
|
||||
style.push(...extra.style);
|
||||
@ -383,6 +387,10 @@ function makeSearchMatch(input, extra = {}) {
|
||||
if (extra.heuristic) {
|
||||
style.push("heuristic");
|
||||
}
|
||||
if ("alias" in extra) {
|
||||
params.alias = extra.alias;
|
||||
style.push("alias");
|
||||
}
|
||||
if ("searchSuggestion" in extra) {
|
||||
params.searchSuggestion = extra.searchSuggestion;
|
||||
style.push("suggestion");
|
||||
@ -489,7 +497,7 @@ async function addTestSuggestionsEngine(suggestionsFn = null) {
|
||||
let searchStr = decodeURIComponent(req.queryString.replace(/\+/g, " "));
|
||||
let suggestions =
|
||||
suggestionsFn ? suggestionsFn(searchStr) :
|
||||
["foo", "bar"].map(s => searchStr + " " + s);
|
||||
[searchStr].concat(["foo", "bar"].map(s => searchStr + " " + s));
|
||||
let data = [searchStr, suggestions];
|
||||
resp.setHeader("Content-Type", "application/json", false);
|
||||
resp.write(JSON.stringify(data));
|
||||
|
||||
@ -26,7 +26,7 @@ add_task(async function getPost() {
|
||||
search: alias,
|
||||
searchParam: "enable-actions",
|
||||
matches: [
|
||||
makeSearchMatch(alias, {
|
||||
makeSearchMatch(`${alias} `, {
|
||||
engineName: `Aliased${alias.toUpperCase()}MozSearch`,
|
||||
searchQuery: "",
|
||||
alias,
|
||||
@ -115,10 +115,10 @@ add_task(async function engineWithSuggestions() {
|
||||
engine.alias = alias;
|
||||
|
||||
await check_autocomplete({
|
||||
search: `${alias}`,
|
||||
search: alias,
|
||||
searchParam: "enable-actions",
|
||||
matches: [
|
||||
makeSearchMatch(alias, {
|
||||
makeSearchMatch(`${alias} `, {
|
||||
engineName: SUGGESTIONS_ENGINE_NAME,
|
||||
alias,
|
||||
searchQuery: "",
|
||||
@ -150,13 +150,15 @@ add_task(async function engineWithSuggestions() {
|
||||
searchQuery: "fire",
|
||||
heuristic: true,
|
||||
}),
|
||||
makeSearchMatch(`fire foo`, {
|
||||
makeSearchMatch(`${alias} fire foo`, {
|
||||
engineName: SUGGESTIONS_ENGINE_NAME,
|
||||
alias,
|
||||
searchQuery: "fire",
|
||||
searchSuggestion: "fire foo",
|
||||
}),
|
||||
makeSearchMatch(`fire bar`, {
|
||||
makeSearchMatch(`${alias} fire bar`, {
|
||||
engineName: SUGGESTIONS_ENGINE_NAME,
|
||||
alias,
|
||||
searchQuery: "fire",
|
||||
searchSuggestion: "fire bar",
|
||||
}),
|
||||
|
||||
@ -31,7 +31,7 @@ add_task(async function setup() {
|
||||
});
|
||||
setSuggestionsFn(searchStr => {
|
||||
let suffixes = ["foo", "bar"];
|
||||
return suffixes.map(s => searchStr + " " + s);
|
||||
return [searchStr].concat(suffixes.map(s => searchStr + " " + s));
|
||||
});
|
||||
|
||||
// Install the test engine.
|
||||
@ -95,6 +95,9 @@ add_task(async function singleWordQuery() {
|
||||
searchParam: "enable-actions",
|
||||
matches: [
|
||||
makeSearchMatch("hello", { engineName: ENGINE_NAME, heuristic: true }),
|
||||
// The test engine echoes back the search string as the first suggestion,
|
||||
// so it would appear here (as "hello"), but we remove suggestions that
|
||||
// duplicate the search string, so it should not actually appear.
|
||||
{ uri: makeActionURI(("searchengine"), {
|
||||
engineName: ENGINE_NAME,
|
||||
input: "hello foo",
|
||||
@ -302,6 +305,17 @@ add_task(async function restrictToken() {
|
||||
// TODO (bug 1177895) This is wrong.
|
||||
makeSearchMatch(`${UrlbarTokenizer.RESTRICT.SEARCH} hello`,
|
||||
{ engineName: ENGINE_NAME, heuristic: true }),
|
||||
{
|
||||
uri: makeActionURI(("searchengine"), {
|
||||
engineName: ENGINE_NAME,
|
||||
input: "hello",
|
||||
searchQuery: "hello",
|
||||
searchSuggestion: "hello",
|
||||
}),
|
||||
title: ENGINE_NAME,
|
||||
style: ["action", "searchengine", "suggestion"],
|
||||
icon: "",
|
||||
},
|
||||
{
|
||||
uri: makeActionURI(("searchengine"), {
|
||||
engineName: ENGINE_NAME,
|
||||
|
||||
Loading…
Reference in New Issue
Block a user