Bug 1410240 - Search suggestions keep displacing awesomebar results as I'm about to click on them. r=mak a=gchang
authorDrew Willcoxon <adw@mozilla.com>
Wed, 22 Nov 2017 16:22:32 -0800
changeset 445204 84e6f350f91e8ed8f0afe17ad113bf9a294cf4fa
parent 445203 0c07f502b744bec35b02295987e56ba1acc00131
child 445205 143546f96bc440dad090e47a47b2cb9bd44133c8
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, gchang
bugs1410240
milestone58.0
Bug 1410240 - Search suggestions keep displacing awesomebar results as I'm about to click on them. r=mak a=gchang 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) {