Bug 1531327 - Properly handle actions overrides in the Quantum Bar. r=dao
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 06 Mar 2019 15:21:35 +0000
changeset 520497 d9e6f5d564be93eb8501320098b1b82f6fb3a501
parent 520496 783a57afe5b8b08a97cdea33bcb9648f0473ba43
child 520498 a28ae4304209412e0cb5256dfa4ef1169c8e42dd
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdao
bugs1531327
milestone67.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 1531327 - Properly handle actions overrides in the Quantum Bar. r=dao This fixes 2 problems causing overrides (with SHIFT/ALT/CTRL) to stick longer than expected: 1. The event bufferer may delay keydown, keyup could then happen before it 2. On some platforms holding a key generates multiple events, so there's no match in number of keydown/keyup Differential Revision: https://phabricator.services.mozilla.com/D21665
browser/components/urlbar/UrlbarEventBufferer.jsm
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/tests/browser/browser.ini
browser/components/urlbar/tests/browser/browser_caret_navigation.js
browser/components/urlbar/tests/browser/browser_switchTab_override.js
browser/components/urlbar/tests/browser/browser_switchToTab_fullUrl_repeatedKeydown.js
browser/components/urlbar/tests/legacy/browser.ini
--- a/browser/components/urlbar/UrlbarEventBufferer.jsm
+++ b/browser/components/urlbar/UrlbarEventBufferer.jsm
@@ -53,17 +53,22 @@ const QUERY_STATUS = {
  */
 class UrlbarEventBufferer {
   /**
    * Initialises the class.
    * @param {UrlbarInput} input The urlbar input object.
    */
   constructor(input) {
     this.input = input;
-    // A queue of deferred events.
+    this.input.inputField.addEventListener("blur", this);
+
+    // A queue of {event, callback} objects representing deferred events.
+    // The callback is invoked when it's the right time to handle the event,
+    // but it may also never be invoked, if the context changed and the event
+    // became obsolete.
     this._eventsQueue = [];
     // If this timer fires, we will unconditionally replay all the deferred
     // events so that, after a certain point, we don't keep blocking the user's
     // actions, when nothing else has caused the events to be replayed.
     // At that point we won't check whether it's safe to replay the events,
     // because otherwise it may look like we ignored the user's actions.
     this._deferringTimeout = null;
 
@@ -105,123 +110,120 @@ class UrlbarEventBufferer {
 
   onQueryFinished(queryContext) {
     this._lastQuery.status = QUERY_STATUS.COMPLETE;
   }
 
   onQueryResults(queryContext) {
     this._lastQuery.results = queryContext.results;
     // Ensure this runs after other results handling code.
-    Services.tm.dispatchToMainThread(this.replaySafeDeferredEvents.bind(this));
+    Services.tm.dispatchToMainThread(() => {
+      this.replayDeferredEvents(true);
+    });
   }
 
   /**
-   * Receives DOM events, eventually queues them up, and passes them back to the
-   * input object when it's the right time.
+   * Handles DOM events.
    * @param {Event} event DOM event from the <textbox>.
-   * @returns {boolean}
    */
   handleEvent(event) {
-    switch (event.type) {
-      case "keydown":
-        if (this.shouldDeferEvent(event)) {
-          this.deferEvent(event);
-          return false;
-        }
-        break;
-      case "blur":
-        logger.debug("Clearing queue on blur");
-        // The input field was blurred, pending events don't matter anymore.
-        // Clear the timeout and the queue.
-        this._eventsQueue.length = 0;
-        if (this._deferringTimeout) {
-          clearTimeout(this._deferringTimeout);
-          this._deferringTimeout = null;
-        }
-        break;
+    if (event.type == "blur") {
+      logger.debug("Clearing queue on blur");
+      // The input field was blurred, pending events don't matter anymore.
+      // Clear the timeout and the queue.
+      this._eventsQueue.length = 0;
+      if (this._deferringTimeout) {
+        clearTimeout(this._deferringTimeout);
+        this._deferringTimeout = null;
+      }
     }
-    // Just pass back the event to the input object.
-    return this.input.handleEvent(event);
+  }
+
+  /**
+   * Receives DOM events, eventually queues them up, and calls back when it's
+   * the right time to handle the event.
+   * @param {Event} event DOM event from the <textbox>.
+   * @param {Function} callback to be invoked when it's the right time to handle
+   *        the event.
+   */
+  maybeDeferEvent(event, callback) {
+    if (!callback) {
+      throw new Error("Must provide a callback");
+    }
+    if (this.shouldDeferEvent(event)) {
+      this.deferEvent(event, callback);
+      return;
+    }
+    // If it has not been deferred, handle the callback immediately.
+    callback();
   }
 
   /**
    * Adds a deferrable event to the deferred event queue.
    * @param {Event} event The event to defer.
+   * @param {Function} callback to be invoked when it's the right time to handle
+   *        the event.
    */
-  deferEvent(event) {
+  deferEvent(event, callback) {
     // TODO: once one-off buttons are implemented, figure out if the following
     // is true for the quantum bar as well: somehow event.defaultPrevented ends
     // up true for deferred events.  Autocomplete ignores defaultPrevented
     // events, which means it would ignore replayed deferred events if we didn't
     // tell it to bypass defaultPrevented through urlbarDeferred.
     // Check we don't try to defer events more than once.
     if (event.urlbarDeferred) {
       throw new Error(`Event ${event.type}:${event.keyCode} already deferred!`);
     }
     logger.debug(`Deferring ${event.type}:${event.keyCode} event`);
     // Mark the event as deferred.
     event.urlbarDeferred = true;
     // Also store the current search string, as an added safety check. If the
     // string will differ later, the event is stale and should be dropped.
     event.searchString = this._lastQuery.searchString;
-    this._eventsQueue.push(event);
+    this._eventsQueue.push({event, callback});
 
     if (!this._deferringTimeout) {
       let elapsed = Cu.now() - this._lastQuery.startDate;
       let remaining = DEFERRING_TIMEOUT_MS - elapsed;
       this._deferringTimeout = setTimeout(() => {
-        this.replayAllDeferredEvents();
+        this.replayDeferredEvents(false);
         this._deferringTimeout = null;
       }, Math.max(0, remaining));
     }
   }
 
   /**
-   * Unconditionally replays all deferred key events.  This does not check
-   * whether it's safe to replay the events; use replaySafeDeferredEvents
-   * for that.  Use this method when you must replay all events, so that it
-   * does not appear that we ignored the user's input.
+   * Replays deferred key events.
+   * @param {boolean} onlyIfSafe replays only if it's a safe time to do so.
+   *        Setting this to false will replay all the queue events, without any
+   *        checks, that is something we want to do only if the deferring
+   *        timeout elapsed, and we don't want to appear ignoring user's input.
    */
-  replayAllDeferredEvents() {
-    let event = this._eventsQueue.shift();
-    if (!event) {
-      return;
+  replayDeferredEvents(onlyIfSafe) {
+    if (typeof onlyIfSafe != "boolean") {
+      throw new Error("Must provide a boolean argument");
     }
-    this.replayDeferredEvent(event);
-    Services.tm.dispatchToMainThread(this.replayAllDeferredEvents.bind(this));
-  }
-
-  /**
-   * Replays deferred events if it's a safe time to do it.
-   * @see isSafeToPlayDeferredEvent
-   */
-  replaySafeDeferredEvents() {
     if (!this._eventsQueue.length) {
       return;
     }
-    let event = this._eventsQueue[0];
-    if (!this.isSafeToPlayDeferredEvent(event)) {
+
+    let {event, callback} = this._eventsQueue[0];
+    if (onlyIfSafe && !this.isSafeToPlayDeferredEvent(event)) {
       return;
     }
+
     // Remove the event from the queue and play it.
     this._eventsQueue.shift();
-    this.replayDeferredEvent(event);
-    // Continue with the next event.
-    Services.tm.dispatchToMainThread(this.replaySafeDeferredEvents.bind(this));
-  }
-
-  /**
-   * Replays a deferred event.
-   * @param {Event} event The deferred event to replay.
-   */
-  replayDeferredEvent(event) {
-    // Safety check: handle only if the search string didn't change.
+    // Safety check: handle only if the search string didn't change meanwhile.
     if (event.searchString == this._lastQuery.searchString) {
-      this.input.handleEvent(event);
+      callback();
     }
+    Services.tm.dispatchToMainThread(() => {
+      this.replayDeferredEvents(onlyIfSafe);
+    });
   }
 
   /**
    * Checks whether a given event should be deferred
    * @param {Event} event The event that should maybe be deferred.
    * @returns {boolean} Whether the event should be deferred.
    */
   shouldDeferEvent(event) {
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -125,28 +125,28 @@ class UrlbarInput {
         },
       });
     }
 
     XPCOMUtils.defineLazyGetter(this, "valueFormatter", () => {
       return new UrlbarValueFormatter(this);
     });
 
-    // The event bufferer handles some events, queues them up, and calls back
-    // our handleEvent at the right time.
+    // The event bufferer can be used to defer events that may affect users
+    // muscle memory; for example quickly pressing DOWN+ENTER should end up
+    // on a predictable result, regardless of the search status. The event
+    // bufferer will invoke the handling code at the right time.
     this.eventBufferer = new UrlbarEventBufferer(this);
-    this.inputField.addEventListener("blur", this.eventBufferer);
-    this.inputField.addEventListener("keydown", this.eventBufferer);
 
-    const inputFieldEvents = [
-      "focus", "input", "keyup", "mouseover", "paste", "scrollend", "select",
-      "overflow", "underflow", "dragstart", "dragover", "drop",
-      "compositionstart", "compositionend",
+    this._inputFieldEvents = [
+      "blur", "focus", "input", "keydown", "keyup", "mouseover", "paste",
+      "scrollend", "select", "overflow", "underflow", "dragstart", "dragover",
+      "drop", "compositionstart", "compositionend",
     ];
-    for (let name of inputFieldEvents) {
+    for (let name of this._inputFieldEvents) {
       this.inputField.addEventListener(name, this);
     }
 
     this.addEventListener("mousedown", this);
     this.view.panel.addEventListener("popupshowing", this);
     this.view.panel.addEventListener("popuphidden", this);
 
     this.inputField.controllers.insertControllerAt(0, new CopyCutController(this));
@@ -155,34 +155,28 @@ class UrlbarInput {
     // Tracks IME composition.
     this._compositionState == UrlbarUtils.COMPOSITION.NONE;
   }
 
   /**
    * Uninitializes this input object, detaching it from the inputField.
    */
   uninit() {
-    this.inputField.removeEventListener("blur", this.eventBufferer);
-    this.inputField.removeEventListener("keydown", this.eventBufferer);
-    delete this.eventBufferer;
-    const inputFieldEvents = [
-      "focus", "input", "keyup", "mouseover", "paste", "scrollend", "select",
-      "overflow", "underflow", "dragstart", "dragover", "drop",
-    ];
-    for (let name of inputFieldEvents) {
+    for (let name of this._inputFieldEvents) {
       this.inputField.removeEventListener(name, this);
     }
     this.removeEventListener("mousedown", this);
 
     this.view.panel.remove();
 
     this.inputField.controllers.removeControllerAt(0);
 
     delete this.document;
     delete this.window;
+    delete this.eventBufferer;
     delete this.valueFormatter;
     delete this.panel;
     delete this.view;
     delete this.controller;
     delete this.textbox;
   }
 
   /**
@@ -742,16 +736,20 @@ class UrlbarInput {
       let trimmedSegments = spec.split(trimmedSpec);
       selectedVal = trimmedSegments[0] + selectedVal;
     }
 
     return selectedVal;
   }
 
   _toggleActionOverride(event) {
+    // Ignore repeated KeyboardEvents.
+    if (event.repeat) {
+      return;
+    }
     if (event.keyCode == KeyEvent.DOM_VK_SHIFT ||
         event.keyCode == KeyEvent.DOM_VK_ALT ||
         event.keyCode == (AppConstants.platform == "macosx" ?
                             KeyEvent.DOM_VK_META :
                             KeyEvent.DOM_VK_CONTROL)) {
       if (event.type == "keydown") {
         this._actionOverrideKeyCount++;
         this.setAttribute("actionoverride", "true");
@@ -1162,18 +1160,20 @@ class UrlbarInput {
     this._updateTextOverflow();
   }
 
   _on_TabSelect(event) {
     this.controller.viewContextChanged();
   }
 
   _on_keydown(event) {
-    this.controller.handleKeyNavigation(event);
     this._toggleActionOverride(event);
+    this.eventBufferer.maybeDeferEvent(event, () => {
+      this.controller.handleKeyNavigation(event);
+    });
   }
 
   _on_keyup(event) {
     this._toggleActionOverride(event);
   }
 
   _on_compositionstart(event) {
     if (this._compositionState == UrlbarUtils.COMPOSITION.COMPOSING) {
--- a/browser/components/urlbar/tests/browser/browser.ini
+++ b/browser/components/urlbar/tests/browser/browser.ini
@@ -66,16 +66,17 @@ support-files =
 support-files =
   slow-page.sjs
 [browser_switchTab_closesUrlbarPopup.js]
 [browser_switchTab_decodeuri.js]
 [browser_switchTab_override.js]
 skip-if = ((os == 'win') && verify && debug)
 [browser_switchToTab_closes_newtab.js]
 skip-if = true # Bug 1531737 - fails when run after browser_switchTab_override.js
+[browser_switchToTab_fullUrl_repeatedKeydown.js]
 [browser_switchToTabHavingURI_aOpenParams.js]
 [browser_tabMatchesInAwesomebar_perwindowpb.js]
 skip-if = os == 'linux' # Bug 1104755 (Intermittent failure)
 [browser_tabMatchesInAwesomebar.js]
 support-files =
   moz.png
 [browser_urlbar_blanking.js]
 support-files =
--- a/browser/components/urlbar/tests/browser/browser_caret_navigation.js
+++ b/browser/components/urlbar/tests/browser/browser_caret_navigation.js
@@ -59,13 +59,13 @@ function checkIfKeyStartsQuery(key, shou
   let queryStarted = false;
   let queryListener = {
     onQueryStarted() {
       queryStarted = true;
     },
   };
   gURLBar.controller.addQueryListener(queryListener);
   EventUtils.synthesizeKey(key);
-  gURLBar.eventBufferer.replayAllDeferredEvents();
+  gURLBar.eventBufferer.replayDeferredEvents(false);
   gURLBar.controller.removeQueryListener(queryListener);
   Assert.equal(queryStarted, shouldStartQuery,
                `${key}: Should${shouldStartQuery ? "" : "n't"} have started a query`);
 }
--- a/browser/components/urlbar/tests/browser/browser_switchTab_override.js
+++ b/browser/components/urlbar/tests/browser/browser_switchTab_override.js
@@ -45,13 +45,16 @@ add_task(async function test_switchtab_o
     gBrowser.tabContainer.removeEventListener("TabSelect", onTabSelect);
   });
   // Otherwise it would load the page.
   BrowserTestUtils.browserLoaded(secondTab.linkedBrowser).then(deferred.resolve);
 
   EventUtils.synthesizeKey("KEY_Shift", {type: "keydown"});
   EventUtils.synthesizeKey("KEY_Enter");
   info(`gURLBar.value = ${gURLBar.value}`);
+  await deferred.promise;
+  // Loading the page may move focus, thus ensure the urlbar receives the keyup
+  // event, to avoid confusing next tests.
+  gURLBar.focus();
   EventUtils.synthesizeKey("KEY_Shift", {type: "keyup"});
-  await deferred.promise;
 
   await PlacesUtils.history.clear();
 });
new file mode 100644
--- /dev/null
+++ b/browser/components/urlbar/tests/browser/browser_switchToTab_fullUrl_repeatedKeydown.js
@@ -0,0 +1,45 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/**
+ * This tests that typing a url and picking a switch to tab actually switches
+ * to the right tab. Also tests repeated keydown/keyup events don't confuse
+ * override.
+ */
+
+"use strict";
+
+add_task(async function test_switchToTab_url() {
+  const TEST_URL = "https://example.org/browser/";
+
+  let baseTab = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL);
+  let testTab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
+
+  // Functions for TabClose and TabSelect
+  let tabClosePromise = BrowserTestUtils.waitForEvent(gBrowser.tabContainer,
+    "TabClose", false, event => event.target == testTab);
+  let tabSelectPromise = BrowserTestUtils.waitForEvent(gBrowser.tabContainer,
+    "TabSelect", false, event => event.target == baseTab);
+
+  await UrlbarTestUtils.promiseAutocompleteResultPopup(window, TEST_URL, waitForFocus, true);
+  // The first result is the heuristic, the second will be the switch to tab.
+  await UrlbarTestUtils.waitForAutocompleteResultAt(window, 1);
+
+  // Simulate a long press, on some platforms (Windows) it can generate multiple
+  // keydown events.
+  EventUtils.synthesizeKey("VK_SHIFT", { type: "keydown", repeat: 3 });
+  EventUtils.synthesizeKey("VK_SHIFT", { type: "keyup" });
+
+  // Pick the switch to tab result.
+  EventUtils.synthesizeKey("KEY_ArrowDown");
+  EventUtils.synthesizeKey("KEY_Enter");
+
+  await Promise.all([tabSelectPromise, tabClosePromise]);
+
+  // Confirm that the selected tab is now the base tab
+  Assert.equal(gBrowser.selectedTab, baseTab,
+    "Should have switched to the correct tab");
+
+  gBrowser.removeTab(baseTab);
+});
--- a/browser/components/urlbar/tests/legacy/browser.ini
+++ b/browser/components/urlbar/tests/legacy/browser.ini
@@ -87,16 +87,18 @@ support-files =
 [../browser/browser_stop_pending.js]
 support-files =
   ../browser/slow-page.sjs
 [../browser/browser_switchTab_closesUrlbarPopup.js]
 [../browser/browser_switchTab_decodeuri.js]
 [../browser/browser_switchTab_override.js]
 skip-if = ((os == 'win') && verify && debug)
 [../browser/browser_switchToTab_closes_newtab.js]
+[../browser/browser_switchToTab_fullUrl_repeatedKeydown.js]
+skip-if = true # Bug 1507755
 [../browser/browser_switchToTabHavingURI_aOpenParams.js]
 [../browser/browser_tabMatchesInAwesomebar_perwindowpb.js]
 skip-if = os == 'linux' # Bug 1104755
 [../browser/browser_tabMatchesInAwesomebar.js]
 support-files =
   ../browser/moz.png
 [../browser/browser_urlbarAboutHomeLoading.js]
 [../browser/browser_urlbarCopying.js]