Bug 1547635: PanelMultiView: Don't override keyboard navigation in context menus. r=Gijs
authorJames Teh <jteh@mozilla.com>
Wed, 08 May 2019 05:18:17 +0000
changeset 531818 9518f9e5ecf9e34e618e728dc8725534bd7a0b70
parent 531817 c464e9d2f4b810cb4da5651c5c5e14c5c9ab1080
child 531819 d851ba3d3b29675ec384dba6a25de24dd8b26e28
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1547635
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 1547635: PanelMultiView: Don't override keyboard navigation in context menus. r=Gijs Normally, context menu keyboard handling takes precedence. However, because we have a capturing window keydown listener, our listener takes precedence. Therefore, we need to check for an open context menu and suppress our keyboard handling in this case. Differential Revision: https://phabricator.services.mozilla.com/D29791
browser/components/customizableui/PanelMultiView.jsm
browser/components/customizableui/test/browser_PanelMultiView_keyboard.js
--- a/browser/components/customizableui/PanelMultiView.jsm
+++ b/browser/components/customizableui/PanelMultiView.jsm
@@ -1596,49 +1596,69 @@ var PanelView = class extends Associated
     // check this for keys we potentially care about, not *all* keys.
     let tabOnly = () => {
       // We use the real focus rather than this.selectedElement because focus
       // might have been moved without keyboard navigation (e.g. mouse click)
       // and this.selectedElement is only updated for keyboard navigation.
       return focus && this._isNavigableWithTabOnly(focus);
     };
 
+    // If a context menu is open, we must let it handle all keys.
+    // Normally, this just happens, but because we have a capturing window
+    // keydown listener, our listener takes precedence.
+    // Again, we only want to do this check on demand for performance.
+    let isContextMenuOpen = () => {
+      if (!focus) {
+        return false;
+      }
+      let contextNode = focus.closest("[context]");
+      if (!contextNode) {
+        return false;
+      }
+      let context = contextNode.getAttribute("context");
+      let popup = this.document.getElementById(context);
+      return popup && popup.state == "open";
+    };
+
     let keyCode = event.code;
     switch (keyCode) {
       case "ArrowDown":
       case "ArrowUp":
         if (tabOnly()) {
           break;
         }
         // Fall-through...
       case "Tab": {
+        if (isContextMenuOpen()) {
+          break;
+        }
         stop();
         let isDown = (keyCode == "ArrowDown") ||
                      (keyCode == "Tab" && !event.shiftKey);
         let button = this.moveSelection(isDown, keyCode != "Tab");
         Services.focus.setFocus(button, Services.focus.FLAG_BYKEY);
         break;
       }
       case "Home":
-        if (tabOnly()) {
+        if (tabOnly() || isContextMenuOpen()) {
           break;
         }
         stop();
         this.focusFirstNavigableElement(true);
         break;
       case "End":
-        if (tabOnly()) {
+        if (tabOnly() || isContextMenuOpen()) {
           break;
         }
         stop();
         this.focusLastNavigableElement(true);
         break;
       case "ArrowLeft":
       case "ArrowRight": {
-        if (tabOnly()) {
+        if (tabOnly() || isContextMenuOpen()) {
           break;
         }
         stop();
         if ((!this.window.RTL_UI && keyCode == "ArrowLeft") ||
             (this.window.RTL_UI && keyCode == "ArrowRight")) {
           this.node.panelMultiView.goBack();
           break;
         }
@@ -1648,17 +1668,17 @@ var PanelView = class extends Associated
         if (!button || !button.classList.contains("subviewbutton-nav")) {
           break;
         }
         // Fall-through...
       }
       case "Space":
       case "NumpadEnter":
       case "Enter": {
-        if (tabOnly()) {
+        if (tabOnly() || isContextMenuOpen()) {
           break;
         }
         let button = this.selectedElement;
         if (!button)
           break;
         stop();
 
         this._doingKeyboardActivation = true;
--- a/browser/components/customizableui/test/browser_PanelMultiView_keyboard.js
+++ b/browser/components/customizableui/test/browser_PanelMultiView_keyboard.js
@@ -8,16 +8,17 @@
  */
 
 const {PanelMultiView} = ChromeUtils.import("resource:///modules/PanelMultiView.jsm");
 
 let gAnchor;
 let gPanel;
 let gPanelMultiView;
 let gMainView;
+let gMainContext;
 let gMainButton1;
 let gMainMenulist;
 let gMainTextbox;
 let gMainButton2;
 let gMainButton3;
 let gMainTabOrder;
 let gMainArrowOrder;
 let gSubView;
@@ -69,21 +70,26 @@ add_task(async function setup() {
   navBar.appendChild(gPanel);
   gPanelMultiView = document.createXULElement("panelmultiview");
   gPanelMultiView.setAttribute("mainViewId", "testMainView");
   gPanel.appendChild(gPanelMultiView);
 
   gMainView = document.createXULElement("panelview");
   gMainView.id = "testMainView";
   gPanelMultiView.appendChild(gMainView);
+  gMainContext = document.createXULElement("menupopup");
+  gMainContext.id = "gMainContext";
+  gMainView.appendChild(gMainContext);
+  gMainContext.appendChild(document.createXULElement("menuitem"));
   gMainButton1 = document.createXULElement("button");
   gMainButton1.id = "gMainButton1";
   gMainView.appendChild(gMainButton1);
   // We use this for anchoring subviews, so it must have a label.
   gMainButton1.setAttribute("label", "gMainButton1");
+  gMainButton1.setAttribute("context", "gMainContext");
   gMainMenulist = document.createXULElement("menulist");
   gMainMenulist.id = "gMainMenulist";
   gMainView.appendChild(gMainMenulist);
   let menuPopup = document.createXULElement("menupopup");
   gMainMenulist.appendChild(menuPopup);
   let item = document.createXULElement("menuitem");
   item.setAttribute("value", "1");
   item.setAttribute("selected", "true");
@@ -328,8 +334,32 @@ add_task(async function testTabArrowsBro
   is(textarea.selectionStart, 0, "selectionStart 0 after ArrowLeft");
   is(doc.activeElement, textarea, "textarea still focused");
   let docButton = doc.getElementById("docButton");
   expectFocusAfterKey("Tab", docButton);
   // Make sure tab leaves the document and reaches the Back button.
   expectFocusAfterKey("Tab", backButton);
   await hidePopup();
 });
+
+// Test that the arrow keys aren't overridden in context menus.
+add_task(async function testArowsContext() {
+  await openPopup();
+  await expectFocusAfterKey("ArrowDown", gMainButton1);
+  let shown = BrowserTestUtils.waitForEvent(gMainContext, "popupshown");
+  // There's no cross-platform way to open a context menu from the keyboard.
+  gMainContext.openPopup();
+  await shown;
+  let item = gMainContext.children[0];
+  ok(!item.getAttribute("_moz-menuactive"),
+     "First context menu item initially inactive");
+  let active = BrowserTestUtils.waitForEvent(item, "DOMMenuItemActive");
+  EventUtils.synthesizeKey("KEY_ArrowDown");
+  await active;
+  ok(item.getAttribute("_moz-menuactive"),
+     "First context menu item active after ArrowDown");
+  is(document.activeElement, gMainButton1,
+     "gMainButton1 still focused after ArrowDown");
+  let hidden = BrowserTestUtils.waitForEvent(gMainContext, "popuphidden");
+  gMainContext.hidePopup();
+  await hidden;
+  await hidePopup();
+});