Bug 1530706 - IME may be displayed along with the urlbar results panel. r=Standard8 a=pascalc
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 19 Mar 2019 14:06:33 +0000
changeset 525658 5aa8eacca7b5501cd8230c607b684c73683c41b3
parent 525657 3abb046ddc034f4c04f59302012acc81154834b6
child 525659 475f82f18288433b2a972c4d28a784bf323fe70b
push id2032
push userffxbld-merge
push dateMon, 13 May 2019 09:36:57 +0000
treeherdermozilla-release@455c1065dcbe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersStandard8, pascalc
bugs1530706
milestone67.0
Bug 1530706 - IME may be displayed along with the urlbar results panel. r=Standard8 a=pascalc Differential Revision: https://phabricator.services.mozilla.com/D23836
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/UrlbarProvidersManager.jsm
browser/components/urlbar/tests/UrlbarTestUtils.jsm
browser/components/urlbar/tests/browser/browser.ini
browser/components/urlbar/tests/browser/browser_UrlbarInput_search.js
browser/components/urlbar/tests/browser/browser_urlbarSearchFunction.js
browser/components/urlbar/tests/unit/test_providersManager_filtering.js
toolkit/components/autocomplete/nsAutoCompleteController.cpp
toolkit/components/autocomplete/tests/unit/test_stopSearch.js
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -552,16 +552,22 @@ class UrlbarInput {
       this.inputField.value = value + " ";
     } else {
       this.inputField.value = value;
     }
 
     // Avoid selecting the text if this method is called twice in a row.
     this.selectionStart = -1;
 
+    // Note: proper IME Composition handling depends on the fact this generates
+    // an input event, rather than directly invoking the controller; everything
+    // goes through _on_input, that will properly skip the search until the
+    // composition is committed.
+    // If this assumption changes, we'll have to first check we are not
+    // composing, before starting a search.
     let event = this.document.createEvent("UIEvents");
     event.initUIEvent("input", true, false, this.window, 0);
     this.inputField.dispatchEvent(event);
   }
 
   /**
    * Focus without the focus styles.
    * This is used by Activity Stream and about:privatebrowsing for search hand-off.
--- a/browser/components/urlbar/UrlbarProvidersManager.jsm
+++ b/browser/components/urlbar/UrlbarProvidersManager.jsm
@@ -232,19 +232,20 @@ class Query {
     this.acceptableSources = getAcceptableMatchSources(this.context);
     logger.debug(`Acceptable sources ${this.acceptableSources}`);
 
     let promises = [];
     for (let provider of this.providers.get(UrlbarUtils.PROVIDER_TYPE.IMMEDIATE).values()) {
       if (this.canceled) {
         break;
       }
-      if (this._providerHasAcceptableSources(provider)) {
-        promises.push(provider.startQuery(this.context, this.add.bind(this)));
-      }
+      // Immediate type providers may return heuristic results, that usually can
+      // bypass suggest.* preferences, so we always execute them, regardless of
+      // this.acceptableSources, and filter results in add().
+      promises.push(provider.startQuery(this.context, this.add.bind(this)));
     }
 
     // Tracks the delay timer. We will fire (in this specific case, cancel would
     // do the same, since the callback is empty) the timer when the search is
     // canceled, unblocking start().
     this._sleepTimer = new SkippableTimer(() => {}, UrlbarPrefs.get("delay"));
     await this._sleepTimer.promise;
 
--- a/browser/components/urlbar/tests/UrlbarTestUtils.jsm
+++ b/browser/components/urlbar/tests/UrlbarTestUtils.jsm
@@ -7,16 +7,17 @@ const EXPORTED_SYMBOLS = ["UrlbarTestUti
 
 const {XPCOMUtils} = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 
 XPCOMUtils.defineLazyModuleGetters(this, {
   BrowserTestUtils: "resource://testing-common/BrowserTestUtils.jsm",
   PlacesUtils: "resource://gre/modules/PlacesUtils.jsm",
   TestUtils: "resource://testing-common/TestUtils.jsm",
   UrlbarPrefs: "resource:///modules/UrlbarPrefs.jsm",
+  UrlbarTokenizer: "resource:///modules/UrlbarTokenizer.jsm",
   UrlbarUtils: "resource:///modules/UrlbarUtils.jsm",
 });
 
 var UrlbarTestUtils = {
   /**
    * Waits to a search to be complete.
    * @param {object} win The window containing the urlbar
    * @param {function} restoreAnimationsFn optional Function to be used to
@@ -449,20 +450,26 @@ class UrlbarAbstraction {
       };
       details.element = {
         action: element._actionText,
         row: element,
         separator: element._separator,
         url: element._urlText,
       };
       if (details.type == UrlbarUtils.RESULT_TYPE.SEARCH) {
+        // Strip restriction tokens from input.
+        let query = action.params.input;
+        let restrictTokens = Object.values(UrlbarTokenizer.RESTRICT);
+        if (restrictTokens.includes(query[0])) {
+          query = query.substring(1).trim();
+        }
         details.searchParams = {
           engine: action.params.engineName,
           keyword: action.params.alias,
-          query: action.params.input,
+          query,
           suggestion: action.params.searchSuggestion,
         };
       }
     }
     return details;
   }
 
   async promiseSearchSuggestions() {
--- a/browser/components/urlbar/tests/browser/browser.ini
+++ b/browser/components/urlbar/tests/browser/browser.ini
@@ -107,17 +107,16 @@ support-files =
 [browser_urlbarEnterAfterMouseOver.js]
 skip-if = os == "linux" # Bug 1073339 - Investigate autocomplete test unreliability on Linux/e10s
 [browser_urlbarFocusedCmdK.js]
 [browser_urlbarHashChangeProxyState.js]
 [browser_UrlbarInput_autofill.js]
 [browser_UrlbarInput_formatValue.js]
 [browser_UrlbarInput_hiddenFocus.js]
 [browser_UrlbarInput_overflow.js]
-[browser_UrlbarInput_search.js]
 [browser_UrlbarInput_tooltip.js]
 [browser_UrlbarInput_trimURLs.js]
 subsuite = clipboard
 [browser_UrlbarInput_unit.js]
 support-files = empty.xul
 [browser_UrlbarLoadRace.js]
 [browser_urlbarOneOffs_contextMenu.js]
 support-files =
deleted file mode 100644
--- a/browser/components/urlbar/tests/browser/browser_UrlbarInput_search.js
+++ /dev/null
@@ -1,22 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-add_task(async function() {
-  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
-
-  registerCleanupFunction(async function() {
-    BrowserTestUtils.removeTab(tab);
-  });
-
-  // Test with a search shortcut.
-  gURLBar.blur();
-  gURLBar.search("@google ");
-  ok(gURLBar.hasAttribute("focused"), "url bar is focused");
-  is(gURLBar.value, "@google ", "url bar has correct value");
-
-  // Test with a restricted token.
-  gURLBar.blur();
-  gURLBar.search("?");
-  ok(gURLBar.hasAttribute("focused"), "url bar is focused");
-  is(gURLBar.value, "? ", "url bar has correct value");
-});
--- a/browser/components/urlbar/tests/browser/browser_urlbarSearchFunction.js
+++ b/browser/components/urlbar/tests/browser/browser_urlbarSearchFunction.js
@@ -25,17 +25,19 @@ add_task(async function init() {
   });
 });
 
 
 // Calls search() with a normal, non-"@engine" search-string argument.
 add_task(async function basic() {
   let resetNotification = enableSearchSuggestionsNotification();
 
+  gURLBar.blur();
   gURLBar.search("basic");
+  ok(gURLBar.hasAttribute("focused"), "url bar is focused");
   await assertUrlbarValue("basic");
 
   assertSearchSuggestionsNotificationVisible(true);
   assertOneOffButtonsVisible(true);
 
   await UrlbarTestUtils.promisePopupClose(window, () =>
     EventUtils.synthesizeKey("KEY_Escape"));
 
@@ -43,40 +45,119 @@ add_task(async function basic() {
 });
 
 
 // Calls search() with an "@engine" search engine alias so that the one-off
 // search buttons and search-suggestions notification are disabled.
 add_task(async function searchEngineAlias() {
   let resetNotification = enableSearchSuggestionsNotification();
 
-  gURLBar.search("@example");
+  gURLBar.blur();
+  await UrlbarTestUtils.promisePopupOpen(window,
+    () => gURLBar.search("@example"));
+  ok(gURLBar.hasAttribute("focused"), "url bar is focused");
   await assertUrlbarValue("@example");
 
   assertSearchSuggestionsNotificationVisible(false);
   assertOneOffButtonsVisible(false);
 
   await UrlbarTestUtils.promisePopupClose(window, () =>
     EventUtils.synthesizeKey("KEY_Escape"));
 
   // Open the popup again (by doing another search) to make sure the
   // notification and one-off buttons are shown -- i.e., that we didn't
   // accidentally break them.
-  gURLBar.search("not an engine alias");
+  await UrlbarTestUtils.promisePopupOpen(window,
+    () => gURLBar.search("not an engine alias"));
   await assertUrlbarValue("not an engine alias");
   assertSearchSuggestionsNotificationVisible(true);
   assertOneOffButtonsVisible(true);
 
   await UrlbarTestUtils.promisePopupClose(window, () =>
     EventUtils.synthesizeKey("KEY_Escape"));
 
   resetNotification();
 });
 
 
+// Calls search() with a restriction character.
+add_task(async function searchRestriction() {
+  let resetNotification = enableSearchSuggestionsNotification();
+
+  gURLBar.blur();
+  await UrlbarTestUtils.promisePopupOpen(window,
+    () => gURLBar.search(UrlbarTokenizer.RESTRICT.SEARCH));
+  ok(gURLBar.hasAttribute("focused"), "url bar is focused");
+  // We always add a whitespace to restrict tokens.
+  await assertUrlbarValue(UrlbarTokenizer.RESTRICT.SEARCH + " ");
+
+  assertSearchSuggestionsNotificationVisible(true);
+  assertOneOffButtonsVisible(false);
+
+  await UrlbarTestUtils.promisePopupClose(window);
+
+  resetNotification();
+});
+
+
+// Calls search() twice with the same value. The popup should reopen.
+add_task(async function searchTwice() {
+  let resetNotification = enableSearchSuggestionsNotification();
+
+  gURLBar.blur();
+  await UrlbarTestUtils.promisePopupOpen(window, () => gURLBar.search("test"));
+  ok(gURLBar.hasAttribute("focused"), "url bar is focused");
+  await assertUrlbarValue("test");
+  assertSearchSuggestionsNotificationVisible(true);
+  assertOneOffButtonsVisible(true);
+  await UrlbarTestUtils.promisePopupClose(window);
+
+  await UrlbarTestUtils.promisePopupOpen(window, () => gURLBar.search("test"));
+  ok(gURLBar.hasAttribute("focused"), "url bar is focused");
+  await assertUrlbarValue("test");
+  assertSearchSuggestionsNotificationVisible(true);
+  assertOneOffButtonsVisible(true);
+  await UrlbarTestUtils.promisePopupClose(window);
+
+  resetNotification();
+});
+
+
+// Calls search() during an IME composition.
+add_task(async function searchIME() {
+  let resetNotification = enableSearchSuggestionsNotification();
+
+  // First run a search.
+  gURLBar.blur();
+  await UrlbarTestUtils.promisePopupOpen(window, () => gURLBar.search("test"));
+  ok(gURLBar.hasAttribute("focused"), "url bar is focused");
+  await assertUrlbarValue("test");
+  // Start composition.
+  await UrlbarTestUtils.promisePopupClose(window,
+    () => EventUtils.synthesizeComposition({ type: "compositionstart" }));
+
+  gURLBar.search("test");
+  // Unfortunately there's no other way to check we don't open the view than to
+  // wait for it.
+  // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+  await new Promise(resolve => setTimeout(resolve, 1000));
+  ok(!UrlbarTestUtils.isPopupOpen(window), "The panel should still be closed");
+
+  await UrlbarTestUtils.promisePopupOpen(window,
+    () => EventUtils.synthesizeComposition({ type: "compositioncommitasis" }));
+
+  assertSearchSuggestionsNotificationVisible(true);
+  assertOneOffButtonsVisible(true);
+
+  await UrlbarTestUtils.promisePopupClose(window);
+
+  resetNotification();
+});
+
+
 /**
  * Makes sure the search-suggestions notification will be shown the next several
  * times the popup opens.
  *
  * @returns {function} A function that you should call when you're done that
  *   resets the state state of the notification.
  */
 function enableSearchSuggestionsNotification() {
@@ -138,11 +219,16 @@ async function assertUrlbarValue(value) 
 
   Assert.equal(gURLBar.value, value);
   Assert.greater(UrlbarTestUtils.getResultCount(window), 0,
     "Should have at least one result");
 
   let result = await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
   Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH,
     "Should have type search for the first result");
+  // Strip restriction token from value.
+  let restrictTokens = Object.values(UrlbarTokenizer.RESTRICT);
+  if (restrictTokens.includes(value[0])) {
+    value = value.substring(1).trim();
+  }
   Assert.equal(result.searchParams.query, value,
     "Should have the correct query for the first result");
 }
--- a/browser/components/urlbar/tests/unit/test_providersManager_filtering.js
+++ b/browser/components/urlbar/tests/unit/test_providersManager_filtering.js
@@ -181,8 +181,72 @@ add_task(async function test_filter_sour
   info("Only tabs should be returned");
   let promise = promiseControllerNotification(controller, "onQueryResults");
   await controller.startQuery(context, controller);
   await promise;
   Assert.deepEqual(context.results.length, 1, "Should find only one match");
   Assert.deepEqual(context.results[0].source, UrlbarUtils.RESULT_SOURCE.TABS,
                    "Should find only a tab match");
 });
+
+add_task(async function test_nofilter_immediate() {
+  // Checks that even if a provider returns a result that should be filtered out
+  // it will still be invoked if it's of type immediate, and only the heuristic
+  // result is returned.
+  let controller = new UrlbarController({
+    browserWindow: {
+      location: {
+        href: AppConstants.BROWSER_CHROME_URL,
+      },
+    },
+  });
+
+  let matches = [
+    new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH,
+                     UrlbarUtils.RESULT_SOURCE.TABS,
+                     { url: "http://mozilla.org/foo/" }),
+    new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH,
+                     UrlbarUtils.RESULT_SOURCE.TABS,
+                     { url: "http://mozilla.org/foo2/" }),
+  ];
+  matches[0].heuristic = true;
+
+  /**
+   * A test provider that should be invoked.
+   */
+  class TestProvider extends UrlbarProvider {
+    get name() {
+      return "GoodProvider";
+    }
+    get type() {
+      return UrlbarUtils.PROVIDER_TYPE.IMMEDIATE;
+    }
+    get sources() {
+      return [
+        UrlbarUtils.RESULT_SOURCE.TABS,
+      ];
+    }
+    async startQuery(context, add) {
+      Assert.ok(true, "expected provider was invoked");
+      for (let match of matches) {
+        add(this, match);
+      }
+    }
+    cancelQuery(context) {}
+  }
+  UrlbarProvidersManager.registerProvider(new TestProvider());
+
+  let context = createContext(undefined, {
+    sources: [UrlbarUtils.RESULT_SOURCE.SEARCH],
+    providers: ["GoodProvider"],
+  });
+  // Disable search matches through prefs.
+  Services.prefs.setBoolPref("browser.urlbar.suggest.openpage", false);
+
+  info("Only 1 heuristic tab result should be returned");
+  let promise = promiseControllerNotification(controller, "onQueryResults");
+  await controller.startQuery(context, controller);
+  await promise;
+  Services.prefs.clearUserPref("browser.urlbar.suggest.openpage");
+  Assert.deepEqual(context.results.length, 1, "Should find only one match");
+  Assert.deepEqual(context.results[0].source, UrlbarUtils.RESULT_SOURCE.TABS,
+                   "Should find only a tab match");
+});
--- a/toolkit/components/autocomplete/nsAutoCompleteController.cpp
+++ b/toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ -160,16 +160,21 @@ nsAutoCompleteController::ResetInternalS
   mMatchCount = 0;
   mCompletedSelectionIndex = -1;
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsAutoCompleteController::StartSearch(const nsAString &aSearchString) {
+  // If composition is ongoing don't start searching yet, until it is committed.
+  if (mCompositionState == eCompositionState_Composing) {
+    return NS_OK;
+  }
+
   SetSearchStringInternal(aSearchString);
   StartSearches();
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsAutoCompleteController::HandleText(bool *_retval) {
   *_retval = false;
--- a/toolkit/components/autocomplete/tests/unit/test_stopSearch.js
+++ b/toolkit/components/autocomplete/tests/unit/test_stopSearch.js
@@ -50,17 +50,17 @@ AutoCompleteInput.prototype = {
  */
 function AutoCompleteSearch(aName) {
   this.name = aName;
 }
 AutoCompleteSearch.prototype = {
   constructor: AutoCompleteSearch,
   stopSearchInvoked: true,
   startSearch(aSearchString, aSearchParam, aPreviousResult, aListener) {
-    print("Check stop search has been called");
+    info("Check stop search has been called");
     Assert.ok(this.stopSearchInvoked);
     this.stopSearchInvoked = false;
   },
   stopSearch() {
     this.stopSearchInvoked = true;
   },
   QueryInterface: ChromeUtils.generateQI([
     Ci.nsIFactory,
@@ -97,85 +97,71 @@ function unregisterAutoCompleteSearch(aS
   let componentManager = Components.manager
                                    .QueryInterface(Ci.nsIComponentRegistrar);
   componentManager.unregisterFactory(aSearch.cid, aSearch);
 }
 
 
 var gTests = [
   function(controller) {
-    print("handleText");
+    info("handleText");
     controller.input.textValue = "hel";
     controller.handleText();
   },
   function(controller) {
-    print("handleStartComposition");
+    info("handleStartComposition");
     controller.handleStartComposition();
-  },
-  function(controller) {
-    print("handleEndComposition");
+    info("handleEndComposition");
     controller.handleEndComposition();
     // an input event always follows compositionend event.
     controller.handleText();
   },
   function(controller) {
-    print("handleEscape");
+    info("handleEscape");
     controller.handleEscape();
   },
   function(controller) {
-    print("handleEnter");
+    info("handleEnter");
     controller.handleEnter(false);
   },
   function(controller) {
-    print("handleTab");
+    info("handleTab");
     controller.handleTab();
   },
 
   function(controller) {
-    print("handleKeyNavigation");
+    info("handleKeyNavigation");
     // Hardcode KeyboardEvent.DOM_VK_RIGHT, because we can't easily
     // include KeyboardEvent here.
     controller.handleKeyNavigation(0x26 /* KeyboardEvent.DOM_VK_UP */);
   },
 ];
 
 
-var gSearch;
-var gCurrentTest;
-function run_test() {
+add_task(async function() {
   // Make an AutoCompleteSearch that always returns nothing
-  gSearch = new AutoCompleteSearch("test");
-  registerAutoCompleteSearch(gSearch);
+  let search = new AutoCompleteSearch("test");
+  registerAutoCompleteSearch(search);
 
   let controller = Cc["@mozilla.org/autocomplete/controller;1"].
                    getService(Ci.nsIAutoCompleteController);
 
   // Make an AutoCompleteInput that uses our search.
-  let input = new AutoCompleteInput([gSearch.name]);
+  let input = new AutoCompleteInput([search.name]);
   controller.input = input;
 
-  input.onSearchBegin = function() {
-    executeSoon(function() {
-      gCurrentTest(controller);
+  for (let testFn of gTests) {
+    input.onSearchBegin = function() {
+      executeSoon(() => testFn(controller));
+    };
+    let promise = new Promise(resolve => {
+      input.onSearchComplete = function() {
+        resolve();
+      };
     });
-  };
-  input.onSearchComplete = function() {
-    run_next_test(controller);
-  };
-
-  // Search is asynchronous, so don't let the test finish immediately
-  do_test_pending();
-
-  run_next_test(controller);
-}
-
-function run_next_test(controller) {
-  if (gTests.length == 0) {
-    unregisterAutoCompleteSearch(gSearch);
-    controller.stopSearch();
-    controller.input = null;
-    do_test_finished();
-    return;
+    controller.startSearch("hello");
+    await promise;
   }
 
-  gCurrentTest = gTests.shift();
-  controller.startSearch("hello");
-}
+  unregisterAutoCompleteSearch(search);
+  controller.stopSearch();
+  controller.input = null;
+});