Add menu item order control (#12362)

Add four new optional properties to menus in Electron. The four properties are:
'before'
'after'
'beforeGroupContaining'
'afterGroupContaining'

'before/after' - provides a means for a single context menu item to declare its placement relative to another context menu item. These also imply that menu item in question should be placed in the same “group” as the item.

'beforeGroupContaining/afterGroupContaining - provides a means for a single menu item to declare the placement of its containing group, relative to the containing group of the specified item.

docs/api/menu.md

@@ -302,27 +302,20 @@ browser windows.
 
 ## Menu Item Position
 
-You can make use of `position` and `id` to control how the item will be placed
+You can make use of `before`, `after`, `beforeGroupContaining`, `afterGroupContaining` and `id` to control how the item will be placed when building a menu with `Menu.buildFromTemplate`.
-when building a menu with `Menu.buildFromTemplate`.
 
-The `position` attribute of `MenuItem` has the form `[placement]=[id]`, where
+* `before` - Inserts this item before the item with the specified label. If the
-`placement` is one of `before`, `after`, or `endof` and `id` is the unique ID of
-an existing item in the menu:
-
-* `before` - Inserts this item before the id referenced item. If the
   referenced item doesn't exist the item will be inserted at the end of
-  the menu.
+  the menu. Also implies that the menu item in question should be placed in the same “group” as the item.
-* `after` - Inserts this item after id referenced item. If the referenced
+* `after` - Inserts this item after the item with the specified label. If the
-  item doesn't exist the item will be inserted at the end of the menu.
+  referenced item doesn't exist the item will be inserted at the end of
-* `endof` - Inserts this item at the end of the logical group containing
+  the menu. Also implies that the menu item in question should be placed in the same “group” as the item.
-  the id referenced item (groups are created by separator items). If
+* `beforeGroupContaining` - Provides a means for a single context menu to declare
-  the referenced item doesn't exist, a new separator group is created with
+  the placement of their containing group before the containing group of the item with the specified label.
-  the given id and this item is inserted after that separator.
+* `afterGroupContaining` - Provides a means for a single context menu to declare
+  the placement of their containing group after the containing group of the item with the specified label.
 
-When an item is positioned, all un-positioned items are inserted after
+By default, items will be inserted in the order they exist in the template unless one of the specified positioning keywords is used.
-it until a new item is positioned. So if you want to position a group of
-menu items in the same location you only need to specify a position for
-the first item.
 
 ### Examples
 
@@ -330,11 +323,10 @@ Template:
 
 ```javascript
 [
-  {label: '4', id: '4'},
+  { id: '1', label: 'one' },
-  {label: '5', id: '5'},
+  { id: '2', label: 'two' },
-  {label: '1', id: '1', position: 'before=4'},
+  { id: '3', label: 'three' },
-  {label: '2', id: '2'},
+  { id: '4', label: 'four' }
-  {label: '3', id: '3'}
 ]
 ```
 
@@ -345,19 +337,39 @@ Menu:
 - 2
 - 3
 - 4
-- 5
 ```
 
 Template:
 
 ```javascript
 [
-  {label: 'a', position: 'endof=letters'},
+  { id: '1', label: 'one' },
-  {label: '1', position: 'endof=numbers'},
+  { type: 'separator' },
-  {label: 'b', position: 'endof=letters'},
+  { id: '3', label: 'three', beforeGroupContaining: ['1'] },
-  {label: '2', position: 'endof=numbers'},
+  { id: '4', label: 'four', afterGroupContaining: ['2'] },
-  {label: 'c', position: 'endof=letters'},
+  { type: 'separator' },
-  {label: '3', position: 'endof=numbers'}
+  { id: '2', label: 'two' }
+]
+```
+
+Menu:
+
+```sh
+- 3
+- 4
+- ---
+- 1
+- ---
+- 2
+```
+
+Template:
+
+```javascript
+[
+  { id: '1', label: 'one', after: ['3'] },
+  { id: '2', label: 'two', before: ['1'] },
+  { id: '3', label: 'three' }
 ]
 ```
 
@@ -365,13 +377,9 @@ Menu:
 
 ```sh
 - ---
-- a
-- b
-- c
-- ---
-- 1
-- 2
 - 3
+- 2
+- 1
 ```
 
 [AboutInformationPropertyListFiles]: https://developer.apple.com/library/ios/documentation/general/Reference/InfoPlistKeyReference/Articles/AboutInformationPropertyListFiles.html

filenames.gypi

@@ -23,6 +23,7 @@
       'lib/browser/api/in-app-purchase.js',
       'lib/browser/api/menu-item-roles.js',
       'lib/browser/api/menu-item.js',
+      'lib/browser/api/menu-utils.js',
       'lib/browser/api/menu.js',
       'lib/browser/api/module-list.js',
       'lib/browser/api/navigation-controller.js',

lib/browser/api/menu-utils.js

@@ -0,0 +1,168 @@
+function splitArray (arr, predicate) {
+  const result = arr.reduce((multi, item) => {
+    const current = multi[multi.length - 1]
+    if (predicate(item)) {
+      if (current.length > 0) multi.push([])
+    } else {
+      current.push(item)
+    }
+    return multi
+  }, [[]])
+
+  if (result[result.length - 1].length === 0) {
+    return result.slice(0, result.length - 1)
+  }
+  return result
+}
+
+function joinArrays (arrays, joiner) {
+  return arrays.reduce((joined, arr, i) => {
+    if (i > 0 && arr.length) {
+      joined.push(joiner)
+    }
+    return joined.concat(arr)
+  }, [])
+}
+
+function pushOntoMultiMap (map, key, value) {
+  if (!map.has(key)) {
+    map.set(key, [])
+  }
+  map.get(key).push(value)
+}
+
+function indexOfGroupContainingID (groups, id, ignoreGroup) {
+  return groups.findIndex(
+    candidateGroup =>
+      candidateGroup !== ignoreGroup &&
+      candidateGroup.some(
+        candidateItem => candidateItem.id === id
+      )
+  )
+}
+
+// Sort nodes topologically using a depth-first approach. Encountered cycles
+// are broken.
+function sortTopologically (originalOrder, edgesById) {
+  const sorted = []
+  const marked = new Set()
+
+  const visit = (mark) => {
+    if (marked.has(mark)) return
+    marked.add(mark)
+    const edges = edgesById.get(mark)
+    if (edges != null) {
+      edges.forEach(visit)
+    }
+    sorted.push(mark)
+  }
+
+  originalOrder.forEach(visit)
+  return sorted
+}
+
+function attemptToMergeAGroup (groups) {
+  for (let i = 0; i < groups.length; i++) {
+    const group = groups[i]
+    for (const item of group) {
+      const toIDs = [...(item.before || []), ...(item.after || [])]
+      for (const id of toIDs) {
+        const index = indexOfGroupContainingID(groups, id, group)
+        if (index === -1) continue
+        const mergeTarget = groups[index]
+
+        mergeTarget.push(...group)
+        groups.splice(i, 1)
+        return true
+      }
+    }
+  }
+  return false
+}
+
+function mergeGroups (groups) {
+  let merged = true
+  while (merged) {
+    merged = attemptToMergeAGroup(groups)
+  }
+  return groups
+}
+
+function sortItemsInGroup (group) {
+  const originalOrder = group.map((node, i) => i)
+  const edges = new Map()
+  const idToIndex = new Map(group.map((item, i) => [item.id, i]))
+
+  group.forEach((item, i) => {
+    if (item.before) {
+      item.before.forEach(toID => {
+        const to = idToIndex.get(toID)
+        if (to != null) {
+          pushOntoMultiMap(edges, to, i)
+        }
+      })
+    }
+    if (item.after) {
+      item.after.forEach(toID => {
+        const to = idToIndex.get(toID)
+        if (to != null) {
+          pushOntoMultiMap(edges, i, to)
+        }
+      })
+    }
+  })
+
+  const sortedNodes = sortTopologically(originalOrder, edges)
+  return sortedNodes.map(i => group[i])
+}
+
+function findEdgesInGroup (groups, i, edges) {
+  const group = groups[i]
+  for (const item of group) {
+    if (item.beforeGroupContaining) {
+      for (const id of item.beforeGroupContaining) {
+        const to = indexOfGroupContainingID(groups, id, group)
+        if (to !== -1) {
+          pushOntoMultiMap(edges, to, i)
+          return
+        }
+      }
+    }
+    if (item.afterGroupContaining) {
+      for (const id of item.afterGroupContaining) {
+        const to = indexOfGroupContainingID(groups, id, group)
+        if (to !== -1) {
+          pushOntoMultiMap(edges, i, to)
+          return
+        }
+      }
+    }
+  }
+}
+
+function sortGroups (groups) {
+  const originalOrder = groups.map((item, i) => i)
+  const edges = new Map()
+
+  for (let i = 0; i < groups.length; i++) {
+    findEdgesInGroup(groups, i, edges)
+  }
+
+  const sortedGroupIndexes = sortTopologically(originalOrder, edges)
+  return sortedGroupIndexes.map(i => groups[i])
+}
+
+function sortMenuItems (menuItems) {
+  const isSeparator = (item) => item.type === 'separator'
+
+  // Split the items into their implicit groups based upon separators.
+  const groups = splitArray(menuItems, isSeparator)
+  const mergedGroups = mergeGroups(groups)
+  const mergedGroupsWithSortedItems = mergedGroups.map(sortItemsInGroup)
+  const sortedGroups = sortGroups(mergedGroupsWithSortedItems)
+
+  const joined = joinArrays(sortedGroups, { type: 'separator' })
+  return joined
+}
+
+module.exports = {sortMenuItems}

lib/browser/api/menu.js

@@ -1,6 +1,7 @@
 'use strict'
 
 const {TopLevelWindow, MenuItem, webContents} = require('electron')
+const {sortMenuItems} = require('./menu-utils')
 const EventEmitter = require('events').EventEmitter
 const v8Util = process.atomBinding('v8_util')
 const bindings = process.atomBinding('menu')
@@ -160,18 +161,9 @@ Menu.buildFromTemplate = function (template) {
 
   const menu = new Menu()
   const filtered = removeExtraSeparators(template)
+  const sorted = sortTemplate(filtered)
 
-  const positioned = []
+  sorted.forEach((item) => {
-  let idx = 0
-
-  // sort template by position
-  filtered.forEach(item => {
-    idx = (item.position) ? indexToInsertByPosition(positioned, item.position) : idx += 1
-    positioned.splice(idx, 0, item)
-  })
-
-  // add each item from positioned menu to application menu
-  positioned.forEach((item) => {
     if (typeof item !== 'object') {
       throw new TypeError('Invalid template for MenuItem')
     }
@@ -183,6 +175,17 @@ Menu.buildFromTemplate = function (template) {
 
 /* Helper Functions */
 
+function sortTemplate (template) {
+  const sorted = sortMenuItems(template)
+  for (let id in sorted) {
+    const item = sorted[id]
+    if (Array.isArray(item.submenu)) {
+      item.submenu = sortTemplate(item.submenu)
+    }
+  }
+  return sorted
+}
+
 // Search between separators to find a radio menu item and return its group id
 function generateGroupId (items, pos) {
   if (pos > 0) {
@@ -200,45 +203,6 @@ function generateGroupId (items, pos) {
   return groupIdIndex
 }
 
-function indexOfItemById (items, id) {
-  const foundItem = items.find(item => item.id === id) || -1
-  return items.indexOf(foundItem)
-}
-
-function indexToInsertByPosition (items, position) {
-  if (!position) return items.length
-
-  const [query, id] = position.split('=') // parse query and id from position
-  const idx = indexOfItemById(items, id) // calculate initial index of item
-
-  // warn if query doesn't exist
-  if (idx === -1 && query !== 'endof') {
-    console.warn(`Item with id ${id} is not found`)
-    return items.length
-  }
-
-  // compute new index based on query
-  const queries = {
-    after: (index) => {
-      index += 1
-      return index
-    },
-    endof: (index) => {
-      if (index === -1) {
-        items.push({id, type: 'separator'})
-        index = items.length - 1
-      }
-
-      index += 1
-      while (index < items.length && items[index].type !== 'separator') index += 1
-      return index
-    }
-  }
-
-  // return new index if needed, or original indexOfItemById
-  return (query in queries) ? queries[query](idx) : idx
-}
-
 function removeExtraSeparators (items) {
   // fold adjacent separators together
   let ret = items.filter((e, idx, arr) => {

spec/api-menu-spec.js

@@ -2,6 +2,7 @@ const assert = require('assert')
 
 const {ipcRenderer, remote} = require('electron')
 const {BrowserWindow, Menu, MenuItem} = remote
+const {sortMenuItems} = require('../lib/browser/api/menu-utils')
 const {closeWindow} = require('./window-helpers')
 
 describe('Menu module', () => {
@@ -37,91 +38,445 @@ describe('Menu module', () => {
       })
     })
 
-    describe('Menu.buildFromTemplate should reorder based on item position specifiers', () => {
+    describe('Menu sorting and building', () => {
+      describe('sorts groups', () => {
+        it('does a simple sort', () => {
+          const items = [
+            {
+              label: 'two',
+              id: '2',
+              afterGroupContaining: ['1'] },
+            { type: 'separator' },
+            {
+              id: '1',
+              label: 'one'
+            }
+          ]
+
+          const expected = [
+            {
+              id: '1',
+              label: 'one'
+            },
+            { type: 'separator' },
+            {
+              id: '2',
+              label: 'two',
+              afterGroupContaining: ['1']
+            }
+          ]
+
+          assert.deepEqual(sortMenuItems(items), expected)
+        })
+
+        it('resolves cycles by ignoring things that conflict', () => {
+          const items = [
+            {
+              id: '2',
+              label: 'two',
+              afterGroupContaining: ['1']
+            },
+            { type: 'separator' },
+            {
+              id: '1',
+              label: 'one',
+              afterGroupContaining: ['2']
+            }
+          ]
+
+          const expected = [
+            {
+              id: '1',
+              label: 'one',
+              afterGroupContaining: ['2']
+            },
+            { type: 'separator' },
+            {
+              id: '2',
+              label: 'two',
+              afterGroupContaining: ['1']
+            }
+          ]
+
+          assert.deepEqual(sortMenuItems(items), expected)
+        })
+
+        it('ignores references to commands that do not exist', () => {
+          const items = [
+            {
+              id: '1',
+              label: 'one'
+            },
+            { type: 'separator' },
+            {
+              id: '2',
+              label: 'two',
+              afterGroupContaining: ['does-not-exist']
+            }
+          ]
+
+          const expected = [
+            {
+              id: '1',
+              label: 'one'
+            },
+            { type: 'separator' },
+            {
+              id: '2',
+              label: 'two',
+              afterGroupContaining: ['does-not-exist']
+            }
+          ]
+
+          assert.deepEqual(sortMenuItems(items), expected)
+        })
+
+        it('only respects the first matching [before|after]GroupContaining rule in a given group', () => {
+          const items = [
+            {
+              id: '1',
+              label: 'one'
+            },
+            { type: 'separator' },
+            {
+              id: '3',
+              label: 'three',
+              beforeGroupContaining: ['1']
+            },
+            {
+              id: '4',
+              label: 'four',
+              afterGroupContaining: ['2']
+            },
+            { type: 'separator' },
+            {
+              id: '2',
+              label: 'two'
+            }
+          ]
+
+          const expected = [
+            {
+              id: '3',
+              label: 'three',
+              beforeGroupContaining: ['1']
+            },
+            {
+              id: '4',
+              label: 'four',
+              afterGroupContaining: ['2']
+            },
+            { type: 'separator' },
+            {
+              id: '1',
+              label: 'one'
+            },
+            { type: 'separator' },
+            {
+              id: '2',
+              label: 'two'
+            }
+          ]
+
+          assert.deepEqual(sortMenuItems(items), expected)
+        })
+      })
+
+      describe('moves an item to a different group by merging groups', () => {
+        it('can move a group of one item', () => {
+          const items = [
+            {
+              id: '1',
+              label: 'one'
+            },
+            { type: 'separator' },
+            {
+              id: '2',
+              label: 'two'
+            },
+            { type: 'separator' },
+            {
+              id: '3',
+              label: 'three',
+              after: ['1']
+            },
+            { type: 'separator' }
+          ]
+
+          const expected = [
+            {
+              id: '1',
+              label: 'one'
+            },
+            {
+              id: '3',
+              label: 'three',
+              after: ['1']
+            },
+            { type: 'separator' },
+            {
+              id: '2',
+              label: 'two'
+            }
+          ]
+
+          assert.deepEqual(sortMenuItems(items), expected)
+        })
+
+        it("moves all items in the moving item's group", () => {
+          const items = [
+            {
+              id: '1',
+              label: 'one'
+            },
+            { type: 'separator' },
+            {
+              id: '2',
+              label: 'two'
+            },
+            { type: 'separator' },
+            {
+              id: '3',
+              label: 'three',
+              after: ['1']
+            },
+            {
+              id: '4',
+              label: 'four'
+            },
+            { type: 'separator' }
+          ]
+
+          const expected = [
+            {
+              id: '1',
+              label: 'one'
+            },
+            {
+              id: '3',
+              label: 'three',
+              after: ['1']
+            },
+            {
+              id: '4',
+              label: 'four'
+            },
+            { type: 'separator' },
+            {
+              id: '2',
+              label: 'two'
+            }
+          ]
+
+          assert.deepEqual(sortMenuItems(items), expected)
+        })
+
+        it("ignores positions relative to commands that don't exist", () => {
+          const items = [
+            {
+              id: '1',
+              label: 'one'
+            },
+            { type: 'separator' },
+            {
+              id: '2',
+              label: 'two'
+            },
+            { type: 'separator' },
+            {
+              id: '3',
+              label: 'three',
+              after: ['does-not-exist']
+            },
+            {
+              id: '4',
+              label: 'four',
+              after: ['1']
+            },
+            { type: 'separator' }
+          ]
+
+          const expected = [
+            {
+              id: '1',
+              label: 'one'
+            },
+            {
+              id: '3',
+              label: 'three',
+              after: ['does-not-exist']
+            },
+            {
+              id: '4',
+              label: 'four',
+              after: ['1']
+            },
+            { type: 'separator' },
+            {
+              id: '2',
+              label: 'two'
+            }
+          ]
+
+          assert.deepEqual(sortMenuItems(items), expected)
+        })
+
+        it('can handle recursive group merging', () => {
+          const items = [
+            {
+              id: '1',
+              label: 'one',
+              after: ['3']
+            },
+            {
+              id: '2',
+              label: 'two',
+              before: ['1']
+            },
+            {
+              id: '3',
+              label: 'three'
+            }
+          ]
+
+          const expected = [
+            {
+              id: '3',
+              label: 'three'
+            },
+            {
+              id: '2',
+              label: 'two',
+              before: ['1']
+            },
+            {
+              id: '1',
+              label: 'one',
+              after: ['3']
+            }
+          ]
+
+          assert.deepEqual(sortMenuItems(items), expected)
+        })
+
+        it('can merge multiple groups when given a list of before/after commands', () => {
+          const items = [
+            {
+              id: '1',
+              label: 'one'
+            },
+            { type: 'separator' },
+            {
+              id: '2',
+              label: 'two'
+            },
+            { type: 'separator' },
+            {
+              id: '3',
+              label: 'three',
+              after: ['1', '2']
+            }
+          ]
+
+          const expected = [
+            {
+              id: '2',
+              label: 'two'
+            },
+            {
+              id: '1',
+              label: 'one'
+            },
+            {
+              id: '3',
+              label: 'three',
+              after: ['1', '2']
+            }
+          ]
+
+          assert.deepEqual(sortMenuItems(items), expected)
+        })
+
+        it('can merge multiple groups based on both before/after commands', () => {
+          const items = [
+            {
+              id: '1',
+              label: 'one'
+            },
+            { type: 'separator' },
+            {
+              id: '2',
+              label: 'two'
+            },
+            { type: 'separator' },
+            {
+              id: '3',
+              label: 'three',
+              after: ['1'],
+              before: ['2']
+            }
+          ]
+
+          const expected = [
+            {
+              id: '1',
+              label: 'one'
+            },
+            {
+              id: '3',
+              label: 'three',
+              after: ['1'],
+              before: ['2']
+            },
+            {
+              id: '2',
+              label: 'two'
+            }
+          ]
+
+          assert.deepEqual(sortMenuItems(items), expected)
+        })
+      })
+
       it('should position before existing item', () => {
         const menu = Menu.buildFromTemplate([
           {
-            label: '2',
+            id: '2',
-            id: '2'
+            label: 'two'
           }, {
-            label: '3',
+            id: '3',
-            id: '3'
+            label: 'three'
           }, {
-            label: '1',
             id: '1',
-            position: 'before=2'
+            label: 'one',
+            before: ['2']
           }
         ])
-        assert.equal(menu.items[0].label, '1')
+
-        assert.equal(menu.items[1].label, '2')
+        assert.equal(menu.items[0].label, 'one')
-        assert.equal(menu.items[2].label, '3')
+        assert.equal(menu.items[1].label, 'two')
+        assert.equal(menu.items[2].label, 'three')
       })
 
       it('should position after existing item', () => {
         const menu = Menu.buildFromTemplate([
           {
-            label: '1',
-            id: '1'
-          }, {
-            label: '3',
-            id: '3'
-          }, {
-            label: '2',
             id: '2',
-            position: 'after=1'
+            label: 'two',
-          }
+            after: ['1']
-        ])
+          },
-        assert.equal(menu.items[0].label, '1')
-        assert.equal(menu.items[1].label, '2')
-        assert.equal(menu.items[2].label, '3')
-      })
-
-      it('should position at endof existing separator groups', () => {
-        const menu = Menu.buildFromTemplate([
           {
-            label: 'first',
-            id: 'first'
-          }, {
-            type: 'separator',
-            id: 'numbers'
-          }, {
-            label: 'a',
-            id: 'a',
-            position: 'endof=letters'
-          }, {
-            type: 'separator',
-            id: 'letters'
-          }, {
-            label: '1',
             id: '1',
-            position: 'endof=numbers'
+            label: 'one'
           }, {
-            label: 'b',
-            id: 'b',
-            position: 'endof=letters'
-          }, {
-            label: '2',
-            id: '2',
-            position: 'endof=numbers'
-          }, {
-            label: 'c',
-            id: 'c',
-            position: 'endof=letters'
-          }, {
-            label: '3',
             id: '3',
-            position: 'endof=numbers'
+            label: 'three'
           }
         ])
 
-        assert.equal(menu.items[1].id, 'numbers')
+        assert.equal(menu.items[0].label, 'one')
-        assert.equal(menu.items[2].label, '1')
+        assert.equal(menu.items[1].label, 'two')
-        assert.equal(menu.items[3].label, '2')
+        assert.equal(menu.items[2].label, 'three')
-        assert.equal(menu.items[4].label, '3')
-        assert.equal(menu.items[5].id, 'letters')
-        assert.equal(menu.items[6].label, 'a')
-        assert.equal(menu.items[7].label, 'b')
-        assert.equal(menu.items[8].label, 'c')
       })
 
       it('should filter excess menu separators', () => {
@@ -168,69 +523,32 @@ describe('Menu module', () => {
         assert.equal(menuTwo.items[2].label, 'c')
       })
 
-      it('should create separator group if endof does not reference existing separator group', () => {
-        const menu = Menu.buildFromTemplate([
-          {
-            label: 'a',
-            id: 'a',
-            position: 'endof=letters'
-          }, {
-            label: '1',
-            id: '1',
-            position: 'endof=numbers'
-          }, {
-            label: 'b',
-            id: 'b',
-            position: 'endof=letters'
-          }, {
-            label: '2',
-            id: '2',
-            position: 'endof=numbers'
-          }, {
-            label: 'c',
-            id: 'c',
-            position: 'endof=letters'
-          }, {
-            label: '3',
-            id: '3',
-            position: 'endof=numbers'
-          }
-        ])
-        assert.equal(menu.items[0].id, 'letters')
-        assert.equal(menu.items[1].label, 'a')
-        assert.equal(menu.items[2].label, 'b')
-        assert.equal(menu.items[3].label, 'c')
-        assert.equal(menu.items[4].id, 'numbers')
-        assert.equal(menu.items[5].label, '1')
-        assert.equal(menu.items[6].label, '2')
-        assert.equal(menu.items[7].label, '3')
-      })
-
       it('should continue inserting items at next index when no specifier is present', () => {
         const menu = Menu.buildFromTemplate([
           {
-            label: '4',
+            id: '2',
-            id: '4'
+            label: 'two'
           }, {
-            label: '5',
+            id: '3',
-            id: '5'
+            label: 'three'
+          }, {
+            id: '4',
+            label: 'four'
+          }, {
+            id: '5',
+            label: 'five'
           }, {
-            label: '1',
             id: '1',
-            position: 'before=4'
+            label: 'one',
-          }, {
+            before: ['2']
-            label: '2',
-            id: '2'
-          }, {
-            label: '3',
-            id: '3'
           }
         ])
-        assert.equal(menu.items[0].label, '1')
+
-        assert.equal(menu.items[1].label, '2')
+        assert.equal(menu.items[0].label, 'one')
-        assert.equal(menu.items[2].label, '3')
+        assert.equal(menu.items[1].label, 'two')
-        assert.equal(menu.items[3].label, '4')
+        assert.equal(menu.items[2].label, 'three')
-        assert.equal(menu.items[4].label, '5')
+        assert.equal(menu.items[3].label, 'four')
+        assert.equal(menu.items[4].label, 'five')
       })
     })
   })
@@ -243,7 +561,7 @@ describe('Menu module', () => {
           submenu: [
             {
               label: 'Enter Fullscreen',
-              accelerator: 'Control+Command+F',
+              accelerator: 'ControlCommandF',
               id: 'fullScreen'
             }
           ]