Bug 1530706 - IME may be displayed along with the urlbar results panel. r=Standard8
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 19 Mar 2019 14:06:33 +0000
changeset 523842 b20f59942a1ec5067cfa06dad730f7bff27c8f07
parent 523841 dc8935d7c0b10afc0401049936cc9d5f9fc5b003
child 523843 eab42b6a2779a0bf01471743f06c225a318c907e
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)
reviewersStandard8
bugs1530706
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 1530706 - IME may be displayed along with the urlbar results panel. r=Standard8 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;
+});