Bug 1438329 - Problem(s) with delayed key handling in urlbar. r?mak draft
authorDrew Willcoxon <adw@mozilla.com>
Fri, 16 Feb 2018 11:13:21 -0800
changeset 756277 589c4a5cc57ad5c7a913503d6abb2925d78a157b
parent 754948 375d162649d2fba6cf967b1062163cb87790dd87
push id99456
push userbmo:adw@mozilla.com
push dateFri, 16 Feb 2018 19:34:06 +0000
reviewersmak
bugs1438329
milestone60.0a1
Bug 1438329 - Problem(s) with delayed key handling in urlbar. r?mak MozReview-Commit-ID: CwvnsLJGgG6
browser/base/content/test/urlbar/browser_autocomplete_enter_race.js
browser/base/content/urlbarBindings.xml
--- a/browser/base/content/test/urlbar/browser_autocomplete_enter_race.js
+++ b/browser/base/content/test/urlbar/browser_autocomplete_enter_race.js
@@ -13,24 +13,16 @@ add_task(async function setup() {
   });
   await PlacesUtils.keywords.insert({ keyword: "keyword",
                                       url: "http://example.com/?q=%s" });
   // Needs at least one success.
   ok(true, "Setup complete");
 });
 
 add_task(async function test_keyword() {
-  // This is set because we see (undiagnosed) test timeouts without it
-  let timerPrecision = Preferences.get("privacy.reduceTimerPrecision");
-  Preferences.set("privacy.reduceTimerPrecision", false);
-
-  registerCleanupFunction(function() {
-    Preferences.set("privacy.reduceTimerPrecision", timerPrecision);
-  });
-
   await promiseAutocompleteResultPopup("keyword bear");
   gURLBar.focus();
   EventUtils.synthesizeKey("d", {});
   EventUtils.synthesizeKey("VK_RETURN", {});
   info("wait for the page to load");
   await BrowserTestUtils.browserLoaded(gBrowser.selectedTab.linkedBrowser,
                                       false, "http://example.com/?q=beard");
 });
@@ -103,27 +95,18 @@ add_task(async function test_disabled_ac
   await cleanup();
 });
 
 add_task(async function test_delay() {
   const TIMEOUT = 10000;
   // Set a large delay.
   let delay = Preferences.get("browser.urlbar.delay");
   Preferences.set("browser.urlbar.delay", TIMEOUT);
-  // This may be a real test regression on Bug 1283329, if we can get it to
-  // fail at a realistic 2ms.
-  let timerPrecision = Preferences.get("privacy.reduceTimerPrecision");
-  Preferences.set("privacy.reduceTimerPrecision", true);
-  let timerPrecisionUSec = Preferences.get("privacy.resistFingerprinting.reduceTimerPrecision.microseconds");
-  Preferences.set("privacy.resistFingerprinting.reduceTimerPrecision.microseconds", 2000);
-
   registerCleanupFunction(function() {
     Preferences.set("browser.urlbar.delay", delay);
-    Preferences.set("privacy.reduceTimerPrecision", timerPrecision);
-    Preferences.set("privacy.resistFingerprinting.reduceTimerPrecision.microseconds", timerPrecisionUSec);
   });
 
   // This is needed to clear the current value, otherwise autocomplete may think
   // the user removed text from the end.
   let start = Date.now();
   await promiseAutocompleteResultPopup("");
   Assert.ok((Date.now() - start) < TIMEOUT);
 
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -261,18 +261,17 @@ file, You can obtain one at http://mozil
             case KeyEvent.DOM_VK_RIGHT:
             case KeyEvent.DOM_VK_HOME:
               // Reset the selected index so that nsAutoCompleteController
               // simply closes the popup without trying to fill anything.
               this.popup.selectedIndex = -1;
               break;
           }
           if (!this.popup.disableKeyNavigation) {
-            if (this._keyCodesToDefer.has(aEvent.keyCode) &&
-                this._shouldDeferKeyEvent(aEvent)) {
+            if (this._shouldDeferKeyEvent(aEvent)) {
               this._deferKeyEvent(aEvent, "onKeyPress");
               return false;
             }
             if (this.popup.popupOpen && this.popup.handleKeyPress(aEvent)) {
               return true;
             }
           }
           return this.handleKeyPress(aEvent);
@@ -286,43 +285,96 @@ file, You can obtain one at http://mozil
         enter keys due to the one-off search buttons: if the user very quickly
         pastes something in the input, presses the down arrow key, and then hits
         enter, they are probably expecting to visit the first result.  But if
         there are no results, then pressing down and enter will trigger the
         first one-off button.  To prevent that undesirable behavior, certain
         keys are buffered and deferred until more results arrive, at which time
         they're replayed.
 
-        @param event
-               The key event that should maybe be deferred.  You can pass null
-               or undefined if you don't have one to see whether the next key
-               event in general should be deferred.
+        @param  event
+                The key event that should maybe be deferred.
         @return True if the event should be deferred, false if not.
        -->
       <method name="_shouldDeferKeyEvent">
         <parameter name="event"/>
         <body><![CDATA[
+          // If any event has been deferred for this search, then defer all
+          // subsequent events so that the user does not experience any
+          // keypresses out of order.  All events will be replayed when this
+          // timeout fires.
+          if (this._deferredKeyEventTimeout) {
+            return true;
+          }
+
+          // At this point, no events have been deferred for this search, and we
+          // need to decide whether `event` is the first one that should be.
+
+          if (!this._keyCodesToDefer.has(event.keyCode)) {
+            // Not a key that should trigger deferring.
+            return false;
+          }
+
           let waitedLongEnough =
-            this._searchStartDate + this._deferredKeyEventTimeoutMs < Date.now();
-          if (waitedLongEnough && !this._deferredKeyEventTimeout) {
+            this._searchStartDate + this._deferredKeyEventTimeoutMs <= Cu.now();
+          if (waitedLongEnough) {
+            // This is a key that we would defer, but enough time has passed
+            // since the start of the search that we don't want to block the
+            // user's keypresses anymore.
+            return false;
+          }
+
+          if (event.keyCode == KeyEvent.DOM_VK_TAB && !this.popupOpen) {
+            // The popup is closed and the user pressed the Tab key.  The
+            // focus should move out of the urlbar immediately.
             return false;
           }
-          if (event && event.keyCode == KeyEvent.DOM_VK_TAB && !this.popupOpen) {
-            // In this case, the popup is closed and the user pressed the Tab
-            // key.  The focus should move out of the urlbar immediately.
+
+          return !this._safeToPlayDeferredKeyEvent(event);
+        ]]></body>
+      </method>
+
+      <!--
+        Returns true if the given deferred key event can be played now without
+        possibly surprising the user.  This depends on the state of the popup,
+        its results, and the type of keypress.  Use this method only after
+        determining that the event should be deferred, or after it's already
+        been deferred and you want to know if it can be played now.
+
+        @param  event
+                The key event.
+        @return True if the event can be played, false if not.
+      -->
+      <method name="_safeToPlayDeferredKeyEvent">
+        <parameter name="event"/>
+        <body><![CDATA[
+          if (!this.gotResultForCurrentQuery || !this.popupOpen) {
+            // We're still waiting on the first result, or the popup hasn't
+            // opened yet, so not safe.
             return false;
           }
-          if (!this.gotResultForCurrentQuery || !this.popupOpen) {
+
+          let maxResultsRemaining =
+            this.popup.maxResults - this.popup.matchCount;
+          if (maxResultsRemaining == 0) {
+            // The popup can't possibly have any more results, so there's no
+            // need to defer any event now.
             return true;
           }
-          let maxResultsRemaining =
-            this.popup.maxResults - this.popup.matchCount;
-          let lastResultSelected =
-            this.popup.selectedIndex + 1 == this.popup.matchCount;
-          return maxResultsRemaining > 0 && lastResultSelected;
+
+          if (event.keyCode == KeyEvent.DOM_VK_DOWN) {
+            // Don't play the event if the last result is selected so that the
+            // user doesn't accidentally arrow down into the one-off buttons
+            // when they didn't mean to.
+            let lastResultSelected =
+              this.popup.selectedIndex + 1 == this.popup.matchCount;
+            return !lastResultSelected;
+          }
+
+          return true;
         ]]></body>
       </method>
 
       <!--
         Adds a key event to the deferred event queue.
 
         @param event
                The key event to defer.
@@ -344,52 +396,89 @@ file, You can obtain one at http://mozil
 
           this._deferredKeyEventQueue.push({
             methodName,
             event,
             searchString: this.mController.searchString,
           });
 
           if (!this._deferredKeyEventTimeout) {
-            this._deferredKeyEventTimeout = setTimeout(() => {
-              this._deferredKeyEventTimeout = null;
-              this.maybeReplayDeferredKeyEvents();
-            }, this._deferredKeyEventTimeoutMs);
+            // Start the timeout that will unconditionally replay all deferred
+            // events when it fires so that, after a certain point, we don't
+            // keep blocking the user's keypresses when nothing else has caused
+            // the events to be replayed.  Do not check whether it's safe to
+            // replay the events because otherwise it may look like we ignored
+            // the user's input.
+            let elapsed = Cu.now() - this._searchStartDate;
+            let remaining = this._deferredKeyEventTimeoutMs - elapsed;
+            if (remaining <= 0) {
+              this.replayAllDeferredKeyEvents();
+            } else {
+              this._deferredKeyEventTimeout = setTimeout(() => {
+                this.replayAllDeferredKeyEvents();
+                this._deferredKeyEventTimeout = null;
+              }, remaining);
+            }
           }
         ]]></body>
       </method>
 
       <!-- The enter key is always deferred, so it's not included here. -->
       <field name="_keyCodesToDefer">new Set([
         KeyboardEvent.DOM_VK_DOWN,
         KeyboardEvent.DOM_VK_TAB,
       ])</field>
       <field name="_deferredKeyEventQueue">[]</field>
       <field name="_deferredKeyEventTimeout">null</field>
       <field name="_deferredKeyEventTimeoutMs">200</field>
       <field name="_searchStartDate">0</field>
 
-      <method name="maybeReplayDeferredKeyEvents">
+      <method name="replaySafeDeferredKeyEvents">
         <body><![CDATA[
-          if (!this._deferredKeyEventQueue.length ||
-              this._shouldDeferKeyEvent()) {
+          if (!this._deferredKeyEventQueue.length) {
+            return;
+          }
+          let instance = this._deferredKeyEventQueue[0];
+          if (!this._safeToPlayDeferredKeyEvent(instance.event)) {
             return;
           }
-          if (this._deferredKeyEventTimeout) {
-            clearTimeout(this._deferredKeyEventTimeout);
-            this._deferredKeyEventTimeout = null;
+          this._deferredKeyEventQueue.shift();
+          this._replayKeyEventInstance(instance);
+          Services.tm.dispatchToMainThread(() => {
+            this.replaySafeDeferredKeyEvents();
+          });
+        ]]></body>
+      </method>
+
+      <!--
+        Unconditionally replays all deferred key events.  This does not check
+        whether it's safe to replay the events; use replaySafeDeferredKeyEvents
+        for that.  Use this method when you must replay all events so that it
+        does not appear that we ignored the user's input.
+      -->
+      <method name="replayAllDeferredKeyEvents">
+        <body><![CDATA[
+          let instance = this._deferredKeyEventQueue.shift();
+          if (!instance) {
+            return;
           }
-          let instance = this._deferredKeyEventQueue.shift();
+          this._replayKeyEventInstance(instance);
+          Services.tm.dispatchToMainThread(() => {
+            this.replayAllDeferredKeyEvents();
+          });
+        ]]></body>
+      </method>
+
+      <method name="_replayKeyEventInstance">
+        <parameter name="instance"/>
+        <body><![CDATA[
           // Safety check: handle only if the search string didn't change.
           if (this.mController.searchString == instance.searchString) {
             this[instance.methodName](instance.event);
           }
-          setTimeout(() => {
-            this.maybeReplayDeferredKeyEvents();
-          });
         ]]></body>
       </method>
 
       <field name="_mayTrimURLs">true</field>
       <method name="trimValue">
         <parameter name="aURL"/>
         <body><![CDATA[
           // This method must not modify the given URL such that calling
@@ -1353,17 +1442,17 @@ file, You can obtain one at http://mozil
             } else {
               this.removeAttribute("usertyping");
             }
             // Only wait for a result when we are sure to get one.  In some
             // cases, like when pasting the same exact text, we may not fire
             // a new search and we won't get a result.
             if (this.mController.handleText()) {
               this.gotResultForCurrentQuery = false;
-              this._searchStartDate = Date.now();
+              this._searchStartDate = Cu.now();
               this._deferredKeyEventQueue = [];
               if (this._deferredKeyEventTimeout) {
                 clearTimeout(this._deferredKeyEventTimeout);
                 this._deferredKeyEventTimeout = null;
               }
             }
           }
           this.resetActionType();
@@ -2207,17 +2296,17 @@ file, You can obtain one at http://mozil
               }
             }
 
             // When a result is present the footer should always be visible.
             this.footer.collapsed = false;
             this.input.tabScrolling = true;
 
             this.input.gotResultForCurrentQuery = true;
-            this.input.maybeReplayDeferredKeyEvents();
+            this.input.replaySafeDeferredKeyEvents();
           ]]>
         </body>
       </method>
 
       <method name="_onSearchBegin">
         <body><![CDATA[
           // Set the selected index to 0 (heuristic) until a result comes back
           // and we can evaluate it better.