Bug 1539804 - Quantumbar: Re-enable browser_urlbarStopSearchOnSelection.js and fix a couple of related problems. r=mak
☠☠ backed out by 99548c52b81f ☠ ☠
authorDrew Willcoxon <adw@mozilla.com>
Wed, 01 May 2019 02:30:47 +0000
changeset 530859 9242458b79d5ae6e9d48c9a2a591680d79c13211
parent 530858 cbdf3fdfcbac59d4ccac118d2e6a96ac360a94a5
child 530860 5c3bff8f45d696710545579b14346a65c07aa2cf
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1539804
milestone68.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 1539804 - Quantumbar: Re-enable browser_urlbarStopSearchOnSelection.js and fix a couple of related problems. r=mak This test uncovered a couple of problems: (1) UrlbarController.handleKeyNavigation relies on event.defaultPrevented to tell whether the one-offs handled the key event. That's a problem when combined with deferring the down arrow key. handleKeyNavigation is called twice in that case. The first time, the event is deferred (so executeAction = false), and handleKeyNavigation calls event.preventDefault. The second time, the event is being replayed, but defaultPrevented is true from the previous call regardless of whether the one-offs actually handled the event. So handleKeyNavigation always returns early because it thinks the one-offs always handled the event, so it never properly replays down arrow keys. (2) UrlbarProviderUnifiedComplete's query promise is never resolved when the query is canceled. That's a problem in general of course but I tripped over it in this test because I need to check results after the query is canceled, and the test ended up hanging since UrlbarTestUtils waits for the query to finish in order to get its results. It's not a problem in UnifiedComplete itself per se because of course awesomebar uses UnifiedComplete too, and it doesn't have this problem. The difference is that nsAutoCompleteController::StopSearch calls input->OnSearchComplete() (via PostSearchCleanup): https://searchfox.org/mozilla-central/rev/b756e6d00728dda4121f8278a744381d8643317a/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1433 Quantumbar's UnifiedComplete provider is missing that behavior, so this patch adds it by resolving its query promise when the query is canceled. Differential Revision: https://phabricator.services.mozilla.com/D29300
browser/components/search/content/search-one-offs.js
browser/components/urlbar/UrlbarController.jsm
browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
browser/components/urlbar/tests/browser/browser.ini
browser/components/urlbar/tests/browser/browser_urlbarStopSearchOnSelection.js
browser/components/urlbar/tests/browser/searchSuggestionEngineSlow.xml
--- a/browser/components/search/content/search-one-offs.js
+++ b/browser/components/search/content/search-one-offs.js
@@ -824,18 +824,18 @@ class SearchOneOffs {
 
   /**
    * This handles key presses specific to the one-off buttons like Tab and
    * Alt+Up/Down, and Up/Down keys within the buttons.  Since one-off buttons
    * are always used in conjunction with a list of some sort (in this.popup),
    * it also handles Up/Down keys that cross the boundaries between list
    * items and the one-off buttons.
    *
-   * If this method handles the key press, then event.defaultPrevented will
-   * be true when it returns.
+   * If this method handles the key press, then it will call
+   * event.preventDefault() and return true.
    *
    * @param {Event} event
    *        The key event.
    * @param {number} numListItems
    *        The number of items in the list.  The reason that this is a
    *        parameter at all is that the list may contain items at the end
    *        that should be ignored, depending on the consumer.  That's true
    *        for the urlbar for example.
@@ -844,28 +844,30 @@ class SearchOneOffs {
    *        buttons contains a selection.  Pass false if either the list or
    *        the one-off buttons (or both) should always contain a selection.
    * @param {string} [textboxUserValue]
    *        When the last list item is selected and the user presses Down,
    *        the first one-off becomes selected and the textbox value is
    *        restored to the value that the user typed.  Pass that value here.
    *        However, if you pass true for allowEmptySelection, you don't need
    *        to pass anything for this parameter.  (Pass undefined or null.)
+   * @returns {boolean} True if the one-offs handled the key press.
    */
   handleKeyPress(event, numListItems, allowEmptySelection, textboxUserValue) {
     if (!this.popup) {
-      return;
+      return false;
     }
     let handled = this._handleKeyPress(event, numListItems,
       allowEmptySelection,
       textboxUserValue);
     if (handled) {
       event.preventDefault();
       event.stopPropagation();
     }
+    return handled;
   }
 
   _handleKeyPress(event, numListItems, allowEmptySelection, textboxUserValue) {
     if (this.compact && this.buttons.collapsed) {
       return false;
     }
     if (event.keyCode == KeyEvent.DOM_VK_RIGHT &&
         this.selectedButton &&
--- a/browser/components/urlbar/UrlbarController.jsm
+++ b/browser/components/urlbar/UrlbarController.jsm
@@ -253,22 +253,22 @@ class UrlbarController {
       }
       event.preventDefault();
       return;
     }
 
     if (this.view.isOpen && executeAction) {
       let queryContext = this._lastQueryContext;
       if (queryContext) {
-        this.view.oneOffSearchButtons.handleKeyPress(
+        let handled = this.view.oneOffSearchButtons.handleKeyPress(
           event,
           queryContext.results.length,
           this.view.allowEmptySelection,
           queryContext.searchString);
-        if (event.defaultPrevented) {
+        if (handled) {
           return;
         }
       }
     }
 
     switch (event.keyCode) {
       case KeyEvent.DOM_VK_ESCAPE:
         if (executeAction) {
--- a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
+++ b/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
@@ -122,20 +122,22 @@ class ProviderUnifiedComplete extends Ur
     await new Promise(resolve => {
       let listener = {
         onSearchResult(_, result) {
           let {done, matches} = convertResultToMatches(queryContext, result, urls);
           for (let match of matches) {
             addCallback(UrlbarProviderUnifiedComplete, match);
           }
           if (done) {
+            delete this._resolveSearch;
             resolve();
           }
         },
       };
+      this._resolveSearch = resolve;
       unifiedComplete.startSearch(queryContext.searchString,
                                   params.join(" "),
                                   null, // previousResult
                                   listener);
     });
 
     // We are done.
     this.queries.delete(queryContext);
@@ -145,16 +147,19 @@ class ProviderUnifiedComplete extends Ur
    * Cancels a running query.
    * @param {object} queryContext The query context object
    */
   cancelQuery(queryContext) {
     logger.info(`Canceling query for ${queryContext.searchString}`);
     // This doesn't properly support being used concurrently by multiple fields.
     this.queries.delete(queryContext);
     unifiedComplete.stopSearch();
+    if (this._resolveSearch) {
+      this._resolveSearch();
+    }
   }
 }
 
 var UrlbarProviderUnifiedComplete = new ProviderUnifiedComplete();
 
 /**
  * Convert from a nsIAutocompleteResult to a list of new matches.
  * Note that at every call we get the full set of matches, included the
--- a/browser/components/urlbar/tests/browser/browser.ini
+++ b/browser/components/urlbar/tests/browser/browser.ini
@@ -161,17 +161,16 @@ support-files =
 [browser_urlbarSearchSuggestions.js]
 support-files =
   searchSuggestionEngine.xml
   searchSuggestionEngine.sjs
 [browser_URLBarSetURI.js]
 skip-if = (os == "linux" || os == "mac") && debug # bug 970052, bug 970053
 [browser_urlbarStop.js]
 [browser_urlbarStopSearchOnSelection.js]
-skip-if = true # Bug 1524510 - Not implemented yet.
 support-files =
   searchSuggestionEngineSlow.xml
   searchSuggestionEngine.sjs
 [browser_urlbarTokenAlias.js]
 [browser_urlbarUpdateForDomainCompletion.js]
 [browser_urlbarValueOnTabSwitch.js]
 [browser_userTypedValue.js]
 support-files = file_userTypedValue.html
--- a/browser/components/urlbar/tests/browser/browser_urlbarStopSearchOnSelection.js
+++ b/browser/components/urlbar/tests/browser/browser_urlbarStopSearchOnSelection.js
@@ -8,17 +8,17 @@
  */
 
 "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;
+const TEST_ENGINE_SUGGESTIONS_TIMEOUT = 3000;
 
 // 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]],
@@ -31,53 +31,73 @@ add_task(async function init() {
   await Services.search.setDefault(engine);
   registerCleanupFunction(async () => {
     await Services.search.setDefault(oldDefaultEngine);
     await PlacesUtils.history.clear();
   });
 });
 
 add_task(async function mainTest() {
-  // Trigger an initial search.  Restrict matches to search suggestions.
-  await promiseAutocompleteResultPopup(`${UrlbarTokenizer.RESTRICT.SEARCH} 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));
+  // Open a tab that will match the search string below so that we're guaranteed
+  // to have more than one result (the heuristic result) so that we can change
+  // the selected result.  We open a tab instead of adding a page in history
+  // because open tabs are kept in a memory SQLite table, so open-tab results
+  // are more likely than history results to be fetched before our slow search
+  // suggestions.  This is important when the test runs on slow debug builds on
+  // slow machines.
+  await BrowserTestUtils.withNewTab("http://example.com/", async () => {
+    await BrowserTestUtils.withNewTab("about:blank", async () => {
+      // Do an initial search.  There should be 4 results: heuristic, open tab,
+      // and the two suggestions.
+      await promiseAutocompleteResultPopup("amp");
+      await TestUtils.waitForCondition(() => {
+        return UrlbarTestUtils.getResultCount(window) ==
+               2 + TEST_ENGINE_NUM_EXPECTED_RESULTS;
+      });
 
-  // 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);
+      // Type a character to start a new search.  The new search should still
+      // match the open tab so that the open-tab result appears again.
+      EventUtils.synthesizeKey("l");
 
-  // Now press the Down arrow key to change the selection.
-  await new Promise(r => EventUtils.synthesizeKey("VK_DOWN", {}, window, r));
+      // There should be 2 results immediately: heuristic and open tab.  (On
+      // quantumbar, there will be only 2, but on awesomebar, there will be 4
+      // since awesomebar will keep showing the excess old results for a bit.)
+      await TestUtils.waitForCondition(() => {
+        return UrlbarTestUtils.getResultCount(window) >= 2;
+      });
 
-  // 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));
+      // Before the search completes, change the selected result.  Pressing only
+      // the down arrow key ends up selecting the first one-off on Linux debug
+      // builds on the infrastructure for some reason, so arrow back up to
+      // select the heuristic result again.  The important thing is to change
+      // the selection.  It doesn't matter which result ends up selected.
+      EventUtils.synthesizeKey("KEY_ArrowDown");
+      EventUtils.synthesizeKey("KEY_ArrowUp");
 
-  // Both of the suggestion results should retain their initial values,
-  // "testfoo" and "testbar".  They should *not* be "testxfoo" and "textxbar".
+      // Wait for the new search to complete.  It should be canceled due to the
+      // selection change, but it should still complete.
+      await promiseSearchComplete();
+
+      // To make absolutely sure the suggestions don't appear after the search
+      // completes, wait a bit.
+      await new Promise(r => setTimeout(r, 1 + TEST_ENGINE_SUGGESTIONS_TIMEOUT));
 
-  // + 1 for the heuristic result
-  let numExpectedResults = TEST_ENGINE_NUM_EXPECTED_RESULTS + 1;
-  Assert.equal(UrlbarTestUtils.getResultCount(window), numExpectedResults,
-    "Should have the correct number of results displayed");
+      // The heuristic result should reflect the new search, "ampl".
+      let result = await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
+      Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH,
+        "Should have the correct result type");
+      Assert.equal(result.searchParams.query, "ampl",
+        "Should have the correct query");
 
-  let expectedSuggestions = ["testfoo", "testbar"];
-  for (let i = 0; i < TEST_ENGINE_NUM_EXPECTED_RESULTS; i++) {
-    // + 1 to skip the heuristic result
-    let result = await UrlbarTestUtils.getDetailsOfResultAt(window, i + 1);
-    Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH,
-      "Should have the correct result type");
-    Assert.equal(result.searchParams.suggestion, expectedSuggestions[i],
-      "Should have the correct suggestion");
-  }
+      // None of the other results should be "ampl" suggestions, i.e., amplfoo
+      // and amplbar should not be in the results.
+      let count = UrlbarTestUtils.getResultCount(window);
+      for (let i = 1; i < count; i++) {
+        result = await UrlbarTestUtils.getDetailsOfResultAt(window, i);
+        Assert.ok(
+          result.type != UrlbarUtils.RESULT_TYPE.SEARCH ||
+          !["amplfoo", "amplbar"].includes(result.searchParams.suggestion),
+          "Suggestions should not contain the typed l char"
+        );
+      }
+    });
+  });
 });
--- a/browser/components/urlbar/tests/browser/searchSuggestionEngineSlow.xml
+++ b/browser/components/urlbar/tests/browser/searchSuggestionEngineSlow.xml
@@ -1,11 +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/components/urlbar/tests/browser/searchSuggestionEngine.sjs?query={searchTerms}&amp;timeout=1000"/>
+<Url type="application/x-suggestions+json" method="GET" template="http://mochi.test:8888/browser/browser/components/urlbar/tests/browser/searchSuggestionEngine.sjs?query={searchTerms}&amp;timeout=3000"/>
 <Url type="text/html" method="GET" template="http://mochi.test:8888/" rel="searchform">
   <Param name="terms" value="{searchTerms}"/>
 </Url>
 </SearchPlugin>