Bug 1506504: Add/fix keyboard behaviour for browser toolbar buttons which open popups. r=Gijs
authorJames Teh <jteh@mozilla.com>
Thu, 20 Dec 2018 11:33:32 +0000
changeset 511852 fb3f227c4f29
parent 511851 2dc724f82cb1
child 511853 dbaf671d5e84
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1506504
milestone66.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 1506504: Add/fix keyboard behaviour for browser toolbar buttons which open popups. r=Gijs 1. Fix the Firefox menu button so that it only handles space and enter, rather than incorrectly activating for *all* key presses. 2. Add keyboard support (space and enter) for the Library and page Actions buttons. 3. Add keyboard support (space and enter) for customizable widgets of type "view"; e.g. the Developer button. 4. Add keyboard support (space and enter) for page action buttons pinned to the URL bar; e.g. the Send Tab to Device button. Differential Revision: https://phabricator.services.mozilla.com/D11608
browser/base/content/browser-pageActions.js
browser/base/content/browser.xul
browser/base/content/moz.build
browser/base/content/test/keyboard/.eslintrc.js
browser/base/content/test/keyboard/browser.ini
browser/base/content/test/keyboard/browser_toolbarButtonKeyPress.js
browser/base/moz.build
browser/components/customizableui/CustomizableUI.jsm
browser/components/customizableui/content/panelUI.js
--- a/browser/base/content/browser-pageActions.js
+++ b/browser/base/content/browser-pageActions.js
@@ -263,18 +263,20 @@ var BrowserPageActions = {
    *
    * @param  action (PageActions.Action, required)
    *         The action for which to toggle the panel.  If the action is in the
    *         urlbar, then the panel will be anchored to it.  Otherwise, a
    *         suitable anchor will be used.
    * @param  panelNode (DOM node, optional)
    *         The panel to use.  This method takes a hands-off approach with
    *         regard to your panel in terms of attributes, styling, etc.
+   * @param  event (DOM event, optional)
+   *         The event which triggered this panel.
    */
-  togglePanelForAction(action, panelNode = null) {
+  togglePanelForAction(action, panelNode = null, event = null) {
     let aaPanelNode = this.activatedActionPanelNode;
     if (panelNode) {
       // Note that this particular code path will not prevent the panel from
       // opening later if PanelMultiView.showPopup was called but the panel has
       // not been opened yet.
       if (panelNode.state != "closed") {
         PanelMultiView.hidePopup(panelNode);
         return;
@@ -293,18 +295,20 @@ var BrowserPageActions = {
     PanelMultiView.hidePopup(this.panelNode);
 
     let anchorNode = this.panelAnchorNodeForAction(action);
     anchorNode.setAttribute("open", "true");
     panelNode.addEventListener("popuphiding", () => {
       anchorNode.removeAttribute("open");
     }, { once: true });
 
-    PanelMultiView.openPopup(panelNode, anchorNode, "bottomcenter topright")
-                  .catch(Cu.reportError);
+    PanelMultiView.openPopup(panelNode, anchorNode, {
+      position: "bottomcenter topright",
+      triggerEvent: event,
+    }).catch(Cu.reportError);
   },
 
   _makeActivatedActionPanelForAction(action) {
     let panelNode = document.createXULElement("panel");
     panelNode.id = this._activatedActionPanelID;
     panelNode.classList.add("cui-widget-panel");
     panelNode.setAttribute("actionID", action.id);
     panelNode.setAttribute("role", "group");
@@ -455,19 +459,21 @@ var BrowserPageActions = {
     action.onPlacedInUrlbar(node);
   },
 
   _makeUrlbarButtonNode(action) {
     let buttonNode = document.createXULElement("image");
     buttonNode.classList.add("urlbar-icon", "urlbar-page-action");
     buttonNode.setAttribute("actionid", action.id);
     buttonNode.setAttribute("role", "button");
-    buttonNode.addEventListener("click", event => {
+    let commandHandler = event => {
       this.doCommandForAction(action, event, buttonNode);
-    });
+    };
+    buttonNode.addEventListener("click", commandHandler);
+    buttonNode.addEventListener("keypress", commandHandler);
     return buttonNode;
   },
 
   /**
    * Removes all the DOM nodes of the given action.
    *
    * @param  action (PageActions.Action, required)
    *         The action to remove.
@@ -623,16 +629,22 @@ var BrowserPageActions = {
       action.onSubviewPlaced(panelViewNode);
     }
   },
 
   doCommandForAction(action, event, buttonNode) {
     if (event && event.type == "click" && event.button != 0) {
       return;
     }
+    if (event && event.type == "keypress") {
+      if (event.key != " " && event.key != "Enter") {
+        return;
+      }
+      event.stopPropagation();
+    }
     PageActions.logTelemetry("used", action, buttonNode);
     // If we're in the panel, open a subview inside the panel:
     // Note that we can't use this.panelNode.contains(buttonNode) here
     // because of XBL boundaries breaking Element.contains.
     if (action.getWantsSubview(window) &&
         buttonNode &&
         buttonNode.closest("panel") == this.panelNode) {
       let panelViewNodeID = this._panelViewNodeIDForActionID(action.id, false);
@@ -644,17 +656,17 @@ var BrowserPageActions = {
     // Otherwise, hide the main popup in case it was open:
     PanelMultiView.hidePopup(this.panelNode);
 
     let aaPanelNode = this.activatedActionPanelNode;
     if (!aaPanelNode || aaPanelNode.getAttribute("actionID") != action.id) {
       action.onCommand(event, buttonNode);
     }
     if (action.getWantsSubview(window) || action.wantsIframe) {
-      this.togglePanelForAction(action);
+      this.togglePanelForAction(action, null, event);
     }
   },
 
   /**
    * Returns the action for a node.
    *
    * @param  node (DOM node, required)
    *         A button DOM node, either one that's shown in the page action panel
--- a/browser/base/content/browser.xul
+++ b/browser/base/content/browser.xul
@@ -1028,17 +1028,18 @@ xmlns="http://www.w3.org/1999/xhtml"
                        onclick="FullZoom.reset();"
                        tooltip="dynamic-shortcut-tooltip"
                        hidden="true"/>
                 <box id="pageActionSeparator" class="urlbar-page-action"/>
                 <image id="pageActionButton"
                        class="urlbar-icon urlbar-page-action"
                        role="button"
                        tooltiptext="&pageActionButton.tooltip;"
-                       onmousedown="BrowserPageActions.mainButtonClicked(event);"/>
+                       onmousedown="BrowserPageActions.mainButtonClicked(event);"
+                       onkeypress="BrowserPageActions.mainButtonClicked(event);"/>
                 <hbox id="star-button-box"
                       hidden="true"
                       class="urlbar-icon-wrapper urlbar-page-action"
                       onclick="BrowserPageActions.doCommandForAction(PageActions.actionForID('bookmark'), event, this);">
                   <image id="star-button"
                          class="urlbar-icon"
                          role="button"/>
                   <hbox id="star-button-animatable-box">
@@ -1080,16 +1081,17 @@ xmlns="http://www.w3.org/1999/xhtml"
                 <box id="downloads-indicator-progress-inner"/>
               </stack>
             </stack>
           </toolbarbutton>
 
         <toolbarbutton id="library-button" class="toolbarbutton-1 chromeclass-toolbar-additional subviewbutton-nav"
                        removable="true"
                        onmousedown="PanelUI.showSubView('appMenu-libraryView', this, event);"
+                       onkeypress="PanelUI.showSubView('appMenu-libraryView', this, event);"
                        closemenu="none"
                        cui-areatype="toolbar"
                        tooltiptext="&libraryButton.tooltip;"
                        label="&places.library.title;"/>
 
       </hbox>
 
       <toolbarbutton id="nav-bar-overflow-button"
--- a/browser/base/content/moz.build
+++ b/browser/base/content/moz.build
@@ -32,16 +32,19 @@ with Files("test/contextMenu/**"):
     BUG_COMPONENT = ("Firefox", "Menus")
 
 with Files("test/forms/**"):
     BUG_COMPONENT = ("Core", "Layout: Form Controls")
 
 with Files("test/historySwipeAnimation/**"):
     BUG_COMPONENT = ("Core", "Widget: Cocoa")
 
+with Files("test/keyboard/**"):
+    BUG_COMPONENT = ("Firefox", "Keyboard Navigation")
+
 with Files("test/pageinfo/**"):
     BUG_COMPONENT = ("Firefox", "Page Info Window")
 
 with Files("test/performance/**"):
     BUG_COMPONENT = ("Firefox", "General")
 
 with Files("test/performance/browser_appmenu.js"):
     BUG_COMPONENT = ("Firefox", "Menus")
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/keyboard/.eslintrc.js
@@ -0,0 +1,7 @@
+"use strict";
+
+module.exports = {
+  "extends": [
+    "plugin:mozilla/browser-test"
+  ]
+};
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/keyboard/browser.ini
@@ -0,0 +1,1 @@
+[browser_toolbarButtonKeyPress.js]
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/keyboard/browser_toolbarButtonKeyPress.js
@@ -0,0 +1,126 @@
+/* Any copyright is dedicated to the Public Domain.
+ * 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");
+}
+
+// Test activation of the app menu button from the keyboard.
+// The app menu should appear and focus should move inside it.
+add_task(async function testAppMenuButtonPress() {
+  let button = document.getElementById("PanelUI-menu-button");
+  forceFocus(button);
+  let focused = BrowserTestUtils.waitForEvent(window.PanelUI.mainView, "focus", true);
+  EventUtils.synthesizeKey(" ");
+  await focused;
+  ok(true, "Focus inside app menu after toolbar button pressed");
+  let hidden = BrowserTestUtils.waitForEvent(window.PanelUI.panel, "popuphidden");
+  EventUtils.synthesizeKey("KEY_Escape");
+  await hidden;
+});
+
+// Test that the app menu doesn't open when a key other than space or enter is
+// pressed .
+add_task(async function testAppMenuButtonWrongKey() {
+  let button = document.getElementById("PanelUI-menu-button");
+  forceFocus(button);
+  EventUtils.synthesizeKey("KEY_Tab");
+  await TestUtils.waitForTick();
+  is(window.PanelUI.panel.state, "closed", "App menu is closed after tab");
+});
+
+// Test activation of the Library button from the keyboard.
+// The Library menu should appear and focus should move inside it.
+add_task(async function testLibraryButtonPress() {
+  let button = document.getElementById("library-button");
+  forceFocus(button);
+  let view = document.getElementById("appMenu-libraryView");
+  let focused = BrowserTestUtils.waitForEvent(view, "focus", true);
+  EventUtils.synthesizeKey(" ");
+  await focused;
+  ok(true, "Focus inside Library menu after toolbar button pressed");
+  let hidden = BrowserTestUtils.waitForEvent(document, "popuphidden", true);
+  view.closest("panel").hidePopup();
+  await hidden;
+});
+
+// Test activation of the Developer button from the keyboard.
+// This is a customizable widget of type "view".
+// The Developer menu should appear and focus should move inside it.
+add_task(async function testDeveloperButtonPress() {
+  CustomizableUI.addWidgetToArea("developer-button", CustomizableUI.AREA_NAVBAR);
+  let button = document.getElementById("developer-button");
+  forceFocus(button);
+  let view = document.getElementById("PanelUI-developer");
+  let focused = BrowserTestUtils.waitForEvent(view, "focus", true);
+  EventUtils.synthesizeKey(" ");
+  await focused;
+  ok(true, "Focus inside Developer menu after toolbar button pressed");
+  let hidden = BrowserTestUtils.waitForEvent(document, "popuphidden", true);
+  view.closest("panel").hidePopup();
+  await hidden;
+  CustomizableUI.reset();
+});
+// Test that the Developer menu doesn't open when a key other than space or
+// enter is pressed .
+add_task(async function testDeveloperButtonWrongKey() {
+  CustomizableUI.addWidgetToArea("developer-button", CustomizableUI.AREA_NAVBAR);
+  let button = document.getElementById("developer-button");
+  forceFocus(button);
+  EventUtils.synthesizeKey("KEY_Tab");
+  await TestUtils.waitForTick();
+  let panel = document.getElementById("PanelUI-developer").closest("panel");
+  ok(!panel || panel.state == "closed", "Developer menu not open after tab");
+  CustomizableUI.reset();
+});
+
+// Test activation of the Page actions button from the keyboard.
+// The Page Actions menu should appear and focus should move inside it.
+add_task(async function testPageActionsButtonPress() {
+  await BrowserTestUtils.withNewTab("https://example.com", async function() {
+    let button = document.getElementById("pageActionButton");
+    forceFocus(button);
+    let view = document.getElementById("pageActionPanelMainView");
+    let focused = BrowserTestUtils.waitForEvent(view, "focus", true);
+    EventUtils.synthesizeKey(" ");
+    await focused;
+    ok(true, "Focus inside Page Actions menu after toolbar button pressed");
+    let hidden = BrowserTestUtils.waitForEvent(document, "popuphidden", true);
+    view.closest("panel").hidePopup();
+    await hidden;
+  });
+});
+
+// Test activation of the Send Tab to Device button from the keyboard.
+// This is a page action button built at runtime by PageActions.
+// The Send Tab to Device menu should appear and focus should move inside it.
+add_task(async function testSendTabToDeviceButtonPress() {
+  await BrowserTestUtils.withNewTab("https://example.com", async function() {
+    PageActions.actionForID("sendToDevice").pinnedToUrlbar = true;
+    let button = document.getElementById("pageAction-urlbar-sendToDevice");
+    forceFocus(button);
+    let mainPopupSet = document.getElementById("mainPopupSet");
+    let focused = BrowserTestUtils.waitForEvent(mainPopupSet, "focus", true);
+    EventUtils.synthesizeKey(" ");
+    await focused;
+    let view = document.getElementById("pageAction-urlbar-sendToDevice-subview");
+    ok(view.contains(document.activeElement),
+       "Focus inside Page Actions menu after toolbar button pressed");
+    let hidden = BrowserTestUtils.waitForEvent(document, "popuphidden", true);
+    view.closest("panel").hidePopup();
+    await hidden;
+    PageActions.actionForID("sendToDevice").pinnedToUrlbar = false;
+  });
+});
--- a/browser/base/moz.build
+++ b/browser/base/moz.build
@@ -25,16 +25,17 @@ BROWSER_CHROME_MANIFESTS += [
     'content/test/about/browser.ini',
     'content/test/alerts/browser.ini',
     'content/test/captivePortal/browser.ini',
     'content/test/contextMenu/browser.ini',
     'content/test/favicons/browser.ini',
     'content/test/forms/browser.ini',
     'content/test/general/browser.ini',
     'content/test/historySwipeAnimation/browser.ini',
+    'content/test/keyboard/browser.ini',
     'content/test/metaTags/browser.ini',
     'content/test/pageinfo/browser.ini',
     'content/test/performance/browser.ini',
     'content/test/performance/hidpi/browser.ini',
     'content/test/performance/lowdpi/browser.ini',
     'content/test/permissions/browser.ini',
     'content/test/plugins/browser.ini',
     'content/test/plugins/xbl/browser.ini',
--- a/browser/components/customizableui/CustomizableUI.jsm
+++ b/browser/components/customizableui/CustomizableUI.jsm
@@ -1515,16 +1515,19 @@ var CustomizableUIInternal = {
           if (aWidget.source == CustomizableUI.SOURCE_BUILTIN) {
             nodeClasses.push("subviewbutton-nav");
           }
           this.ensureSubviewListeners(viewNode);
         } else {
           log.error("Could not find the view node with id: " + aWidget.viewId +
                     ", for widget: " + aWidget.id + ".");
         }
+
+        let keyPressHandler = this.handleWidgetKeyPress.bind(this, aWidget, node);
+        node.addEventListener("keypress", keyPressHandler);
       }
       node.setAttribute("class", nodeClasses.join(" "));
 
       if (aWidget.onCreated) {
         aWidget.onCreated(node);
       }
     }
 
@@ -1611,16 +1614,17 @@ var CustomizableUIInternal = {
     if (!shortcut) {
       return;
     }
 
     aTargetNode.setAttribute("shortcut", ShortcutUtils.prettifyShortcut(shortcut));
   },
 
   handleWidgetCommand(aWidget, aNode, aEvent) {
+    // Note that aEvent can be a keypress event for widgets of type "view".
     log.debug("handleWidgetCommand");
 
     if (aWidget.onBeforeCommand) {
       try {
         aWidget.onBeforeCommand.call(null, aEvent);
       } catch (e) {
         log.error(e);
       }
@@ -1667,16 +1671,25 @@ var CustomizableUIInternal = {
         Cu.reportError(e);
       }
     } else {
       // XXXunf Need to think this through more, and formalize.
       Services.obs.notifyObservers(aNode, "customizedui-widget-click", aWidget.id);
     }
   },
 
+  handleWidgetKeyPress(aWidget, aNode, aEvent) {
+    if (aEvent.key != " " && aEvent.key != "Enter") {
+      return;
+    }
+    aEvent.stopPropagation();
+    aEvent.preventDefault();
+    this.handleWidgetCommand(aWidget, aNode, aEvent);
+  },
+
   _getPanelForNode(aNode) {
     return aNode.closest("panel");
   },
 
   /*
    * If people put things in the panel which need more than single-click interaction,
    * we don't want to close it. Right now we check for text inputs and menu buttons.
    * We also check for being outside of any toolbaritem/toolbarbutton, ie on a blank
--- a/browser/components/customizableui/content/panelUI.js
+++ b/browser/components/customizableui/content/panelUI.js
@@ -270,17 +270,20 @@ const PanelUI = {
           CustomizableUI.removePanelCloseListeners(this.panel);
         }
         break;
       case "mousedown":
         if (aEvent.button == 0)
           this.toggle(aEvent);
         break;
       case "keypress":
-        this.toggle(aEvent);
+        if (aEvent.key == " " || aEvent.key == "Enter") {
+          this.toggle(aEvent);
+          aEvent.stopPropagation();
+        }
         break;
       case "MozDOMFullscreen:Entered":
       case "MozDOMFullscreen:Exited":
       case "fullscreen":
       case "activate":
         this._updateNotifications();
         break;
       case "ViewShowing":
@@ -341,22 +344,26 @@ const PanelUI = {
    * @param aEvent the event triggering the view showing.
    */
   async showSubView(aViewId, aAnchor, aEvent) {
     let domEvent = null;
     if (aEvent) {
       if (aEvent.type == "mousedown" && aEvent.button != 0) {
         return;
       }
+      if (aEvent.type == "keypress" && aEvent.key != " " &&
+          aEvent.key != "Enter") {
+        return;
+      }
       if (aEvent.type == "command" && aEvent.inputSource != null) {
         // Synthesize a new DOM mouse event to pass on the inputSource.
         domEvent = document.createEvent("MouseEvent");
         domEvent.initNSMouseEvent("click", true, true, null, 0, aEvent.screenX, aEvent.screenY,
                                   0, 0, false, false, false, false, 0, aEvent.target, 0, aEvent.inputSource);
-      } else if (aEvent.mozInputSource != null) {
+      } else if (aEvent.mozInputSource != null || aEvent.type == "keypress") {
         domEvent = aEvent;
       }
     }
 
     this._ensureEventListenersAdded();
     let viewNode = document.getElementById(aViewId);
     if (!viewNode) {
       Cu.reportError("Could not show panel subview with id: " + aViewId);