Bug 1532738 - Fix the expected outcome of browser_caret_navigation.js. r=dao
☠☠ backed out by ca7e5845a895 ☠ ☠
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 04 Apr 2019 09:09:33 +0000
changeset 467961 278767179a87ed8d32d87cf70a44fe70d9ba6ef4
parent 467960 a9aed50dd3ebc0c725736bad607845928cfbd60b
child 467962 58a240bea3cfbfe088f4bdf5ca9bce8e14c0517c
push id112667
push useraiakab@mozilla.com
push dateThu, 04 Apr 2019 16:12:45 +0000
treeherdermozilla-inbound@230bb363f2f3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdao
bugs1532738
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 1532738 - Fix the expected outcome of browser_caret_navigation.js. r=dao Differential Revision: https://phabricator.services.mozilla.com/D25978
browser/components/urlbar/UrlbarController.jsm
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/tests/browser/browser_caret_navigation.js
--- a/browser/components/urlbar/UrlbarController.jsm
+++ b/browser/components/urlbar/UrlbarController.jsm
@@ -230,89 +230,107 @@ class UrlbarController {
   }
 
   /**
    * Receives keyboard events from the input and handles those that should
    * navigate within the view or pick the currently selected item.
    *
    * @param {KeyboardEvent} event
    *   The DOM KeyboardEvent.
+   * @param {boolean} executeAction
+   *   Whether the event should actually execute the associated action, or just
+   *   be managed (at a preventDefault() level). This is used when the event
+   *   will be deferred by the event bufferer, but preventDefault() and friends
+   *   should still happen synchronously.
    */
-  handleKeyNavigation(event) {
+  handleKeyNavigation(event, executeAction = true) {
     const isMac = AppConstants.platform == "macosx";
     // Handle readline/emacs-style navigation bindings on Mac.
     if (isMac &&
         this.view.isOpen &&
         event.ctrlKey &&
         (event.key == "n" || event.key == "p")) {
-      this.view.selectBy(1, { reverse: event.key == "p" });
+      if (executeAction) {
+        this.view.selectBy(1, { reverse: event.key == "p" });
+      }
       event.preventDefault();
       return;
     }
 
-    if (this.view.isOpen) {
+    if (this.view.isOpen && executeAction) {
       let queryContext = this._lastQueryContext;
       if (queryContext) {
         this.view.oneOffSearchButtons.handleKeyPress(
           event,
           queryContext.results.length,
           this.view.allowEmptySelection,
           queryContext.searchString);
         if (event.defaultPrevented) {
           return;
         }
       }
     }
 
     switch (event.keyCode) {
       case KeyEvent.DOM_VK_ESCAPE:
-        this.input.handleRevert();
+        if (executeAction) {
+          this.input.handleRevert();
+        }
         event.preventDefault();
         break;
       case KeyEvent.DOM_VK_RETURN:
         if (isMac &&
             event.metaKey) {
           // Prevent beep on Mac.
           event.preventDefault();
         }
-        this.input.handleCommand(event);
+        if (executeAction) {
+          this.input.handleCommand(event);
+        }
         break;
       case KeyEvent.DOM_VK_TAB:
         if (this.view.isOpen) {
-          this.view.selectBy(1, { reverse: event.shiftKey });
-          this.userSelectionBehavior = "tab";
+          if (executeAction) {
+            this.view.selectBy(1, { reverse: event.shiftKey });
+            this.userSelectionBehavior = "tab";
+          }
           event.preventDefault();
         }
         break;
       case KeyEvent.DOM_VK_DOWN:
       case KeyEvent.DOM_VK_UP:
       case KeyEvent.DOM_VK_PAGE_DOWN:
       case KeyEvent.DOM_VK_PAGE_UP:
         if (event.ctrlKey || event.altKey) {
           break;
         }
         if (this.view.isOpen) {
-          this.userSelectionBehavior = "arrow";
-          this.view.selectBy(
-            event.keyCode == KeyEvent.DOM_VK_PAGE_DOWN ||
-            event.keyCode == KeyEvent.DOM_VK_PAGE_UP ?
-              5 : 1,
-            { reverse: event.keyCode == KeyEvent.DOM_VK_UP ||
-                       event.keyCode == KeyEvent.DOM_VK_PAGE_UP });
+          if (executeAction) {
+            this.userSelectionBehavior = "arrow";
+            this.view.selectBy(
+              event.keyCode == KeyEvent.DOM_VK_PAGE_DOWN ||
+              event.keyCode == KeyEvent.DOM_VK_PAGE_UP ?
+                5 : 1,
+              { reverse: event.keyCode == KeyEvent.DOM_VK_UP ||
+                        event.keyCode == KeyEvent.DOM_VK_PAGE_UP });
+          }
         } else {
           if (this.keyEventMovesCaret(event)) {
             break;
           }
-          this.input.startQuery();
+          if (executeAction) {
+            this.input.startQuery();
+          }
         }
         event.preventDefault();
         break;
       case KeyEvent.DOM_VK_DELETE:
       case KeyEvent.DOM_VK_BACK_SPACE:
-        if (event.shiftKey && this.view.isOpen && this._handleDeleteEntry()) {
+        if (event.shiftKey && this.view.isOpen &&
+            (!executeAction || this._handleDeleteEntry())) {
           event.preventDefault();
         }
         break;
     }
   }
 
   /**
    * Tries to initialize a speculative connection on a result.
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -1370,16 +1370,26 @@ class UrlbarInput {
   }
 
   _on_TabSelect(event) {
     this._resetSearchState();
     this.controller.viewContextChanged();
   }
 
   _on_keydown(event) {
+    // Due to event deferring, it's possible preventDefault() won't be invoked
+    // soon enough to actually prevent some of the default behaviors, thus we
+    // have to handle the event "twice". This first immediate call passes false
+    // as second argument so that handleKeyNavigation will only simulate the
+    // event handling, without actually executing actions.
+    // TODO (Bug 1541806): improve this handling, maybe by delaying actions
+    // instead of events.
+    if (this.eventBufferer.shouldDeferEvent(event)) {
+      this.controller.handleKeyNavigation(event, false);
+    }
     this._toggleActionOverride(event);
     this.eventBufferer.maybeDeferEvent(event, () => {
       this.controller.handleKeyNavigation(event);
     });
   }
 
   _on_keyup(event) {
     this._toggleActionOverride(event);
--- a/browser/components/urlbar/tests/browser/browser_caret_navigation.js
+++ b/browser/components/urlbar/tests/browser/browser_caret_navigation.js
@@ -15,48 +15,60 @@ add_task(async function() {
   const INITIAL_SELECTION_START = 3;
   const INITIAL_SELECTION_END = 10;
   gURLBar.selectionStart = INITIAL_SELECTION_START;
   gURLBar.selectionEnd = INITIAL_SELECTION_END;
 
   if (AppConstants.platform == "macosx" ||
       AppConstants.platform == "linux") {
     if (AppConstants.platform == "linux") {
-      await checkCaretMoves("KEY_ArrowUp", INITIAL_SELECTION_START, "Selection should be collapsed to its start");
+      await checkCaretMoves("KEY_ArrowUp", INITIAL_SELECTION_START,
+                            "Selection should be collapsed to its start");
 
       gURLBar.selectionStart = INITIAL_SELECTION_START;
       gURLBar.selectionEnd = INITIAL_SELECTION_END;
-      await checkCaretMoves("KEY_ArrowDown", INITIAL_SELECTION_END, "Selection should be collapsed to its end");
+      await checkCaretMoves("KEY_ArrowDown", INITIAL_SELECTION_END,
+                            "Selection should be collapsed to its end");
     }
 
-    await checkCaretMoves("KEY_ArrowDown", gURLBar.textValue.length, "Caret should have moved to the end");
-    await checkPopupOpens("KEY_ArrowDown", gURLBar.textValue.length);
+    await checkCaretMoves("KEY_ArrowDown", gURLBar.textValue.length,
+                          "Caret should have moved to the end");
+    await checkPopupOpens("KEY_ArrowDown");
 
-    await checkCaretMoves("KEY_ArrowUp", 0, "Caret should have moved to the start");
-    await checkPopupOpens("KEY_ArrowUp", 0);
+    await checkCaretMoves("KEY_ArrowUp", 0,
+                          "Caret should have moved to the start");
+    await checkPopupOpens("KEY_ArrowUp");
   } else {
-    await checkPopupOpens("KEY_ArrowDown", gURLBar.textValue.length);
-    await checkPopupOpens("KEY_ArrowUp", gURLBar.textValue.length);
+    await checkPopupOpens("KEY_ArrowDown");
+    await checkPopupOpens("KEY_ArrowUp");
   }
 });
 
 async function checkCaretMoves(key, pos, msg) {
   checkIfKeyStartsQuery(key, false);
-  Assert.equal(UrlbarTestUtils.isPopupOpen(window), false, `${key}: Popup shouldn't be open`);
-  Assert.equal(gURLBar.selectionStart, gURLBar.selectionEnd, `${key}: Input selection should be empty`);
+  Assert.equal(UrlbarTestUtils.isPopupOpen(window), false,
+               `${key}: Popup shouldn't be open`);
+  Assert.equal(gURLBar.selectionStart, gURLBar.selectionEnd,
+               `${key}: Input selection should be empty`);
   Assert.equal(gURLBar.selectionStart, pos, `${key}: ${msg}`);
 }
 
-async function checkPopupOpens(key, expectedCaretPosition) {
+async function checkPopupOpens(key) {
+  // Store current selection and check it doesn't change.
+  let selectionStart = gURLBar.selectionStart;
+  let selectionEnd = gURLBar.selectionEnd;
   await UrlbarTestUtils.promisePopupOpen(window, () => {
     checkIfKeyStartsQuery(key, true);
   });
-  Assert.equal(UrlbarTestUtils.getSelectedIndex(window), 0, `${key}: Heuristic result should be selected`);
-  Assert.equal(gURLBar.selectionStart, gURLBar.selectionEnd, `${key}: Input selection should be empty`);
-  Assert.equal(gURLBar.selectionStart, expectedCaretPosition, `${key}: Caret is at the expected position`);
+  Assert.equal(UrlbarTestUtils.getSelectedIndex(window), 0,
+               `${key}: Heuristic result should be selected`);
+  Assert.equal(gURLBar.selectionStart, selectionStart,
+               `${key}: Input selection start should not change`);
+  Assert.equal(gURLBar.selectionEnd, selectionEnd,
+               `${key}: Input selection end should not change`);
   await UrlbarTestUtils.promisePopupClose(window);
 }
 
 function checkIfKeyStartsQuery(key, shouldStartQuery) {
   let queryStarted = false;
   let queryListener = {
     onQueryStarted() {
       queryStarted = true;