Submenus are now fully navigable via keyboard.

app/browser/lib/menuUtil.js

@@ -18,10 +18,12 @@ const locale = require('../../locale')
 module.exports.getMenuItem = (appMenu, label) => {
   if (appMenu && appMenu.items && appMenu.items.length > 0) {
     for (let i = 0; i < appMenu.items.length; i++) {
-      const menuItem = appMenu.items[i].submenu && appMenu.items[i].submenu.items.find(function (item) {
-        return item && item.label === label
-      })
-      if (menuItem) return menuItem
+      const item = appMenu.items[i]
+      if (item && item.label === label) return item
+      if (item.submenu) {
+        const nestedItem = module.exports.getMenuItem(item.submenu, label)
+        if (nestedItem) return nestedItem
+      }
     }
   }
   return null

app/renderer/components/menubar.js

@@ -45,20 +45,24 @@ class MenubarItem extends ImmutableComponent {
       e.stopPropagation()
     }
     // If clicking on an already selected item, deselect it
-    const selected = this.props.menubar.props.selectedLabel
-    if (selected && selected === this.props.label) {
+    const selected = this.props.menubar.props.selectedIndex
+      ? this.props.menubar.props.selectedIndex[0]
+      : null
+    if (selected && selected === this.props.index) {
       windowActions.setContextMenuDetail()
-      windowActions.setMenubarSelectedLabel()
+      windowActions.setSubmenuSelectedIndex()
       return
     }
     // Otherwise, mark item as selected and show its context menu
-    windowActions.setMenubarSelectedLabel(this.props.label)
+    windowActions.setSubmenuSelectedIndex([this.props.index])
     const rect = e.target.getBoundingClientRect()
     showContextMenu(rect, this.props.submenu, this.props.lastFocusedSelector)
   }
   onMouseOver (e) {
-    const selected = this.props.menubar.props.selectedLabel
-    if (selected && selected !== this.props.label) {
+    const selected = this.props.menubar.props.selectedIndex
+      ? this.props.menubar.props.selectedIndex[0]
+      : null
+    if (typeof selected === 'number' && selected !== this.props.index) {
       this.onClick(e)
     }
   }
@@ -67,7 +71,7 @@ class MenubarItem extends ImmutableComponent {
       className={'menubarItem' + (this.props.selected ? ' selected' : '')}
       onClick={this.onClick}
       onMouseOver={this.onMouseOver}
-      data-label={this.props.label}>
+      data-index={this.props.index}>
       { this.props.label }
     </span>
   }
@@ -89,118 +93,215 @@ class Menubar extends ImmutableComponent {
   componentWillUnmount () {
     document.removeEventListener('keydown', this.onKeyDown)
   }
-  getTemplateByLabel (label) {
-    const element = this.props.template.find((element) => {
-      return element.get('label') === label
-    })
-    return element ? element.get('submenu') : null
-  }
-  get selectedTemplate () {
-    return this.getTemplateByLabel(this.props.selectedLabel)
+
+  /**
+   * Used to get the submenu of a top level menu like File, Edit, etc.
+   * Index will default to the selected menu if not provided / valid.
+   */
+  getTemplate (index) {
+    if (typeof index !== 'number') index = this.props.selectedIndex[0]
+    return this.props.template.get(index).get('submenu')
   }
-  get selectedTemplateItemsOnly () {
-    // exclude the separators AND items that are not visible
-    return this.selectedTemplate.filter((element) => {
+  /**
+   * Same as getTemplate but excluding line separators and items that are not visible.
+   */
+  getTemplateItemsOnly (index) {
+    return this.getTemplate(index).filter((element) => {
       if (element.get('type') === separatorMenuItem.type) return false
       if (element.has('visible')) return element.get('visible')
       return true
     })
   }
-  get selectedIndexMax () {
-    const result = this.selectedTemplateItemsOnly
-    if (result && result.size && result.size > 0) {
-      return result.size
-    }
-    return 0
-  }
-  getRectByLabel (label) {
-    const selected = document.querySelectorAll('.menubar .menubarItem[data-label=\'' + label + '\']')
+  /**
+   * Get client rect for the MenubarItem controls.
+   * Used to position the context menu object.
+   */
+  getMenubarItemBounds (index) {
+    if (typeof index !== 'number') index = this.props.selectedIndex[0]
+    const selected = document.querySelectorAll('.menubar .menubarItem[data-index=\'' + index + '\']')
     if (selected.length === 1) {
       return selected.item(0).getBoundingClientRect()
     }
     return null
   }
-  get selectedRect () {
-    return this.getRectByLabel(this.props.selectedLabel)
+  /**
+   * Get client rect for the actively selected ContextMenu.
+   * Used to position the child menu if parent has children.
+   */
+  getContextMenuItemBounds () {
+    const selected = document.querySelectorAll('.contextMenuItem.selectedByKeyboard')
+    if (selected.length > 0) {
+      return selected.item(selected.length - 1).getBoundingClientRect()
+    }
+    return null
+  }
+  /**
+   * Returns index for the active / focused menu.
+   */
+  get currentIndex () {
+    return this.props.selectedIndex[this.props.selectedIndex.length - 1]
+  }
+  /**
+   * Upper bound for the active / focused menu.
+   */
+  get maxIndex () {
+    return this.getMenuByIndex().size - 1
+  }
+  /**
+   * Returns true is current state is inside a regular menu.
+   */
+  get hasMenuSelection () {
+    return this.props.selectedIndex.length > 1
   }
+  /**
+   * Returns true if current state is inside a submenu.
+   */
+  get hasSubmenuSelection () {
+    return this.props.selectedIndex.length > 2
+  }
+  /**
+   * Fetch menu based on selected index.
+   * Will navigate children to find nested menus.
+   */
+  getMenuByIndex (parentItem, currentDepth) {
+    if (!parentItem) parentItem = this.getTemplateItemsOnly()
+    if (!currentDepth) currentDepth = 0
+
+    const selectedIndices = this.props.selectedIndex.slice(1)
+    if (selectedIndices.length === 0) return parentItem
+
+    const submenuIndex = selectedIndices[currentDepth]
+    const childItem = parentItem.get(submenuIndex)
+
+    if (childItem && childItem.get('submenu') && currentDepth < (selectedIndices.length - 1)) {
+      return this.getMenuByIndex(childItem.get('submenu'), currentDepth + 1)
+    }
+
+    return parentItem
+  }
+
   onKeyDown (e) {
+    const selectedIndex = this.props.selectedIndex
+
+    if (!selectedIndex || !this.props.template) return
+
     switch (e.which) {
       case keyCodes.ENTER:
         e.preventDefault()
-        if (this.selectedTemplate) {
-          const selectedLabel = this.selectedTemplateItemsOnly.getIn([this.props.selectedIndex, 'label'])
-          windowActions.clickMenubarSubmenu(selectedLabel)
-          windowActions.resetMenuState()
-        }
+        const selectedLabel = this.getMenuByIndex().getIn([this.currentIndex, 'label'])
+        windowActions.clickMenubarSubmenu(selectedLabel)
+        windowActions.resetMenuState()
         break
 
       case keyCodes.LEFT:
       case keyCodes.RIGHT:
-        if (!this.props.autohide && !this.props.selectedLabel) break
-
         e.preventDefault()
-        if (this.props.template.size > 0) {
-          const selectedIndex = this.props.template.findIndex((element) => {
-            return element.get('label') === this.props.selectedLabel
-          })
-          const nextIndex = selectedIndex === -1
-            ? 0
-            : wrappingClamp(
-              selectedIndex + (e.which === keyCodes.LEFT ? -1 : 1),
-              0,
-              this.props.template.size - 1)
 
-          // BSCTODO: consider submenus (ex: for bookmark folders)
+        // Left arrow inside a submenu
+        // <= go back one level
+        if (e.which === keyCodes.LEFT && this.hasSubmenuSelection) {
+          const newIndices = selectedIndex.slice()
+          newIndices.pop()
+          windowActions.setSubmenuSelectedIndex(newIndices)
 
-          const nextLabel = this.props.template.getIn([nextIndex, 'label'])
-          const nextRect = this.getRectByLabel(nextLabel)
+          let openedSubmenuDetails = this.props.contextMenuDetail.get('openedSubmenuDetails')
+            ? this.props.contextMenuDetail.get('openedSubmenuDetails')
+            : new Immutable.List()
+          openedSubmenuDetails = openedSubmenuDetails.pop()
 
-          windowActions.setMenubarSelectedLabel(nextLabel)
+          windowActions.setContextMenuDetail(this.props.contextMenuDetail.set('openedSubmenuDetails', openedSubmenuDetails))
+          break
+        }
 
-          // Context menu already being displayed; auto-open the next one
-          if (this.props.contextMenuDetail && this.selectedTemplate && nextRect) {
-            windowActions.setSubmenuSelectedIndex(0)
-            showContextMenu(nextRect, this.getTemplateByLabel(nextLabel).toJS(), this.props.lastFocusedSelector)
-          }
+        const selectedMenuItem = selectedIndex
+          ? this.getMenuByIndex().get(this.currentIndex)
+          : null
+
+        // Right arrow on a menu item which has a submenu
+        // => go up one level (default next menu to item 0)
+        if (e.which === keyCodes.RIGHT && selectedMenuItem && selectedMenuItem.has('submenu')) {
+          const newIndices = selectedIndex.slice()
+          newIndices.push(0)
+          windowActions.setSubmenuSelectedIndex(newIndices)
+
+          let openedSubmenuDetails = this.props.contextMenuDetail.get('openedSubmenuDetails')
+            ? this.props.contextMenuDetail.get('openedSubmenuDetails')
+            : new Immutable.List()
+
+          const rect = this.getContextMenuItemBounds()
+          const itemHeight = (rect.bottom - rect.top)
+
+          openedSubmenuDetails = openedSubmenuDetails.push(Immutable.fromJS({
+            y: (rect.top - itemHeight),
+            template: selectedMenuItem.get('submenu')
+          }))
+
+          windowActions.setContextMenuDetail(this.props.contextMenuDetail.set('openedSubmenuDetails', openedSubmenuDetails))
+          break
+        }
+
+        // Regular old menu item
+        const nextIndex = selectedIndex === null
+          ? 0
+          : wrappingClamp(
+              selectedIndex[0] + (e.which === keyCodes.LEFT ? -1 : 1),
+              0,
+              this.props.template.size - 1)
+
+        // Context menu already being displayed; auto-open the next one
+        if (this.props.contextMenuDetail) {
+          windowActions.setSubmenuSelectedIndex([nextIndex, 0])
+          showContextMenu(this.getMenubarItemBounds(nextIndex), this.getTemplate(nextIndex).toJS(), this.props.lastFocusedSelector)
+        } else {
+          windowActions.setSubmenuSelectedIndex([nextIndex])
         }
         break
 
       case keyCodes.UP:
       case keyCodes.DOWN:
-        if (!this.props.autohide && !this.props.selectedLabel) break
-
         e.preventDefault()
-        if (this.props.selectedLabel && this.selectedTemplate) {
-          if (!this.props.contextMenuDetail && this.selectedRect) {
+        if (this.getTemplateItemsOnly()) {
+          if (!this.props.contextMenuDetail) {
             // First time hitting up/down; popup the context menu
-            windowActions.setSubmenuSelectedIndex(0)
-            showContextMenu(this.selectedRect, this.selectedTemplate.toJS(), this.props.lastFocusedSelector)
+            const newIndices = selectedIndex.slice()
+            newIndices.push(0)
+            windowActions.setSubmenuSelectedIndex(newIndices)
+            showContextMenu(this.getMenubarItemBounds(), this.getTemplate().toJS(), this.props.lastFocusedSelector)
           } else {
             // Context menu already visible; move selection up or down
             const nextIndex = wrappingClamp(
-              this.props.selectedIndex + (e.which === keyCodes.UP ? -1 : 1),
+              this.currentIndex + (e.which === keyCodes.UP ? -1 : 1),
               0,
-              this.selectedIndexMax - 1)
-            windowActions.setSubmenuSelectedIndex(nextIndex)
+              this.maxIndex)
+
+            const newIndices = selectedIndex.slice()
+            if (this.hasMenuSelection) {
+              newIndices[selectedIndex.length - 1] = nextIndex
+            } else {
+              newIndices.push(0)
+            }
+            windowActions.setSubmenuSelectedIndex(newIndices)
           }
         }
         break
     }
   }
   shouldComponentUpdate (nextProps, nextState) {
-    return this.props.selectedLabel !== nextProps.selectedLabel
+    return this.props.selectedIndex !== nextProps.selectedIndex
   }
   render () {
+    let i = 0
     return <div className='menubar'>
       {
         this.props.template.map((menubarItem) => {
           let props = {
             label: menubarItem.get('label'),
+            index: i++,
             submenu: menubarItem.get('submenu').toJS(),
-            menubar: this,
-            lastFocusedSelector: this.props.lastFocusedSelector
+            lastFocusedSelector: this.props.lastFocusedSelector,
+            menubar: this
           }
-          if (props.label === this.props.selectedLabel) {
+          if (this.props.selectedIndex && props.index === this.props.selectedIndex[0]) {
             props.selected = true
           }
           return <MenubarItem {...props} />

docs/state.md

@@ -374,8 +374,7 @@ WindowStore
     },
     menubar: { // windows only
       isVisible: boolean, // true if Menubar control is visible
-      selectedLabel: string, // label of menu that is selected (or null for none selected)
-      selectedIndex: number, // index of the selected context menu item
+      selectedIndex: Array<number>, // indices of the selected menu item(s) (or null for none selected)
       lastFocusedSelector: string // selector for the last selected element (browser ui, not frame content)
     }
   },

docs/windowActions.md

@@ -831,17 +831,6 @@ Called from the Menubar control, handled in menu.js
 
 
 
-### setMenubarSelectedLabel(label) 
-
-(Windows only)
-Used to track which menubar item is currently selected (or null for none selected)
-
-**Parameters**
-
-**label**: `string`, text of the menubar item label that was clicked (file, edit, etc)
-
-
-
 ### resetMenuState() 
 
 Used by `main.js` when click happens on content area (not on a link or react control).

js/actions/windowActions.js

@@ -1036,11 +1036,10 @@ const windowActions = {
    * Dispatches a message to indicate the custom rendered Menubar should be toggled (shown/hidden)
    * @param {boolean} isVisible (optional)
    */
-  toggleMenubarVisible: function (isVisible, defaultLabel) {
+  toggleMenubarVisible: function (isVisible) {
     dispatch({
       actionType: WindowConstants.WINDOW_TOGGLE_MENUBAR_VISIBLE,
-      isVisible,
-      defaultLabel
+      isVisible
     })
   },
 
@@ -1057,18 +1056,6 @@ const windowActions = {
     })
   },
 
-  /**
-   * (Windows only)
-   * Used to track which menubar item is currently selected (or null for none selected)
-   * @param {string} label - text of the menubar item label that was clicked (file, edit, etc)
-   */
-  setMenubarSelectedLabel: function (label) {
-    dispatch({
-      actionType: WindowConstants.WINDOW_SET_MENUBAR_SELECTED_LABEL,
-      label
-    })
-  },
-
   /**
    * Used by `main.js` when click happens on content area (not on a link or react control).
    * - closes context menu