Bug 1467385 - Use PanelMultiView APIs for keyboard navigation in the identity popup. r=Paolo draft
authorJohann Hofmann <jhofmann@mozilla.com>
Mon, 02 Jul 2018 14:34:25 +0200
changeset 814705 ec7a6f83fdda176f3286cf4395dc72de6cad2466
parent 814704 afdeb0288690f0acde2ba6bd008f860e1acdd026
push id115315
push userjhofmann@mozilla.com
push dateThu, 05 Jul 2018 22:52:02 +0000
reviewersPaolo
bugs1467385
milestone63.0a1
Bug 1467385 - Use PanelMultiView APIs for keyboard navigation in the identity popup. r=Paolo PanelMultiView has its own concept of "selected", so we should use this rather than move focus using advanceFocusIntoSubTree. MozReview-Commit-ID: DW2JqqggLl1
browser/base/content/browser-siteIdentity.js
browser/components/controlcenter/content/panel.inc.xul
browser/components/customizableui/PanelMultiView.jsm
browser/components/customizableui/test/browser_panel_keyboard_navigation.js
--- a/browser/base/content/browser-siteIdentity.js
+++ b/browser/base/content/browser-siteIdentity.js
@@ -41,22 +41,16 @@ var gIdentityHandler = {
   _sslStatus: null,
 
   /**
    * Bitmask provided by nsIWebProgressListener.onSecurityChange.
    */
   _state: 0,
 
   /**
-   * This flag gets set if the identity popup was opened by a keypress,
-   * to be able to focus it on the popupshown event.
-   */
-  _popupTriggeredByKeyboard: false,
-
-  /**
    * RegExp used to decide if an about url should be shown as being part of
    * the browser UI.
    */
   _secureInternalUIWhitelist: /^(?:accounts|addons|cache|config|crashes|customizing|downloads|healthreport|license|permissions|preferences|rights|searchreset|sessionrestore|support|welcomeback)(?:[?#]|$)/i,
 
   get _isBroken() {
     return this._state & Ci.nsIWebProgressListener.STATE_IS_BROKEN;
   },
@@ -826,17 +820,24 @@ 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;
     }
 
-    this._popupTriggeredByKeyboard = event.type == "keypress";
+    // 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");
 
@@ -848,23 +849,16 @@ var gIdentityHandler = {
 
     // Now open the popup, anchored off the primary chrome element
     PanelMultiView.openPopup(this._identityPopup, this._identityIcon,
                              "bottomcenter topleft").catch(Cu.reportError);
   },
 
   onPopupShown(event) {
     if (event.target == this._identityPopup) {
-      if (this._popupTriggeredByKeyboard) {
-        // 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.
-        document.commandDispatcher.advanceFocusIntoSubtree(this._identityPopup);
-      }
-
       window.addEventListener("focus", this, true);
     }
   },
 
   onPopupHidden(event) {
     if (event.target == this._identityPopup) {
       window.removeEventListener("focus", this, true);
       this._identityBox.removeAttribute("open");
--- a/browser/components/controlcenter/content/panel.inc.xul
+++ b/browser/components/controlcenter/content/panel.inc.xul
@@ -42,17 +42,17 @@
                          when-mixedcontent="passive-loaded">&identity.passiveLoaded;</description>
             <description when-mixedcontent="active-loaded">&identity.activeLoaded;</description>
             <description class="identity-popup-warning-yellow"
                          when-ciphers="weak">&identity.weakEncryption;</description>
             <description when-loginforms="insecure">&identity.insecureLoginForms2;</description>
           </vbox>
         </vbox>
         <button id="identity-popup-security-expander"
-                class="identity-popup-expander"
+                class="identity-popup-expander subviewkeynav"
                 when-connection="not-secure secure secure-ev secure-cert-user-overridden"
                 oncommand="gIdentityHandler.showSecuritySubView();"/>
       </hbox>
 
       <!-- Tracking Protection Section -->
       <hbox id="tracking-protection-container"
             class="identity-popup-section"
             when-connection="not-secure secure secure-ev secure-cert-user-overridden extension">
@@ -79,32 +79,32 @@
           <description id="tracking-loaded-exception"
                        crop="end">&trackingProtection.detectedException;</description>
           <description id="tracking-not-detected-exception"
                        crop="end">&trackingProtection.notDetectedException;</description>
           <description id="tracking-reload-required"
                        crop="end">&trackingProtection.reloadRequired2;</description>
 
           <button id="tracking-action-reload"
-                  class="tracking-protection-button"
+                  class="tracking-protection-button subviewkeynav"
                   label="&trackingProtection.reload2.label;"
                   accesskey="&trackingProtection.reload2.accesskey;"
                   oncommand="TrackingProtection.hideIdentityPopupAndReload();" />
           <button id="tracking-action-unblock"
-                  class="tracking-protection-button"
+                  class="tracking-protection-button subviewkeynav"
                   label="&trackingProtection.unblock3.label;"
                   accesskey="&trackingProtection.unblock3.accesskey;"
                   oncommand="TrackingProtection.disableForCurrentPage();" />
           <button id="tracking-action-unblock-private"
-                  class="tracking-protection-button"
+                  class="tracking-protection-button subviewkeynav"
                   label="&trackingProtection.unblockPrivate3.label;"
                   accesskey="&trackingProtection.unblockPrivate3.accesskey;"
                   oncommand="TrackingProtection.disableForCurrentPage();" />
           <button id="tracking-action-block"
-                  class="tracking-protection-button"
+                  class="tracking-protection-button subviewkeynav"
                   label="&trackingProtection.block4.label;"
                   accesskey="&trackingProtection.block4.accesskey;"
                   oncommand="TrackingProtection.enableForCurrentPage();" />
         </vbox>
       </hbox>
 
       <!-- Permissions Section -->
       <hbox class="identity-popup-section"
--- a/browser/components/customizableui/PanelMultiView.jsm
+++ b/browser/components/customizableui/PanelMultiView.jsm
@@ -1358,46 +1358,55 @@ var PanelView = class extends Associated
   }
 
   /**
    * Retrieves the button elements that can be used for navigation using the
    * keyboard, that is all enabled buttons including the back button if visible.
    *
    * @return {Array}
    */
-  getNavigableElements() {
+  _getNavigableElements() {
     let buttons = Array.from(this.node.querySelectorAll(
       ".subviewbutton:not([disabled]), .subviewkeynav:not([disabled])"));
     let dwu = this._dwu;
     return buttons.filter(button => {
       let bounds = dwu.getBoundsWithoutFlushing(button);
       return bounds.width > 0 && bounds.height > 0;
     });
   }
 
   /**
    * Element that is currently selected with the keyboard, or null if no element
    * is selected. Since the reference is held weakly, it can become null or
    * undefined at any time.
    *
    * The element is usually, but not necessarily, in the "buttons" property
-   * which in turn is initialized from the getNavigableElements list.
+   * which in turn is initialized from the _getNavigableElements list.
    */
   get selectedElement() {
     return this._selectedElement && this._selectedElement.get();
   }
   set selectedElement(value) {
     if (!value) {
       delete this._selectedElement;
     } else {
       this._selectedElement = Cu.getWeakReference(value);
     }
   }
 
   /**
+   * Focuses and moves keyboard selection to the first navigable element.
+   * This is a no-op if there are no navigable elements.
+   */
+  focusFirstNavigableElement() {
+    this.selectedElement = this._getNavigableElements()[0];
+    this.focusSelectedElement();
+  }
+
+  /**
    * Based on going up or down, select the previous or next focusable button.
    *
    * @param {Boolean} isDown   whether we're going down (true) or up (false).
    *
    * @return {DOMNode} the button we selected.
    */
   moveSelection(isDown) {
     let buttons = this.buttons;
@@ -1467,17 +1476,17 @@ var PanelView = class extends Associated
    */
   keyNavigation(event, dir) {
     if (!this.active) {
       return;
     }
 
     let buttons = this.buttons;
     if (!buttons || !buttons.length) {
-      buttons = this.buttons = this.getNavigableElements();
+      buttons = this.buttons = this._getNavigableElements();
       // Set the 'tabindex' attribute on the buttons to make sure they're focussable.
       for (let button of buttons) {
         if (!button.classList.contains("subviewbutton-back") &&
             !button.hasAttribute("tabindex")) {
           button.setAttribute("tabindex", 0);
         }
       }
     }
--- a/browser/components/customizableui/test/browser_panel_keyboard_navigation.js
+++ b/browser/components/customizableui/test/browser_panel_keyboard_navigation.js
@@ -5,17 +5,17 @@
  */
 
 const {PanelView} = ChromeUtils.import("resource:///modules/PanelMultiView.jsm", {});
 const kHelpButtonId = "appMenu-help-button";
 
 add_task(async function testUpDownKeys() {
   await gCUITestUtils.openMainMenu();
 
-  let buttons = PanelView.forNode(PanelUI.mainView).getNavigableElements();
+  let buttons = PanelView.forNode(PanelUI.mainView)._getNavigableElements();
 
   for (let button of buttons) {
     if (button.disabled)
       continue;
     EventUtils.synthesizeKey("KEY_ArrowDown");
     Assert.equal(document.commandDispatcher.focusedElement, button,
       "The correct button should be focused after navigating downward");
   }
@@ -34,34 +34,34 @@ add_task(async function testUpDownKeys()
   }
 
   await gCUITestUtils.hideMainMenu();
 });
 
 add_task(async function testEnterKeyBehaviors() {
   await gCUITestUtils.openMainMenu();
 
-  let buttons = PanelView.forNode(PanelUI.mainView).getNavigableElements();
+  let buttons = PanelView.forNode(PanelUI.mainView)._getNavigableElements();
 
   // Navigate to the 'Help' button, which points to a subview.
   EventUtils.synthesizeKey("KEY_ArrowUp");
   let focusedElement = document.commandDispatcher.focusedElement;
   Assert.equal(focusedElement, buttons[buttons.length - 1],
     "The last button should be focused after navigating upward");
 
   let promise = BrowserTestUtils.waitForEvent(PanelUI.helpView, "ViewShown");
   // Make sure the Help button is in focus.
   while (!focusedElement || !focusedElement.id || focusedElement.id != kHelpButtonId) {
     EventUtils.synthesizeKey("KEY_ArrowUp");
     focusedElement = document.commandDispatcher.focusedElement;
   }
   EventUtils.synthesizeKey("KEY_Enter");
   await promise;
 
-  let helpButtons = PanelView.forNode(PanelUI.helpView).getNavigableElements();
+  let helpButtons = PanelView.forNode(PanelUI.helpView)._getNavigableElements();
   Assert.ok(helpButtons[0].classList.contains("subviewbutton-back"),
     "First button in help view should be a back button");
 
   // For posterity, check navigating the subview using up/ down arrow keys as well.
   for (let i = helpButtons.length - 1; i >= 0; --i) {
     let button = helpButtons[i];
     if (button.disabled)
       continue;
@@ -127,17 +127,17 @@ add_task(async function testLeftRightKey
                "Help button should be focused again now that we're back in the main view");
 
   await gCUITestUtils.hideMainMenu();
 });
 
 add_task(async function testTabKey() {
   await gCUITestUtils.openMainMenu();
 
-  let buttons = PanelView.forNode(PanelUI.mainView).getNavigableElements();
+  let buttons = PanelView.forNode(PanelUI.mainView)._getNavigableElements();
 
   for (let button of buttons) {
     if (button.disabled)
       continue;
     EventUtils.synthesizeKey("KEY_Tab");
     Assert.equal(document.commandDispatcher.focusedElement, button,
       "The correct button should be focused after tabbing");
   }
@@ -160,17 +160,17 @@ add_task(async function testTabKey() {
     "Pressing shift + tab should cycle around and select the last button again");
 
   await gCUITestUtils.hideMainMenu();
 });
 
 add_task(async function testInterleavedTabAndArrowKeys() {
   await gCUITestUtils.openMainMenu();
 
-  let buttons = PanelView.forNode(PanelUI.mainView).getNavigableElements();
+  let buttons = PanelView.forNode(PanelUI.mainView)._getNavigableElements();
   let tab = false;
 
   for (let button of buttons) {
     if (button.disabled)
       continue;
     if (tab) {
       EventUtils.synthesizeKey("KEY_Tab");
     } else {
@@ -183,17 +183,17 @@ add_task(async function testInterleavedT
     "The last button should be focused after a mix of Tab and ArrowDown");
 
   await gCUITestUtils.hideMainMenu();
 });
 
 add_task(async function testSpaceDownAfterTabNavigation() {
   await gCUITestUtils.openMainMenu();
 
-  let buttons = PanelView.forNode(PanelUI.mainView).getNavigableElements();
+  let buttons = PanelView.forNode(PanelUI.mainView)._getNavigableElements();
   let button;
 
   for (button of buttons) {
     if (button.disabled)
       continue;
     EventUtils.synthesizeKey("KEY_Tab");
     if (button.id == kHelpButtonId) {
       break;