Bug 1473748 - Part 2 - Simplify how navigable elements are initialized. r=johannh draft
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Mon, 23 Jul 2018 11:28:23 +0100
changeset 821456 3076c2d5f1b1137c9daba937495a52ea442fad85
parent 821455 b566a9b8bfa9461d530dfbdc4c55cdb7994a7d69
push id117100
push userpaolo.mozmail@amadzone.org
push dateMon, 23 Jul 2018 10:30:24 +0000
reviewersjohannh
bugs1473748
milestone63.0a1
Bug 1473748 - Part 2 - Simplify how navigable elements are initialized. r=johannh MozReview-Commit-ID: HN2ZYXNVi6m
browser/components/customizableui/PanelMultiView.jsm
browser/components/customizableui/test/browser_panel_keyboard_navigation.js
--- a/browser/components/customizableui/PanelMultiView.jsm
+++ b/browser/components/customizableui/PanelMultiView.jsm
@@ -1353,38 +1353,46 @@ var PanelView = class extends Associated
     // Now we can make all the necessary DOM changes at once.
     for (let { element, bounds } of items) {
       gMultiLineElementsMap.set(element, { bounds, textContent: element.textContent });
       element.style.height = bounds.height + "px";
     }
   }
 
   /**
-   * Retrieves the button elements that can be used for navigation using the
-   * keyboard, that is all enabled buttons including the back button if visible.
+   * Array of enabled elements that can be selected with the keyboard. This
+   * means all buttons, menulists, and text links including the back button.
    *
-   * @return {Array}
+   * This list is cached until the view is closed, so elements that become
+   * enabled later may not be navigable.
    */
-  _getNavigableElements() {
-    let buttons = Array.from(this.node.querySelectorAll(
+  get _navigableElements() {
+    if (this.__navigableElements) {
+      return this.__navigableElements;
+    }
+
+    let navigableElements = Array.from(this.node.querySelectorAll(
       ":-moz-any(button,toolbarbutton,menulist,.text-link):not([disabled])"));
     let dwu = this._dwu;
-    return buttons.filter(button => {
-      let bounds = dwu.getBoundsWithoutFlushing(button);
+    return this.__navigableElements = navigableElements.filter(element => {
+      // Set the "tabindex" attribute to make sure the element is focusable.
+      if (!element.hasAttribute("tabindex")) {
+        element.setAttribute("tabindex", "0");
+      }
+      let bounds = dwu.getBoundsWithoutFlushing(element);
       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.
+   * The element is usually, but not necessarily, among the _navigableElements.
    */
   get selectedElement() {
     return this._selectedElement && this._selectedElement.get();
   }
   set selectedElement(value) {
     if (!value) {
       delete this._selectedElement;
     } else {
@@ -1392,29 +1400,29 @@ var PanelView = class extends Associated
     }
   }
 
   /**
    * 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.selectedElement = this._navigableElements[0];
     this.focusSelectedElement();
   }
 
   /**
-   * Based on going up or down, select the previous or next focusable button.
+   * Based on going up or down, select the previous or next focusable element.
    *
    * @param {Boolean} isDown   whether we're going down (true) or up (false).
    *
-   * @return {DOMNode} the button we selected.
+   * @return {DOMNode} the element we selected.
    */
   moveSelection(isDown) {
-    let buttons = this.buttons;
+    let buttons = this._navigableElements;
     let lastSelected = this.selectedElement;
     let newButton = null;
     let maxIdx = buttons.length - 1;
     if (lastSelected) {
       let buttonIndex = buttons.indexOf(lastSelected);
       if (buttonIndex != -1) {
         // Buttons may get selected whilst the panel is shown, so add an extra
         // check here.
@@ -1474,29 +1482,20 @@ var PanelView = class extends Associated
    * @param {String} dir
    *        Direction for arrow navigation, either "ltr" or "rtl".
    */
   keyNavigation(event, dir) {
     if (!this.active) {
       return;
     }
 
-    let buttons = this.buttons;
-    if (!buttons || !buttons.length) {
-      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);
-        }
-      }
+    let buttons = this._navigableElements;
+    if (!buttons.length) {
+      return;
     }
-    if (!buttons.length)
-      return;
 
     let stop = () => {
       event.stopPropagation();
       event.preventDefault();
     };
 
     let keyCode = event.code;
     switch (keyCode) {
@@ -1555,16 +1554,16 @@ var PanelView = class extends Associated
       selected.focus();
     }
   }
 
   /**
    * Clear all traces of keyboard navigation happening right now.
    */
   clearNavigation() {
-    delete this.buttons;
+    delete this.__navigableElements;
     let selected = this.selectedElement;
     if (selected) {
       selected.blur();
       this.selectedElement = null;
     }
   }
 };
--- a/browser/components/customizableui/test/browser_panel_keyboard_navigation.js
+++ b/browser/components/customizableui/test/browser_panel_keyboard_navigation.js
@@ -2,20 +2,29 @@
 
 /**
  * Test keyboard navigation in the app menu panel.
  */
 
 const {PanelView} = ChromeUtils.import("resource:///modules/PanelMultiView.jsm", {});
 const kHelpButtonId = "appMenu-help-button";
 
+function getEnabledNavigableElementsForView(panelView) {
+  return Array.from(panelView.querySelectorAll(
+    "button,toolbarbutton,menulist,.text-link"
+  )).filter(element => {
+    let bounds = element.getBoundingClientRect();
+    return !element.disabled && (bounds.width > 0 && bounds.height > 0);
+  });
+}
+
 add_task(async function testUpDownKeys() {
   await gCUITestUtils.openMainMenu();
 
-  let buttons = PanelView.forNode(PanelUI.mainView)._getNavigableElements();
+  let buttons = getEnabledNavigableElementsForView(PanelUI.mainView);
 
   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 +43,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 = getEnabledNavigableElementsForView(PanelUI.mainView);
 
   // 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 = getEnabledNavigableElementsForView(PanelUI.helpView);
   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 +136,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 = getEnabledNavigableElementsForView(PanelUI.mainView);
 
   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 +169,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 = getEnabledNavigableElementsForView(PanelUI.mainView);
   let tab = false;
 
   for (let button of buttons) {
     if (button.disabled)
       continue;
     if (tab) {
       EventUtils.synthesizeKey("KEY_Tab");
     } else {
@@ -183,17 +192,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 = getEnabledNavigableElementsForView(PanelUI.mainView);
   let button;
 
   for (button of buttons) {
     if (button.disabled)
       continue;
     EventUtils.synthesizeKey("KEY_Tab");
     if (button.id == kHelpButtonId) {
       break;