Bug 1438329 - Problem(s) with delayed key handling in urlbar. r?mak
MozReview-Commit-ID: CwvnsLJGgG6
--- 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><;
}
- 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.