Backed out changeset 278767179a87 (bug 1532738) for turning bug 1352679 into permafail.
authorCosmin Sabou <csabou@mozilla.com>
Thu, 04 Apr 2019 19:13:27 +0300
changeset 468029 ca7e5845a89512d93f2106b33d94ab9006230e76
parent 468028 220cb2c1a0e5d563cae87af10d3d675223b5398f
child 468030 33510bccced0c4691cfbe8214a1ca404bb401271
push id112673
push userccoroiu@mozilla.com
push dateThu, 04 Apr 2019 22:20:03 +0000
treeherdermozilla-inbound@f0fb97a222ff [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1532738, 1352679
milestone68.0a1
backs out278767179a87ed8d32d87cf70a44fe70d9ba6ef4
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
Backed out changeset 278767179a87 (bug 1532738) for turning bug 1352679 into permafail.
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,107 +230,89 @@ 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, executeAction = true) {
+  handleKeyNavigation(event) {
     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")) {
-      if (executeAction) {
-        this.view.selectBy(1, { reverse: event.key == "p" });
-      }
+      this.view.selectBy(1, { reverse: event.key == "p" });
       event.preventDefault();
       return;
     }
 
-    if (this.view.isOpen && executeAction) {
+    if (this.view.isOpen) {
       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:
-        if (executeAction) {
-          this.input.handleRevert();
-        }
+        this.input.handleRevert();
         event.preventDefault();
         break;
       case KeyEvent.DOM_VK_RETURN:
         if (isMac &&
             event.metaKey) {
           // Prevent beep on Mac.
           event.preventDefault();
         }
-        if (executeAction) {
-          this.input.handleCommand(event);
-        }
+        this.input.handleCommand(event);
         break;
       case KeyEvent.DOM_VK_TAB:
         if (this.view.isOpen) {
-          if (executeAction) {
-            this.view.selectBy(1, { reverse: event.shiftKey });
-            this.userSelectionBehavior = "tab";
-          }
+          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) {
-          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 });
-          }
+          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;
           }
-          if (executeAction) {
-            this.input.startQuery();
-          }
+          this.input.startQuery();
         }
         event.preventDefault();
         break;
       case KeyEvent.DOM_VK_DELETE:
       case KeyEvent.DOM_VK_BACK_SPACE:
-        if (event.shiftKey && this.view.isOpen &&
-            (!executeAction || this._handleDeleteEntry())) {
+        if (event.shiftKey && this.view.isOpen && 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,26 +1370,16 @@ 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,60 +15,48 @@ 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");
+    await checkCaretMoves("KEY_ArrowDown", gURLBar.textValue.length, "Caret should have moved to the end");
+    await checkPopupOpens("KEY_ArrowDown", gURLBar.textValue.length);
 
-    await checkCaretMoves("KEY_ArrowUp", 0,
-                          "Caret should have moved to the start");
-    await checkPopupOpens("KEY_ArrowUp");
+    await checkCaretMoves("KEY_ArrowUp", 0, "Caret should have moved to the start");
+    await checkPopupOpens("KEY_ArrowUp", 0);
   } else {
-    await checkPopupOpens("KEY_ArrowDown");
-    await checkPopupOpens("KEY_ArrowUp");
+    await checkPopupOpens("KEY_ArrowDown", gURLBar.textValue.length);
+    await checkPopupOpens("KEY_ArrowUp", gURLBar.textValue.length);
   }
 });
 
 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) {
-  // Store current selection and check it doesn't change.
-  let selectionStart = gURLBar.selectionStart;
-  let selectionEnd = gURLBar.selectionEnd;
+async function checkPopupOpens(key, expectedCaretPosition) {
   await UrlbarTestUtils.promisePopupOpen(window, () => {
     checkIfKeyStartsQuery(key, true);
   });
-  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`);
+  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`);
   await UrlbarTestUtils.promisePopupClose(window);
 }
 
 function checkIfKeyStartsQuery(key, shouldStartQuery) {
   let queryStarted = false;
   let queryListener = {
     onQueryStarted() {
       queryStarted = true;