Bug 1536521: Use a capturing listener for PanelMultiView's keydown handler. r=Gijs a=pascalc
authorJames Teh <jteh@mozilla.com>
Tue, 26 Mar 2019 10:33:11 +0000
changeset 525752 7b34fc9165b9047186a4bf940d8028aaf0281f3f
parent 525751 affaab9df5b77f8ff5571748e149d39dfa25e27c
child 525753 d0f88c4218dd9ebd852de9d210175e42f4d6f758
push id2032
push userffxbld-merge
push dateMon, 13 May 2019 09:36:57 +0000
treeherdermozilla-release@455c1065dcbe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs, pascalc
bugs1536521
milestone67.0
Bug 1536521: Use a capturing listener for PanelMultiView's keydown handler. r=Gijs a=pascalc PanelMultiView adds the keydown handler on the window so that it handles key presses when a panel appears but doesn't get focus, as happens when a button to open a panel is clicked with the mouse. However, this means the listener is on an ancestor of the panel, which means that handlers such as ToolbarKeyboardNavigator are deeper in the tree. Previously, PanelMultiView used a bubbling (default) listener. This meant that ToolbarKeyboardNavigator handled the event first, causing it to interfere when a panel opened within the toolbar; e.g. the Library menu. To fix this, use a capturing listener for PanelMultiView so it gets the event first. Differential Revision: https://phabricator.services.mozilla.com/D24848
browser/base/content/test/keyboard/browser.ini
browser/base/content/test/keyboard/browser_toolbarButtonKeyPress.js
browser/base/content/test/keyboard/browser_toolbarKeyNav.js
browser/base/content/test/keyboard/head.js
browser/components/customizableui/PanelMultiView.jsm
--- a/browser/base/content/test/keyboard/browser.ini
+++ b/browser/base/content/test/keyboard/browser.ini
@@ -1,3 +1,6 @@
+[DEFAULT]
+support-files = head.js
+
 [browser_toolbarButtonKeyPress.js]
 [browser_toolbarKeyNav.js]
 support-files = !/browser/base/content/test/permissions/permissions.html
--- a/browser/base/content/test/keyboard/browser_toolbarButtonKeyPress.js
+++ b/browser/base/content/test/keyboard/browser_toolbarButtonKeyPress.js
@@ -2,26 +2,16 @@
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
 /**
  * Test the behavior of key presses on various toolbar buttons.
  */
 
-// Toolbar buttons aren't focusable because if they were, clicking them would
-// focus them, which is undesirable. Therefore, they're only made focusable
-// when a user is navigating with the keyboard. This function forces focus as
-// is done during keyboard navigation.
-function forceFocus(aElem) {
-  aElem.setAttribute("tabindex", "-1");
-  aElem.focus();
-  aElem.removeAttribute("tabindex");
-}
-
 function waitForLocationChange() {
   let promise = new Promise(resolve => {
     let wpl = {
       onLocationChange(aWebProgress, aRequest, aLocation) {
         gBrowser.removeProgressListener(wpl);
         resolve();
       },
     };
--- a/browser/base/content/test/keyboard/browser_toolbarKeyNav.js
+++ b/browser/base/content/test/keyboard/browser_toolbarKeyNav.js
@@ -236,9 +236,29 @@ add_task(async function testArrowsOverfl
     await expectFocusAfterKey("ArrowRight", "PanelUI-menu-button");
     resetToolbarWithoutDevEditionButtons();
     // Flush layout so its invisibility can be detected.
     document.getElementById("nav-bar-overflow-button").clientWidth;
     await expectFocusAfterKey("ArrowLeft", "sidebar-button");
   });
 });
 
+// Test that toolbar keyboard navigation doesn't interfere with PanelMultiView
+// keyboard navigation.
+// We do this by opening the Library menu and ensuring that pressing left arrow
+// does nothing.
+add_task(async function testArrowsInPanelMultiView() {
+  let button = document.getElementById("library-button");
+  forceFocus(button);
+  let view = document.getElementById("appMenu-libraryView");
+  let focused = BrowserTestUtils.waitForEvent(view, "focus", true);
+  EventUtils.synthesizeKey(" ");
+  let focusEvt = await focused;
+  ok(true, "Focus inside Library menu after toolbar button pressed");
+  EventUtils.synthesizeKey("KEY_ArrowLeft");
+  is(document.activeElement, focusEvt.target,
+     "ArrowLeft inside panel does nothing");
+  let hidden = BrowserTestUtils.waitForEvent(document, "popuphidden", true);
+  view.closest("panel").hidePopup();
+  await hidden;
+});
+
 registerCleanupFunction(() => CustomizableUI.reset());
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/keyboard/head.js
@@ -0,0 +1,17 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+/**
+ * Force focus to an element that isn't focusable.
+ * Toolbar buttons aren't focusable because if they were, clicking them would
+ * focus them, which is undesirable. Therefore, they're only made focusable
+ * when a user is navigating with the keyboard. This function forces focus as
+ * is done during toolbar keyboard navigation.
+ */
+function forceFocus(aElem) {
+  aElem.setAttribute("tabindex", "-1");
+  aElem.focus();
+  aElem.removeAttribute("tabindex");
+}
--- a/browser/components/customizableui/PanelMultiView.jsm
+++ b/browser/components/customizableui/PanelMultiView.jsm
@@ -407,17 +407,17 @@ var PanelMultiView = class extends Assoc
     if (!this.node || !this.connected)
       return;
 
     this._panel.removeEventListener("mousemove", this);
     this._panel.removeEventListener("popupshowing", this);
     this._panel.removeEventListener("popuppositioned", this);
     this._panel.removeEventListener("popupshown", this);
     this._panel.removeEventListener("popuphidden", this);
-    this.window.removeEventListener("keydown", this);
+    this.window.removeEventListener("keydown", this, true);
     this.node = this._openPopupPromise = this._openPopupCancelCallback =
       this._viewContainer = this._viewStack = this._transitionDetails = null;
   }
 
   /**
    * Tries to open the panel associated with this PanelMultiView, and displays
    * the main view specified with the "mainViewId" attribute.
    *
@@ -1109,17 +1109,24 @@ var PanelMultiView = class extends Assoc
         currentView.keyNavigation(aEvent);
         break;
       case "mousemove":
         this.openViews.forEach(panelView => panelView.clearNavigation());
         break;
       case "popupshowing": {
         this._viewContainer.setAttribute("panelopen", "true");
         if (!this.node.hasAttribute("disablekeynav")) {
-          this.window.addEventListener("keydown", this);
+          // We add the keydown handler on the window so that it handles key
+          // presses when a panel appears but doesn't get focus, as happens
+          // when a button to open a panel is clicked with the mouse.
+          // However, this means the listener is on an ancestor of the panel,
+          // which means that handlers such as ToolbarKeyboardNavigator are
+          // deeper in the tree. Therefore, this must be a capturing listener
+          // so we get the event first.
+          this.window.addEventListener("keydown", this, true);
           this._panel.addEventListener("mousemove", this);
         }
         break;
       }
       case "popuppositioned": {
         if (this._panel.state == "showing") {
           let maxHeight = this._calculateMaxHeight();
           this._viewStack.style.maxHeight = maxHeight + "px";
@@ -1138,17 +1145,17 @@ var PanelMultiView = class extends Assoc
         this._activateView(mainPanelView);
         break;
       case "popuphidden": {
         // WebExtensions consumers can hide the popup from viewshowing, or
         // mid-transition, which disrupts our state:
         this._transitioning = false;
         this._viewContainer.removeAttribute("panelopen");
         this._cleanupTransitionPhase();
-        this.window.removeEventListener("keydown", this);
+        this.window.removeEventListener("keydown", this, true);
         this._panel.removeEventListener("mousemove", this);
         this.closeAllViews();
 
         // Clear the main view size caches. The dimensions could be different
         // when the popup is opened again, e.g. through touch mode sizing.
         this._viewContainer.style.removeProperty("min-height");
         this._viewStack.style.removeProperty("max-height");
         this._viewContainer.style.removeProperty("width");