Bug 1410240 - Search suggestions keep displacing awesomebar results as I'm about to click on them. r=mak
authorDrew Willcoxon <adw@mozilla.com>
Wed, 22 Nov 2017 16:22:32 -0800
changeset 393250 f955a43accb4f537a9102a2741b43af850b01ca7
parent 393249 1d9c0a75a00780b24036c0c85c352860f80148dd
child 393251 7beea3063907269c70c04ad030b65fb1712169df
push id55871
push userdwillcoxon@mozilla.com
push dateThu, 23 Nov 2017 00:25:37 +0000
treeherderautoland@f955a43accb4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1410240
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1410240 - Search suggestions keep displacing awesomebar results as I'm about to click on them. r=mak MozReview-Commit-ID: 2NdV9qWzld1
browser/base/content/test/urlbar/browser.ini
browser/base/content/test/urlbar/browser_urlbarOneOffs_searchSuggestions.js
browser/base/content/test/urlbar/browser_urlbarSearchSuggestions_opt-out.js
browser/base/content/test/urlbar/browser_urlbarStopSearchOnSelection.js
browser/base/content/test/urlbar/head.js
browser/base/content/test/urlbar/searchSuggestionEngine.sjs
browser/base/content/test/urlbar/searchSuggestionEngineSlow.xml
toolkit/components/autocomplete/nsAutoCompleteController.cpp
--- 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}&amp;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
@@ -497,16 +497,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) {