Bug 1410240 - Search suggestions keep displacing awesomebar results as I'm about to click on them. r?mak
MozReview-Commit-ID: 2NdV9qWzld1
--- a/browser/base/content/test/urlbar/browser.ini
+++ b/browser/base/content/test/urlbar/browser.ini
@@ -123,8 +123,12 @@ support-files =
slow-page.sjs
[browser_urlbar_remoteness_switch.js]
run-if = e10s
[browser_urlHighlight.js]
[browser_wyciwyg_urlbarCopying.js]
subsuite = clipboard
support-files =
test_wyciwyg_copying.html
+[browser_urlbarStopSearchOnSelection.js]
+support-files =
+ searchSuggestionEngineSlow.xml
+ searchSuggestionEngine.sjs
--- a/browser/base/content/test/urlbar/browser_urlbarOneOffs_searchSuggestions.js
+++ b/browser/base/content/test/urlbar/browser_urlbarOneOffs_searchSuggestions.js
@@ -22,18 +22,17 @@ add_task(async function init() {
// Presses the Return key when a one-off is selected after selecting a search
// suggestion.
add_task(async function oneOffReturnAfterSuggestion() {
let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
let typedValue = "foo";
await promiseAutocompleteResultPopup(typedValue, window, true);
- await BrowserTestUtils.waitForCondition(suggestionsPresent,
- "waiting for suggestions");
+ await promiseSuggestionsPresent();
assertState(0, -1, typedValue);
// Down to select the first search suggestion.
EventUtils.synthesizeKey("VK_DOWN", {});
assertState(1, -1, "foofoo");
// Down to select the next search suggestion.
EventUtils.synthesizeKey("VK_DOWN", {});
@@ -54,18 +53,17 @@ add_task(async function oneOffReturnAfte
});
// Clicks a one-off engine after selecting a search suggestion.
add_task(async function oneOffClickAfterSuggestion() {
let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
let typedValue = "foo";
await promiseAutocompleteResultPopup(typedValue, window, true);
- await BrowserTestUtils.waitForCondition(suggestionsPresent,
- "waiting for suggestions");
+ await promiseSuggestionsPresent();
assertState(0, -1, typedValue);
// Down to select the first search suggestion.
EventUtils.synthesizeKey("VK_DOWN", {});
assertState(1, -1, "foofoo");
// Down to select the next search suggestion.
EventUtils.synthesizeKey("VK_DOWN", {});
@@ -82,31 +80,29 @@ add_task(async function oneOffClickAfter
await BrowserTestUtils.removeTab(tab);
});
add_task(async function overridden_engine_not_reused() {
info("An overridden search suggestion item should not be reused by a search with another engine");
let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
let typedValue = "foo";
await promiseAutocompleteResultPopup(typedValue, window, true);
- await BrowserTestUtils.waitForCondition(suggestionsPresent,
- "waiting for suggestions");
+ await promiseSuggestionsPresent();
// Down to select the first search suggestion.
EventUtils.synthesizeKey("VK_DOWN", {});
assertState(1, -1, "foofoo");
// ALT+Down to select the second search engine.
EventUtils.synthesizeKey("VK_DOWN", { altKey: true });
EventUtils.synthesizeKey("VK_DOWN", { altKey: true });
assertState(1, 1, "foofoo");
let label = gURLBar.popup.richlistbox.children[gURLBar.popup.richlistbox.selectedIndex].label;
// Run again the query, check the label has been replaced.
await promiseAutocompleteResultPopup(typedValue, window, true);
- await BrowserTestUtils.waitForCondition(suggestionsPresent,
- "waiting for suggestions");
+ await promiseSuggestionsPresent();
assertState(0, -1, "foo");
let newLabel = gURLBar.popup.richlistbox.children[1].label;
Assert.notEqual(newLabel, label, "The label should have been updated");
await BrowserTestUtils.removeTab(tab);
});
function assertState(result, oneOff, textValue = undefined) {
@@ -118,25 +114,8 @@ function assertState(result, oneOff, tex
Assert.equal(gURLBar.textValue, textValue, "Expected textValue");
}
}
async function hidePopup() {
EventUtils.synthesizeKey("VK_ESCAPE", {});
await promisePopupHidden(gURLBar.popup);
}
-
-function suggestionsPresent() {
- let controller = gURLBar.popup.input.controller;
- let matchCount = controller.matchCount;
- for (let i = 0; i < matchCount; i++) {
- let url = controller.getValueAt(i);
- let mozActionMatch = url.match(/^moz-action:([^,]+),(.*)$/);
- if (mozActionMatch) {
- let [, type, paramStr] = mozActionMatch;
- let params = JSON.parse(paramStr);
- if (type == "searchengine" && "searchSuggestion" in params) {
- return true;
- }
- }
- }
- return false;
-}
--- a/browser/base/content/test/urlbar/browser_urlbarSearchSuggestions_opt-out.js
+++ b/browser/base/content/test/urlbar/browser_urlbarSearchSuggestions_opt-out.js
@@ -134,33 +134,16 @@ add_task(async function userMadeChoice()
function setupVisibleHint() {
Services.prefs.clearUserPref(TIMES_PREF);
Services.prefs.setBoolPref(SUGGEST_ALL_PREF, true);
// Toggle to reset the whichNotification cache.
Services.prefs.setBoolPref(SUGGEST_URLBAR_PREF, false);
Services.prefs.setBoolPref(SUGGEST_URLBAR_PREF, true);
}
-function suggestionsPresent() {
- let controller = gURLBar.popup.input.controller;
- let matchCount = controller.matchCount;
- for (let i = 0; i < matchCount; i++) {
- let url = controller.getValueAt(i);
- let mozActionMatch = url.match(/^moz-action:([^,]+),(.*)$/);
- if (mozActionMatch) {
- let [, type, paramStr] = mozActionMatch;
- let params = JSON.parse(paramStr);
- if (type == "searchengine" && "searchSuggestion" in params) {
- return true;
- }
- }
- }
- return false;
-}
-
function assertVisible(visible, win = window) {
let style =
win.getComputedStyle(win.gURLBar.popup.searchSuggestionsNotification);
let check = visible ? "notEqual" : "equal";
Assert[check](style.display, "none");
}
function assertFooterVisible(visible, win = window) {
let style = win.getComputedStyle(win.gURLBar.popup.footer);
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/urlbar/browser_urlbarStopSearchOnSelection.js
@@ -0,0 +1,88 @@
+/* eslint-disable mozilla/no-arbitrary-setTimeout */
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const TEST_ENGINE_BASENAME = "searchSuggestionEngineSlow.xml";
+
+// This should match the `timeout` query param used in the suggestions URL in
+// the test engine.
+const TEST_ENGINE_SUGGESTIONS_TIMEOUT = 1000;
+
+// The number of suggestions returned by the test engine.
+const TEST_ENGINE_NUM_EXPECTED_RESULTS = 2;
+
+add_task(async function init() {
+ await PlacesUtils.history.clear();
+ await SpecialPowers.pushPrefEnv({
+ set: [["browser.urlbar.suggest.searches", true]],
+ });
+ // Add a test search engine that returns suggestions on a delay.
+ let engine = await promiseNewSearchEngine(TEST_ENGINE_BASENAME);
+ let oldCurrentEngine = Services.search.currentEngine;
+ Services.search.moveEngine(engine, 0);
+ Services.search.currentEngine = engine;
+ registerCleanupFunction(async () => {
+ Services.search.currentEngine = oldCurrentEngine;
+ await PlacesUtils.history.clear();
+ // Make sure the popup is closed for the next test.
+ gURLBar.blur();
+ Assert.ok(!gURLBar.popup.popupOpen, "popup should be closed");
+ });
+});
+
+add_task(async function mainTest() {
+ // Trigger an initial search. Use the "$" token to restrict matches to search
+ // suggestions.
+ await promiseAutocompleteResultPopup("$ test", window);
+ await promiseSuggestionsPresent("Waiting for initial suggestions");
+
+ // Now synthesize typing a character. promiseAutocompleteResultPopup doesn't
+ // work in this case because it causes the popup to close and re-open with the
+ // new input text.
+ await new Promise(r => EventUtils.synthesizeKey("x", {}, window, r));
+
+ // At this point, a new search starts, the autocomplete's _invalidate is
+ // called, _appendCurrentResult is called, and matchCount remains 3 from the
+ // previous search (the heuristic result plus the two search suggestions).
+ // The suggestion results should not outwardly change, however, since the
+ // autocomplete controller should still be returning the initial suggestions.
+ // What has changed, though, is the search string, which is kept in the
+ // results' ac-text attributes. Call waitForAutocompleteResultAt to wait for
+ // the results' ac-text to be set to the new search string, "testx", to make
+ // sure the autocomplete has seen the new search.
+ await waitForAutocompleteResultAt(TEST_ENGINE_NUM_EXPECTED_RESULTS);
+
+ // Now press the Down arrow key to change the selection.
+ await new Promise(r => EventUtils.synthesizeKey("VK_DOWN", {}, window, r));
+
+ // Changing the selection should have stopped the new search triggered by
+ // typing the "x" character. Wait a bit to make sure it really stopped.
+ await new Promise(r => setTimeout(r, 2 * TEST_ENGINE_SUGGESTIONS_TIMEOUT));
+
+ // Both of the suggestion results should retain their initial values,
+ // "testfoo" and "testbar". They should *not* be "testxfoo" and "textxbar".
+
+ // + 1 for the heuristic result
+ let numExpectedResults = TEST_ENGINE_NUM_EXPECTED_RESULTS + 1;
+ let results = gURLBar.popup.richlistbox.children;
+ let numActualResults = Array.reduce(results, (memo, result) => {
+ if (!result.collapsed) {
+ memo++;
+ }
+ return memo;
+ }, 0);
+ Assert.equal(numActualResults, numExpectedResults);
+
+ let expectedSuggestions = ["testfoo", "testbar"];
+ for (let i = 0; i < TEST_ENGINE_NUM_EXPECTED_RESULTS; i++) {
+ // + 1 to skip the heuristic result
+ let item = gURLBar.popup.richlistbox.children[i + 1];
+ let action = item._parseActionUrl(item.getAttribute("url"));
+ Assert.ok(action);
+ Assert.equal(action.type, "searchengine");
+ Assert.ok("searchSuggestion" in action.params);
+ Assert.equal(action.params.searchSuggestion, expectedSuggestions[i]);
+ }
+});
--- a/browser/base/content/test/urlbar/head.js
+++ b/browser/base/content/test/urlbar/head.js
@@ -319,8 +319,30 @@ async function waitForAutocompleteResult
await BrowserTestUtils.waitForCondition(
() => gURLBar.popup.richlistbox.children.length > index &&
gURLBar.popup.richlistbox.children[index].getAttribute("ac-text") == searchString,
`Waiting for the autocomplete result for "${searchString}" at [${index}] to appear`);
// Ensure the addition is complete, for proper mouse events on the entries.
await new Promise(resolve => window.requestIdleCallback(resolve, {timeout: 1000}));
return gURLBar.popup.richlistbox.children[index];
}
+
+function promiseSuggestionsPresent(msg = "") {
+ return TestUtils.waitForCondition(suggestionsPresent,
+ msg || "Waiting for suggestions");
+}
+
+function suggestionsPresent() {
+ let controller = gURLBar.popup.input.controller;
+ let matchCount = controller.matchCount;
+ for (let i = 0; i < matchCount; i++) {
+ let url = controller.getValueAt(i);
+ let mozActionMatch = url.match(/^moz-action:([^,]+),(.*)$/);
+ if (mozActionMatch) {
+ let [, type, paramStr] = mozActionMatch;
+ let params = JSON.parse(paramStr);
+ if (type == "searchengine" && "searchSuggestion" in params) {
+ return true;
+ }
+ }
+ }
+ return false;
+}
--- a/browser/base/content/test/urlbar/searchSuggestionEngine.sjs
+++ b/browser/base/content/test/urlbar/searchSuggestionEngine.sjs
@@ -1,9 +1,48 @@
/* Any copyright is dedicated to the Public Domain.
* http://creativecommons.org/publicdomain/zero/1.0/ */
+const { classes: Cc, interfaces: Ci } = Components;
+
+let gTimer;
+
function handleRequest(req, resp) {
+ // Parse the query params. If the params aren't in the form "foo=bar", then
+ // treat the entire query string as a search string.
+ let params = req.queryString.split("&").reduce((memo, pair) => {
+ let [key, val] = pair.split("=");
+ if (!val) {
+ // This part isn't in the form "foo=bar". Treat it as the search string
+ // (the "query").
+ val = key;
+ key = "query";
+ }
+ memo[decode(key)] = decode(val);
+ return memo;
+ }, {});
+
+ let timeout = parseInt(params["timeout"]);
+ if (timeout) {
+ // Write the response after a timeout.
+ resp.processAsync();
+ gTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
+ gTimer.init(() => {
+ writeResponse(params, resp);
+ resp.finish();
+ }, timeout, Ci.nsITimer.TYPE_ONE_SHOT);
+ return;
+ }
+
+ writeResponse(params, resp);
+}
+
+function writeResponse(params, resp) {
+ // Echo back the search string with "foo" and "bar" appended.
let suffixes = ["foo", "bar"];
- let data = [req.queryString, suffixes.map(s => req.queryString + s)];
+ let data = [params["query"], suffixes.map(s => params["query"] + s)];
resp.setHeader("Content-Type", "application/json", false);
resp.write(JSON.stringify(data));
}
+
+function decode(str) {
+ return decodeURIComponent(str.replace(/\+/g, encodeURIComponent(" ")));
+}
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/urlbar/searchSuggestionEngineSlow.xml
@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!-- Any copyright is dedicated to the Public Domain.
+ - http://creativecommons.org/publicdomain/zero/1.0/ -->
+
+<SearchPlugin xmlns="http://www.mozilla.org/2006/browser/search/">
+<ShortName>searchSuggestionEngineSlow.xml</ShortName>
+<Url type="application/x-suggestions+json" method="GET" template="http://mochi.test:8888/browser/browser/base/content/test/urlbar/searchSuggestionEngine.sjs?query={searchTerms}&timeout=1000"/>
+<Url type="text/html" method="GET" template="http://mochi.test:8888/" rel="searchform">
+ <Param name="terms" value="{searchTerms}"/>
+</Url>
+</SearchPlugin>
--- a/toolkit/components/autocomplete/nsAutoCompleteController.cpp
+++ b/toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ -496,16 +496,21 @@ nsAutoCompleteController::HandleKeyNavig
aKey == nsIDOMKeyEvent::DOM_VK_PAGE_DOWN ? true : false;
// Fill in the value of the textbox with whatever is selected in the popup
// if the completeSelectedIndex attribute is set. We check this before
// calling SelectBy of an earlier attempt to avoid crashing.
bool completeSelection;
input->GetCompleteSelectedIndex(&completeSelection);
+ // The user has keyed up or down to change the selection. Stop the search
+ // (if there is one) now so that the results do not change while the user
+ // is making a selection.
+ Unused << StopSearch();
+
// Instruct the result view to scroll by the given amount and direction
popup->SelectBy(reverse, page);
if (completeSelection)
{
int32_t selectedIndex;
popup->GetSelectedIndex(&selectedIndex);
if (selectedIndex >= 0) {