Bug 1506503: PanelMultiView: Automatically focus the first item if a panel is opened via the keyboard. r=johannh,paolo
authorJames Teh <jteh@mozilla.com>
Fri, 16 Nov 2018 22:51:59 +0000
changeset 503307 7949f29501222774639dec4173237d3b1e962df6
parent 503306 eb21007e7da0cc392906a517602935cb77a6c37e
child 503308 045f0da5633d362ecad059b35ae5f696c2db2384
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjohannh, paolo
bugs1506503
milestone65.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 1506503: PanelMultiView: Automatically focus the first item if a panel is opened via the keyboard. r=johannh,paolo Previously, we had specific code to do this for the "View site information" button (#identity-box) when activated via the keyboard. To work well for keyboard and screen reader users, all such popups (e.g. Firefox menu, Page Actions, etc.) should do this. These are all based on panelMultiView. The arguments passed to PanelMultiView.openPopup can include the event which triggered the popup. We now use this to detect keypress events and focus the first item in the panel in that case. Differential Revision: https://phabricator.services.mozilla.com/D11605
browser/base/content/browser-siteIdentity.js
browser/components/customizableui/PanelMultiView.jsm
browser/components/customizableui/test/browser.ini
browser/components/customizableui/test/browser_PanelMultiView_focus.js
--- a/browser/base/content/browser-siteIdentity.js
+++ b/browser/base/content/browser-siteIdentity.js
@@ -813,41 +813,34 @@ var gIdentityHandler = {
     // being focused (and therefore, interacted with) by the user. However, we
     // want to allow opening the identity popup from the device control menu,
     // which calls click() on the identity button, so we don't return early.
     if (!this._sharingState &&
         gURLBar.getAttribute("pageproxystate") != "valid") {
       return;
     }
 
-    // Move focus to the next available element in the identity popup.
-    // This is required by role=alertdialog and fixes an issue where
-    // an already open panel would steal focus from the identity popup.
-    if (event.type == "keypress") {
-      let panelView = PanelView.forNode(this._identityPopupMainView);
-      this._identityPopupMainView.addEventListener("ViewShown", () => panelView.focusFirstNavigableElement(),
-        {once: true});
-    }
-
     // Make sure that the display:none style we set in xul is removed now that
     // the popup is actually needed
     this._identityPopup.hidden = false;
 
     // Remove the reload hint that we show after a user has cleared a permission.
     this._permissionReloadHint.setAttribute("hidden", "true");
 
     // Update the popup strings
     this.refreshIdentityPopup();
 
     // Add the "open" attribute to the identity box for styling
     this._identityBox.setAttribute("open", "true");
 
     // Now open the popup, anchored off the primary chrome element
-    PanelMultiView.openPopup(this._identityPopup, this._identityIcon,
-                             "bottomcenter topleft").catch(Cu.reportError);
+    PanelMultiView.openPopup(this._identityPopup, this._identityIcon, {
+      position: "bottomcenter topleft",
+      triggerEvent: event,
+    }).catch(Cu.reportError);
   },
 
   onPopupShown(event) {
     if (event.target == this._identityPopup) {
       window.addEventListener("focus", this, true);
     }
 
     let extra = {};
--- a/browser/components/customizableui/PanelMultiView.jsm
+++ b/browser/components/customizableui/PanelMultiView.jsm
@@ -438,27 +438,33 @@ var PanelMultiView = class extends Assoc
    * canceled, the panel is opened using the arguments from the previous call,
    * and this call is ignored. If the operation was canceled, it will be
    * retried again using the arguments from this call.
    *
    * It's not necessary for the <panelmultiview> binding to be connected when
    * this method is called, but the containing panel must have its display
    * turned on, for example it shouldn't have the "hidden" attribute.
    *
+   * @param anchor
+   *        The node to anchor the popup to.
+   * @param options
+   *        Either options to use or a string position. This is forwarded to
+   *        the openPopup method of the panel.
    * @param args
-   *        Arguments to be forwarded to the openPopup method of the panel.
+   *        Additional arguments to be forwarded to the openPopup method of the
+   *        panel.
    *
    * @resolves With true as soon as the request to display the panel has been
    *           sent, or with false if the operation was canceled. The state of
    *           the panel at this point is not guaranteed. It may be still
    *           showing, completely shown, or completely hidden.
    * @rejects If an exception is thrown at any point in the process before the
    *          request to display the panel is sent.
    */
-  async openPopup(...args) {
+  async openPopup(anchor, options, ...args) {
     // Set up the function that allows hidePopup or a second call to showPopup
     // to cancel the specific panel opening operation that we're starting below.
     // This function must be synchronous, meaning we can't use Promise.race,
     // because hidePopup wants to dispatch the "popuphidden" event synchronously
     // even if the panel has not been opened yet.
     let canCancel = true;
     let cancelCallback = this._openPopupCancelCallback = () => {
       // If the cancel callback is called and the panel hasn't been prepared
@@ -510,26 +516,33 @@ var PanelMultiView = class extends Assoc
         return false;
       }
       // We have to set canCancel to false before opening the popup because the
       // hidePopup method of PanelMultiView can be re-entered by event handlers.
       // If the openPopup call fails, however, we still have to dispatch the
       // "popuphidden" event even if canCancel was set to false.
       try {
         canCancel = false;
-        this._panel.openPopup(...args);
+        this._panel.openPopup(anchor, options, ...args);
 
         // On Windows, if another popup is hiding while we call openPopup, the
         // call won't fail but the popup won't open. In this case, we have to
         // dispatch an artificial "popuphidden" event to reset our state.
         if (this._panel.state == "closed" && this.openViews.length) {
           this.dispatchCustomEvent("popuphidden");
           return false;
         }
 
+        if (options && typeof options == "object" && options.triggerEvent &&
+            options.triggerEvent.type == "keypress" &&
+            this.openViews.length) {
+          // This was opened via the keyboard, so focus the first item.
+          this.openViews[0].focusWhenActive = true;
+        }
+
         return true;
       } catch (ex) {
         this.dispatchCustomEvent("popuphidden");
         throw ex;
       }
     });
   }
 
@@ -788,16 +801,20 @@ var PanelMultiView = class extends Assoc
 
   /**
    * Activates the specified view and raises the ViewShown event, unless the
    * view was closed in the meantime.
    */
   _activateView(panelView) {
     if (panelView.isOpenIn(this)) {
       panelView.active = true;
+      if (panelView.focusWhenActive) {
+        panelView.focusFirstNavigableElement();
+        panelView.focusWhenActive = false;
+      }
       panelView.dispatchCustomEvent("ViewShown");
     }
   }
 
   /**
    * Closes the most recent PanelView and raises the ViewHiding event.
    *
    * @note The ViewHiding event is not cancelable and should probably be renamed
@@ -1148,16 +1165,25 @@ var PanelView = class extends Associated
   constructor(node) {
     super(node);
 
     /**
      * Indicates whether the view is active. When this is false, consumers can
      * wait for the ViewShown event to know when the view becomes active.
      */
     this.active = false;
+
+    /**
+     * Specifies whether the view should be focused when active. When this
+     * is true, the first navigable element in the view will be focused
+     * when the view becomes active. This should be set to true when the view
+     * is activated from the keyboard. It will be set to false once the view
+     * is active.
+     */
+    this.focusWhenActive = false;
   }
 
   /**
    * Indicates whether the view is open in the specified PanelMultiView object.
    */
   isOpenIn(panelMultiView) {
     return this.node.panelMultiView == panelMultiView.node;
   }
@@ -1181,16 +1207,17 @@ var PanelView = class extends Associated
    * the "active" property.
    */
   set visible(value) {
     if (value) {
       this.node.setAttribute("visible", true);
     } else {
       this.node.removeAttribute("visible");
       this.active = false;
+      this.focusWhenActive = false;
     }
   }
 
   /**
    * Constrains the width of this view using the "min-width" and "max-width"
    * styles. Setting this to zero removes the constraints.
    */
   set minMaxWidth(value) {
--- a/browser/components/customizableui/test/browser.ini
+++ b/browser/components/customizableui/test/browser.ini
@@ -168,16 +168,17 @@ tags = fullscreen
 [browser_backfwd_enabled_post_customize.js]
 [browser_check_tooltips_in_navbar.js]
 [browser_editcontrols_update.js]
 subsuite = clipboard
 skip-if = (verify && !debug && os == 'mac') || (os == 'mac') # Bug 1458046
 [browser_customization_context_menus.js]
 [browser_newtab_button_customizemode.js]
 [browser_open_from_popup.js]
+[browser_PanelMultiView_focus.js]
 [browser_sidebar_toggle.js]
 skip-if = verify
 [browser_tabbar_big_widgets.js]
 [browser_remote_tabs_button.js]
 skip-if = (verify && debug && (os == 'linux' || os == 'mac'))
 [browser_widget_animation.js]
 
 # Unit tests for the PanelMultiView module. These are independent from
new file mode 100644
--- /dev/null
+++ b/browser/components/customizableui/test/browser_PanelMultiView_focus.js
@@ -0,0 +1,66 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+/**
+ * Test the focus behavior when opening PanelViews.
+ */
+
+ChromeUtils.import("resource:///modules/PanelMultiView.jsm");
+
+let gAnchor;
+let gPanel;
+let gMainView;
+let gMainButton;
+
+add_task(async function setup() {
+  let navBar = document.getElementById("nav-bar");
+  gAnchor = document.createXULElement("toolbarbutton");
+  // Must be focusable in order for key presses to work.
+  gAnchor.style["-moz-user-focus"] = "normal";
+  navBar.appendChild(gAnchor);
+  gPanel = document.createXULElement("panel");
+  navBar.appendChild(gPanel);
+  let panelMultiView = document.createXULElement("panelmultiview");
+  panelMultiView.setAttribute("mainViewId", "testMainView");
+  gPanel.appendChild(panelMultiView);
+  gMainView = document.createXULElement("panelview");
+  gMainView.id = "testMainView";
+  panelMultiView.appendChild(gMainView);
+  gMainButton = document.createXULElement("button");
+  gMainView.appendChild(gMainButton);
+
+  let onPress = event => PanelMultiView.openPopup(gPanel, gAnchor, {
+    triggerEvent: event,
+  });
+  gAnchor.addEventListener("keypress", onPress);
+  gAnchor.addEventListener("click", onPress);
+
+  registerCleanupFunction(() => {
+    gAnchor.remove();
+    gPanel.remove();
+  });
+});
+
+// Activate the main view by pressing a key. Focus should be moved inside.
+add_task(async function testMainViewByKeypress() {
+  gAnchor.focus();
+  await gCUITestUtils.openPanelMultiView(gPanel, gMainView,
+    () => EventUtils.synthesizeKey(" "));
+  Assert.equal(document.activeElement, gMainButton,
+    "Focus on button in main view");
+  await gCUITestUtils.hidePanelMultiView(gPanel,
+    () => PanelMultiView.hidePopup(gPanel));
+});
+
+// Activate the main view by clicking the mouse. Focus should not be moved
+// inside.
+add_task(async function testMainViewByClick() {
+  await gCUITestUtils.openPanelMultiView(gPanel, gMainView,
+    () => gAnchor.click());
+  Assert.notEqual(document.activeElement, gMainButton,
+    "Focus not on button in main view");
+  await gCUITestUtils.hidePanelMultiView(gPanel,
+    () => PanelMultiView.hidePopup(gPanel));
+});