From eeeeea2f96a11d8ea859370dd332103d8a52b6c2 Mon Sep 17 00:00:00 2001 From: Drew Willcoxon Date: Fri, 17 Apr 2020 00:42:23 +0000 Subject: [PATCH] Bug 1618769 - Increase max chars for search suggestions, and don't fetch suggestions at all when max is reached due to paste. r=mak * Add a new `allowSearchSuggestions` property to the query context. It defaults to true. * `UrlbarInput` sets this property when it starts a query. If the event that started the query is a paste event and the pasted string's length is larger than maxChars, then don't allow suggestions. Otherwise do. * `UrlbarProviderSearchSuggestions.isActive` returns false when `!context.allowSearchSuggestions`. * `UrlbarProviderSearchSuggestions` doesn't truncate the query anymore. * Keep the `browser.urlbar.maxCharsForSearchSuggestions` pref but use it only for pastes, and increase it from 20 to 100. I considered making a new pref but this way if a user changed it, then it still applies to pastes at least. I'm not sure it's important though. Differential Revision: https://phabricator.services.mozilla.com/D70956 --- browser/app/profile/firefox.js | 6 +- browser/components/urlbar/UrlbarInput.jsm | 5 + .../UrlbarProviderSearchSuggestions.jsm | 7 +- browser/components/urlbar/UrlbarUtils.jsm | 15 +- browser/components/urlbar/docs/overview.rst | 6 + .../browser/browser_searchSuggestions.js | 142 +++++++++++++++++- .../test_UrlbarQueryContext_restrictSource.js | 9 +- 7 files changed, 171 insertions(+), 19 deletions(-) diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js index aef3faa1e6aa..280fc18dcae4 100644 --- a/browser/app/profile/firefox.js +++ b/browser/app/profile/firefox.js @@ -281,9 +281,9 @@ pref("browser.urlbar.suggest.bookmark", true); pref("browser.urlbar.suggest.openpage", true); pref("browser.urlbar.suggest.searches", true); -// Limit the number of characters sent to the current search engine to fetch -// suggestions. -pref("browser.urlbar.maxCharsForSearchSuggestions", 20); +// As a user privacy measure, don't fetch search suggestions if a pasted string +// is longer than this. +pref("browser.urlbar.maxCharsForSearchSuggestions", 100); pref("browser.urlbar.formatting.enabled", true); pref("browser.urlbar.trimURLs", true); diff --git a/browser/components/urlbar/UrlbarInput.jsm b/browser/components/urlbar/UrlbarInput.jsm index 9ee7405d1d50..28c60e6a25f5 100644 --- a/browser/components/urlbar/UrlbarInput.jsm +++ b/browser/components/urlbar/UrlbarInput.jsm @@ -935,6 +935,11 @@ class UrlbarInput { "usercontextid" ), currentPage: this.window.gBrowser.currentURI.spec, + allowSearchSuggestions: + !event || + !UrlbarUtils.isPasteEvent(event) || + !event.data || + event.data.length <= UrlbarPrefs.get("maxCharsForSearchSuggestions"), }) ); } diff --git a/browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm b/browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm index 26aef5793a31..a9c43dd532b8 100644 --- a/browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm +++ b/browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm @@ -120,6 +120,10 @@ class ProviderSearchSuggestions extends UrlbarProvider { return false; } + if (!queryContext.allowSearchSuggestions) { + return false; + } + if ( queryContext.isPrivate && !UrlbarPrefs.get("browser.search.suggest.enabled.private") @@ -252,9 +256,6 @@ class ProviderSearchSuggestions extends UrlbarProvider { query = substringAfter(query, leadingRestrictionToken).trim(); } - // Limit the string sent for search suggestions to a maximum length. - query = query.substr(0, UrlbarPrefs.get("maxCharsForSearchSuggestions")); - // Find our search engine. It may have already been set with an alias. let engine; if (aliasEngine) { diff --git a/browser/components/urlbar/UrlbarUtils.jsm b/browser/components/urlbar/UrlbarUtils.jsm index b591f52af5ca..26aba2fc4e47 100644 --- a/browser/components/urlbar/UrlbarUtils.jsm +++ b/browser/components/urlbar/UrlbarUtils.jsm @@ -810,6 +810,11 @@ class UrlbarQueryContext { * If sources is restricting to just SEARCH, this property can be used to * pick a specific search engine, by setting it to the name under which the * engine is registered with the search service. + * @param {boolean} [options.allowSearchSuggestions] + * Whether to allow search suggestions. This is a veto, meaning that when + * false, suggestions will not be fetched, but when true, some other + * condition may still prohibit suggestions, like private browsing mode. + * Defaults to true. */ constructor(options = {}) { this._checkRequiredOptions(options, [ @@ -826,20 +831,26 @@ class UrlbarQueryContext { } // Manage optional properties of options. - for (let [prop, checkFn] of [ + for (let [prop, checkFn, defaultValue] of [ ["providers", v => Array.isArray(v) && v.length], ["sources", v => Array.isArray(v) && v.length], ["engineName", v => typeof v == "string" && !!v.length], ["currentPage", v => typeof v == "string" && !!v.length], + ["allowSearchSuggestions", v => true, true], ]) { - if (options[prop]) { + if (prop in options) { if (!checkFn(options[prop])) { throw new Error(`Invalid value for option "${prop}"`); } this[prop] = options[prop]; + } else if (defaultValue !== undefined) { + this[prop] = defaultValue; } } + this.allowSearchSuggestions = + "allowSearchSuggestions" in this ? !!this.allowSearchSuggestions : true; + this.lastResultCount = 0; this.userContextId = options.userContextId || diff --git a/browser/components/urlbar/docs/overview.rst b/browser/components/urlbar/docs/overview.rst index bf4cc3610ce8..b0d1738a066c 100644 --- a/browser/components/urlbar/docs/overview.rst +++ b/browser/components/urlbar/docs/overview.rst @@ -52,6 +52,12 @@ It is augmented as it progresses through the system, with various information: // with the search service. currentPage: // {string} url of the page that was loaded when the search // began. + allowSearchSuggestions: // {boolean} Whether to allow search suggestions. + // This is a veto, meaning that when false, + // suggestions will not be fetched, but when true, + // some other condition may still prohibit + // suggestions, like private browsing mode. Defaults + // to true. // Properties added by the Model. results; // {array} list of UrlbarResult objects. diff --git a/browser/components/urlbar/tests/browser/browser_searchSuggestions.js b/browser/components/urlbar/tests/browser/browser_searchSuggestions.js index 65c19640d635..dccbde64dc7e 100644 --- a/browser/components/urlbar/tests/browser/browser_searchSuggestions.js +++ b/browser/components/urlbar/tests/browser/browser_searchSuggestions.js @@ -11,6 +11,8 @@ const SUGGEST_URLBAR_PREF = "browser.urlbar.suggest.searches"; const TEST_ENGINE_BASENAME = "searchSuggestionEngine.xml"; +const MAX_CHARS_PREF = "browser.urlbar.maxCharsForSearchSuggestions"; + // Must run first. add_task(async function prepare() { let suggestionsEnabled = Services.prefs.getBoolPref(SUGGEST_URLBAR_PREF); @@ -38,7 +40,7 @@ add_task(async function clickSuggestion() { waitForFocus: SimpleTest.waitForFocus, value: "foo", }); - let [idx, suggestion, engineName] = await promiseFirstSuggestion(); + let [idx, suggestion, engineName] = await getFirstSuggestion(); Assert.equal( engineName, "browser_searchSuggestionEngine searchSuggestionEngine.xml", @@ -68,7 +70,7 @@ async function testPressEnterOnSuggestion( waitForFocus: SimpleTest.waitForFocus, value: "foo", }); - let [idx, suggestion, engineName] = await promiseFirstSuggestion(); + let [idx, suggestion, engineName] = await getFirstSuggestion(); Assert.equal( engineName, "browser_searchSuggestionEngine searchSuggestionEngine.xml", @@ -109,7 +111,7 @@ add_task(async function copySuggestionText() { waitForFocus: SimpleTest.waitForFocus, value: "foo", }); - let [idx, suggestion] = await promiseFirstSuggestion(); + let [idx, suggestion] = await getFirstSuggestion(); for (let i = 0; i < idx; ++i) { EventUtils.synthesizeKey("KEY_ArrowDown"); } @@ -119,7 +121,125 @@ add_task(async function copySuggestionText() { }); }); +add_task(async function typeMaxChars() { + gURLBar.focus(); + + let maxChars = 10; + await SpecialPowers.pushPrefEnv({ + set: [[MAX_CHARS_PREF, maxChars]], + }); + + // Make a string as long as maxChars and type it. + let value = ""; + for (let i = 0; i < maxChars; i++) { + value += String.fromCharCode("a".charCodeAt(0) + i); + } + await UrlbarTestUtils.promiseAutocompleteResultPopup({ + window, + value, + waitForFocus: SimpleTest.waitForFocus, + }); + + // Suggestions should be fetched since we allow them when typing, and the + // value so far isn't longer than maxChars anyway. + await assertSuggestions([value + "foo", value + "bar"]); + + // Now type some additional chars. Suggestions should still be fetched since + // we allow them when typing. + for (let i = 0; i < 3; i++) { + let char = String.fromCharCode("z".charCodeAt(0) - i); + value += char; + EventUtils.synthesizeKey(char); + await assertSuggestions([value + "foo", value + "bar"]); + } + + await SpecialPowers.popPrefEnv(); +}); + +add_task(async function pasteMaxChars() { + gURLBar.focus(); + + let maxChars = 10; + await SpecialPowers.pushPrefEnv({ + set: [[MAX_CHARS_PREF, maxChars]], + }); + + // Make a string as long as maxChars and paste it. + let value = ""; + for (let i = 0; i < maxChars; i++) { + value += String.fromCharCode("a".charCodeAt(0) + i); + } + await selectAndPaste(value); + + // Suggestions should be fetched since the pasted string is not longer than + // maxChars. + await assertSuggestions([value + "foo", value + "bar"]); + + // Now type some additional chars. Suggestions should still be fetched since + // we allow them when typing. + for (let i = 0; i < 3; i++) { + let char = String.fromCharCode("z".charCodeAt(0) - i); + value += char; + EventUtils.synthesizeKey(char); + await assertSuggestions([value + "foo", value + "bar"]); + } + + await SpecialPowers.popPrefEnv(); +}); + +add_task(async function pasteMoreThanMaxChars() { + gURLBar.focus(); + + let maxChars = 10; + await SpecialPowers.pushPrefEnv({ + set: [[MAX_CHARS_PREF, maxChars]], + }); + + // Make a string longer than maxChars and paste it. + let value = ""; + for (let i = 0; i < 2 * maxChars; i++) { + value += String.fromCharCode("a".charCodeAt(0) + i); + } + await selectAndPaste(value); + + // Suggestions should not be fetched since the value was pasted and it was + // longer than maxChars. + await assertSuggestions([]); + + // Now type some additional chars. Suggestions should now be fetched since we + // allow them when typing. + for (let i = 0; i < 3; i++) { + let char = String.fromCharCode("z".charCodeAt(0) - i); + value += char; + EventUtils.synthesizeKey(char); + await assertSuggestions([value + "foo", value + "bar"]); + } + + // Paste again. The string is longer than maxChars, so suggestions should not + // be fetched. + await selectAndPaste(value); + await assertSuggestions([]); + + await SpecialPowers.popPrefEnv(); +}); + async function getFirstSuggestion() { + let results = await getSuggestionResults(); + if (!results.length) { + return [-1, null, null]; + } + let result = results[0]; + return [ + result.index, + result.searchParams.suggestion, + result.searchParams.engine, + ]; +} + +async function getSuggestionResults() { + await UrlbarTestUtils.promiseSearchComplete(window); + + let results = []; let matchCount = UrlbarTestUtils.getResultCount(window); for (let i = 0; i < matchCount; i++) { let result = await UrlbarTestUtils.getDetailsOfResultAt(window, i); @@ -127,13 +247,19 @@ async function getFirstSuggestion() { result.type == UrlbarUtils.RESULT_TYPE.SEARCH && result.searchParams.suggestion ) { - return [i, result.searchParams.suggestion, result.searchParams.engine]; + result.index = i; + results.push(result); } } - return [-1, null, null]; + return results; } -async function promiseFirstSuggestion() { - await UrlbarTestUtils.promiseSuggestionsPresent(window); - return getFirstSuggestion(); +async function assertSuggestions(expectedSuggestions) { + let results = await getSuggestionResults(); + let actualSuggestions = results.map(r => r.searchParams.suggestion); + Assert.deepEqual( + actualSuggestions, + expectedSuggestions, + "Expected suggestions" + ); } diff --git a/browser/components/urlbar/tests/unit/test_UrlbarQueryContext_restrictSource.js b/browser/components/urlbar/tests/unit/test_UrlbarQueryContext_restrictSource.js index 5889b47986df..4804568c9f13 100644 --- a/browser/components/urlbar/tests/unit/test_UrlbarQueryContext_restrictSource.js +++ b/browser/components/urlbar/tests/unit/test_UrlbarQueryContext_restrictSource.js @@ -128,13 +128,16 @@ add_task(async function test_restrictions() { async function get_results(test) { let controller = UrlbarTestUtils.newMockController(); - let queryContext = createContext(test.searchString, { + let options = { allowAutofill: false, isPrivate: false, maxResults: 10, sources: test.sources, - engineName: test.engineName, - }); + }; + if (test.engineName) { + options.engineName = test.engineName; + } + let queryContext = createContext(test.searchString, options); await controller.startQuery(queryContext); return queryContext.results; }