diff --git a/browser/base/content/test/urlbar/browser_urlbarHighlightSearchAlias.js b/browser/base/content/test/urlbar/browser_urlbarHighlightSearchAlias.js index 1f59e95e65d3..f9bd72feea05 100644 --- a/browser/base/content/test/urlbar/browser_urlbarHighlightSearchAlias.js +++ b/browser/base/content/test/urlbar/browser_urlbarHighlightSearchAlias.js @@ -62,6 +62,19 @@ add_task(async function charsBeforeAlias() { }); +// In a search string that starts with a restriction character followed by an +// alias, the alias should be neither recognized nor highlighted. +add_task(async function restrictionCharBeforeAlias() { + gURLBar.search(UrlbarTokenizer.RESTRICT.BOOKMARK + " " + ALIAS); + await promiseSearchComplete(); + await waitForAutocompleteResultAt(0); + assertAlias(false); + + EventUtils.synthesizeKey("KEY_Escape"); + await promisePopupHidden(gURLBar.popup); +}); + + // Aliases are case insensitive, and the alias in the result uses the case that // the user typed in the input. Make sure that searching with an alias using a // weird case still causes the alias to be highlighted. @@ -175,6 +188,26 @@ add_task(async function nonHeuristicAliases() { }); +// Aliases that don't start with @ shouldn't be highlighted. +add_task(async function nonAtMarkAlias() { + let alias = "notatmarkalias"; + let engine = Services.search.getEngineByName("Test"); + engine.alias = "notatmarkalias"; + Assert.equal(engine.alias, alias); + + gURLBar.search(alias); + await promiseSearchComplete(); + await waitForAutocompleteResultAt(0); + assertFirstResultIsAlias(true, alias); + assertHighlighted(false); + + EventUtils.synthesizeKey("KEY_Escape"); + await promisePopupHidden(gURLBar.popup); + + engine.alias = ALIAS; +}); + + async function doSimpleTest(revertBetweenSteps) { // When autofill is enabled, searching for "@tes" will autofill to "@test", // which gets in the way of this test task, so temporarily disable it. diff --git a/browser/components/urlbar/UrlbarValueFormatter.jsm b/browser/components/urlbar/UrlbarValueFormatter.jsm index 9b11b3434979..0dda8b12d69f 100644 --- a/browser/components/urlbar/UrlbarValueFormatter.jsm +++ b/browser/components/urlbar/UrlbarValueFormatter.jsm @@ -261,8 +261,7 @@ class UrlbarValueFormatter { } /** - * If the input value starts with a search alias, this formatter method - * highlights it. + * If the input value starts with an @engine search alias, this highlights it. * * @returns {boolean} * True if formatting was applied and false if not. @@ -282,12 +281,21 @@ class UrlbarValueFormatter { return false; } - // To determine whether the input contains a search engine alias, check the - // value of the selected result -- whether it's a search engine result with - // an alias. Actually, check the selected listbox item, not the result in - // the controller, because we want to continue highlighting the alias when - // the popup is closed and the search has stopped. The selected index when - // the popup is closed is zero, however, which is why we check the previous + let editor = this.urlbarInput.editor; + let textNode = editor.rootElement.firstChild; + let value = textNode.textContent; + let trimmedValue = value.trim(); + + if (!trimmedValue.startsWith("@")) { + return false; + } + + // To determine whether the input contains a valid alias, check the value of + // the selected result -- whether it's a search engine result with an alias. + // Actually, check the selected listbox item, not the result in the + // controller, because we want to continue highlighting the alias when the + // popup is closed and the search has stopped. The selected index when the + // popup is closed is zero, however, which is why we also check the previous // selected index. let itemIndex = popup.selectedIndex < 0 ? popup._previousSelectedIndex : @@ -316,10 +324,6 @@ class UrlbarValueFormatter { return false; } - let editor = this.urlbarInput.editor; - let textNode = editor.rootElement.firstChild; - let value = textNode.textContent; - // Make sure the item's input matches the current urlbar input because the // urlbar input can change without the popup results changing. Most notably // that happens when the user performs a search using an alias: The popup @@ -331,7 +335,7 @@ class UrlbarValueFormatter { // its input is "@engine ". So in order to make sure the item's input // matches the current urlbar input, we need to check that the urlbar input // starts with the item's input. - if (!value.trim().startsWith(decodeURIComponent(action.params.input).trim())) { + if (!trimmedValue.startsWith(action.params.input.trim())) { return false; } diff --git a/toolkit/components/places/UnifiedComplete.js b/toolkit/components/places/UnifiedComplete.js index e7f74f0b8228..3495f601d7be 100644 --- a/toolkit/components/places/UnifiedComplete.js +++ b/toolkit/components/places/UnifiedComplete.js @@ -527,6 +527,38 @@ function looksLikeOrigin(str) { return REGEXP_ORIGIN.test(str); } +/** + * Returns the portion of a string starting at the index where another string + * begins. + * + * @param {string} sourceStr + * The string to search within. + * @param {string} targetStr + * The string to search for. + * @returns {string} The substring within sourceStr starting at targetStr, or + * the empty string if targetStr does not occur in sourceStr. + */ +function substringAt(sourceStr, targetStr) { + let index = sourceStr.indexOf(targetStr); + return index < 0 ? "" : sourceStr.substr(index); +} + +/** + * Returns the portion of a string starting at the index where another string + * ends. + * + * @param {string} sourceStr + * The string to search within. + * @param {string} targetStr + * The string to search for. + * @returns {string} The substring within sourceStr where targetStr ends, or the + * empty string if targetStr does not occur in sourceStr. + */ +function substringAfter(sourceStr, targetStr) { + let index = sourceStr.indexOf(targetStr); + return index < 0 ? "" : sourceStr.substr(index + targetStr.length); +} + /** * Manages a single instance of an autocomplete search. * @@ -583,6 +615,20 @@ function Search(searchString, searchParam, autocompleteListener, this._searchTokens = this.filterTokens(getUnfilteredSearchTokens(this._searchString)); + + // The heuristic token is the first filtered search token, but only when it's + // actually the first thing in the search string. If a prefix or restriction + // character occurs first, then the heurstic token is null. We use the + // heuristic token to help determine the heuristic result. It may be a Places + // keyword, a search engine alias, an extension keyword, or simply a URL or + // part of the search string the user has typed. We won't know until we + // create the heuristic result. + this._heuristicToken = + this._searchTokens[0] && + this._trimmedOriginalSearchString.startsWith(this._searchTokens[0]) ? + this._searchTokens[0] : + null; + this._keywordSubstitute = null; this._prohibitSearchSuggestions = prohibitSearchSuggestions; @@ -695,11 +741,12 @@ Search.prototype = { /** * Given an array of tokens, this function determines which query should be - * ran. It also removes any special search tokens. + * ran. It also removes any special search tokens. The given array of tokens + * is modified in place and returned. * * @param tokens - * An array of search tokens. - * @return the filtered list of tokens to search with. + * An array of search tokens. This array is modified in place. + * @return The given array of tokens, modified to remove special search tokens. */ filterTokens(tokens) { let foundToken = false; @@ -861,9 +908,9 @@ Search.prototype = { // Only add extension suggestions if the first token is a registered keyword // and the search string has characters after the first token. let extensionsCompletePromise = Promise.resolve(); - if (this._searchTokens.length > 0 && - ExtensionSearchHandler.isKeywordRegistered(this._searchTokens[0]) && - this._originalSearchString.length > this._searchTokens[0].length && + if (this._heuristicToken && + ExtensionSearchHandler.isKeywordRegistered(this._heuristicToken) && + substringAfter(this._originalSearchString, this._heuristicToken) && !this._searchEngineAliasMatch) { // Do not await on this, since extensions cannot notify when they are done // adding results, it may take too long. @@ -878,13 +925,9 @@ Search.prototype = { 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(" "); + substringAt(this._originalSearchString, this._searchTokens[0]); if (query) { // Limit the string sent for search suggestions to a maximum length. query = query.substr(0, UrlbarPrefs.get("maxCharsForSearchSuggestions")); @@ -1115,16 +1158,13 @@ Search.prototype = { }, async _matchSearchEngineTokenAliasForAutofill() { - // We need a single "@engine" search token. - if (this._searchTokens.length != 1) { - return false; - } - let token = this._searchTokens[0]; - if (token[0] != "@" || token.length == 1) { + // We need an "@engine" heuristic token. + let token = this._heuristicToken; + if (!token || token.length == 1 || !token.startsWith("@")) { return false; } - // See if any engine token alias starts with the search token. + // See if any engine has a token alias that starts with the heuristic token. let engines = await PlacesSearchAutocompleteProvider.tokenAliasEngines(); for (let { engine, tokenAliases } of engines) { for (let alias of tokenAliases) { @@ -1375,9 +1415,11 @@ Search.prototype = { }, _matchExtensionHeuristicResult() { - if (ExtensionSearchHandler.isKeywordRegistered(this._searchTokens[0]) && - this._originalSearchString.length > this._searchTokens[0].length) { - let description = ExtensionSearchHandler.getDescription(this._searchTokens[0]); + if (this._heuristicToken && + ExtensionSearchHandler.isKeywordRegistered(this._heuristicToken) && + substringAfter(this._originalSearchString, this._heuristicToken)) { + let description = + ExtensionSearchHandler.getDescription(this._heuristicToken); this._addExtensionMatch(this._originalSearchString, description); return true; } @@ -1385,13 +1427,17 @@ Search.prototype = { }, async _matchPlacesKeyword() { - // The first word could be a keyword, so that's what we'll search. - let keyword = this._strippedPrefix + this._searchTokens[0]; - let entry = await PlacesUtils.keywords.fetch(keyword); - if (!entry) + if (!this._heuristicToken) { return false; + } + let keyword = this._heuristicToken; + let entry = await PlacesUtils.keywords.fetch(keyword); + if (!entry) { + return false; + } - let searchString = this._trimmedOriginalSearchString.substr(keyword.length + 1); + let searchString = + substringAfter(this._originalSearchString, keyword).trim(); let url = null, postData = null; try { @@ -1495,11 +1541,11 @@ Search.prototype = { }, async _matchSearchEngineAlias() { - if (this._searchTokens.length < 1) { + if (!this._heuristicToken) { return false; } - let alias = this._searchTokens[0]; + let alias = this._heuristicToken; let engine = await PlacesSearchAutocompleteProvider.engineForAlias(alias); if (!engine) { return false; @@ -1508,7 +1554,7 @@ Search.prototype = { this._searchEngineAliasMatch = { engine, alias, - query: this._trimmedOriginalSearchString.substr(alias.length + 1), + query: substringAfter(this._originalSearchString, alias).trim(), }; this._addSearchEngineMatch(this._searchEngineAliasMatch); if (!this._keywordSubstitute) { @@ -1539,7 +1585,7 @@ Search.prototype = { this._addMatch({ value: PlacesUtils.mozActionURI("extension", { content, - keyword: this._searchTokens[0], + keyword: this._heuristicToken, }), comment, icon: "chrome://browser/content/extension.svg", @@ -1609,10 +1655,10 @@ Search.prototype = { }, _matchExtensionSuggestions() { - let promise = ExtensionSearchHandler.handleSearch(this._searchTokens[0], this._originalSearchString, + let promise = ExtensionSearchHandler.handleSearch(this._heuristicToken, this._originalSearchString, suggestions => { for (let suggestion of suggestions) { - let content = `${this._searchTokens[0]} ${suggestion.content}`; + let content = `${this._heuristicToken} ${suggestion.content}`; this._addExtensionMatch(content, suggestion.description); } } diff --git a/toolkit/components/places/tests/unifiedcomplete/test_keyword_search.js b/toolkit/components/places/tests/unifiedcomplete/test_keyword_search.js index b6eec176698b..313460e4b044 100644 --- a/toolkit/components/places/tests/unifiedcomplete/test_keyword_search.js +++ b/toolkit/components/places/tests/unifiedcomplete/test_keyword_search.js @@ -12,7 +12,7 @@ * same keyword appear in the list. */ -add_task(async function test_keyword_searc() { +add_task(async function test_keyword_search() { let uri1 = NetUtil.newURI("http://abc/?search=%s"); let uri2 = NetUtil.newURI("http://abc/?search=ThisPageIsInHistory"); let uri3 = NetUtil.newURI("http://somedomain.example/key"); diff --git a/toolkit/components/places/tests/unifiedcomplete/test_search_engine_alias.js b/toolkit/components/places/tests/unifiedcomplete/test_search_engine_alias.js index 43ba4e377916..779156f6ea06 100644 --- a/toolkit/components/places/tests/unifiedcomplete/test_search_engine_alias.js +++ b/toolkit/components/places/tests/unifiedcomplete/test_search_engine_alias.js @@ -99,6 +99,25 @@ add_task(async function getPost() { }), ], }); + + // When a restriction token is used before the alias, the alias should *not* + // be recognized. It should be treated as part of the search string. Try + // all the restriction tokens to test that. We should get a single "search + // with" heuristic result without an alias. + for (let restrictToken in UrlbarTokenizer.RESTRICT) { + let search = `${restrictToken} ${alias} query string`; + await check_autocomplete({ + search, + searchParam: "enable-actions", + matches: [ + makeSearchMatch(search, { + engineName: "MozSearch", + searchQuery: search, + heuristic: true, + }), + ], + }); + } } await cleanup();