diff --git a/lib/browser/api/menu-utils.ts b/lib/browser/api/menu-utils.ts index 2bd83ace383b..084432a66f4f 100644 --- a/lib/browser/api/menu-utils.ts +++ b/lib/browser/api/menu-utils.ts @@ -157,9 +157,12 @@ function sortGroups (groups: {id?: T}[][]) { return sortedGroupIndexes.map(i => groups[i]); } -export function sortMenuItems (menuItems: {type?: string, id?: string}[]) { - const isSeparator = (item: {type?: string}) => item.type === 'separator'; - const separators = menuItems.filter(i => i.type === 'separator'); +export function sortMenuItems (menuItems: (Electron.MenuItemConstructorOptions | Electron.MenuItem)[]) { + const isSeparator = (i: Electron.MenuItemConstructorOptions | Electron.MenuItem) => { + const opts = i as Electron.MenuItemConstructorOptions; + return i.type === 'separator' && !opts.before && !opts.after && !opts.beforeGroupContaining && !opts.afterGroupContaining; + }; + const separators = menuItems.filter(isSeparator); // Split the items into their implicit groups based upon separators. const groups = splitArray(menuItems, isSeparator); diff --git a/lib/browser/api/menu.ts b/lib/browser/api/menu.ts index 725396fb82c2..fe0fb9d5aad1 100644 --- a/lib/browser/api/menu.ts +++ b/lib/browser/api/menu.ts @@ -182,11 +182,11 @@ Menu.buildFromTemplate = function (template) { throw new TypeError('Invalid template for MenuItem: must have at least one of label, role or type'); } - const filtered = removeExtraSeparators(template); - const sorted = sortTemplate(filtered); + const sorted = sortTemplate(template); + const filtered = removeExtraSeparators(sorted); const menu = new Menu(); - sorted.forEach(item => { + filtered.forEach(item => { if (item instanceof MenuItem) { menu.append(item); } else { diff --git a/spec-main/api-menu-spec.ts b/spec-main/api-menu-spec.ts index cbce05964ae1..681085f49a37 100644 --- a/spec-main/api-menu-spec.ts +++ b/spec-main/api-menu-spec.ts @@ -101,7 +101,7 @@ describe('Menu module', function () { describe('Menu sorting and building', () => { describe('sorts groups', () => { it('does a simple sort', () => { - const items = [ + const items: Electron.MenuItemConstructorOptions[] = [ { label: 'two', id: '2', @@ -146,7 +146,7 @@ describe('Menu module', function () { }); it('resolves cycles by ignoring things that conflict', () => { - const items = [ + const items: Electron.MenuItemConstructorOptions[] = [ { id: '2', label: 'two', @@ -178,7 +178,7 @@ describe('Menu module', function () { }); it('ignores references to commands that do not exist', () => { - const items = [ + const items: Electron.MenuItemConstructorOptions[] = [ { id: '1', label: 'one' @@ -208,7 +208,7 @@ describe('Menu module', function () { }); it('only respects the first matching [before|after]GroupContaining rule in a given group', () => { - const items = [ + const items: Electron.MenuItemConstructorOptions[] = [ { id: '1', label: 'one' @@ -260,7 +260,7 @@ describe('Menu module', function () { describe('moves an item to a different group by merging groups', () => { it('can move a group of one item', () => { - const items = [ + const items: Electron.MenuItemConstructorOptions[] = [ { id: '1', label: 'one' @@ -300,7 +300,7 @@ describe('Menu module', function () { }); it("moves all items in the moving item's group", () => { - const items = [ + const items: Electron.MenuItemConstructorOptions[] = [ { id: '1', label: 'one' @@ -348,7 +348,7 @@ describe('Menu module', function () { }); it("ignores positions relative to commands that don't exist", () => { - const items = [ + const items: Electron.MenuItemConstructorOptions[] = [ { id: '1', label: 'one' @@ -436,7 +436,7 @@ describe('Menu module', function () { }); it('can merge multiple groups when given a list of before/after commands', () => { - const items = [ + const items: Electron.MenuItemConstructorOptions[] = [ { id: '1', label: 'one' @@ -474,7 +474,7 @@ describe('Menu module', function () { }); it('can merge multiple groups based on both before/after commands', () => { - const items = [ + const items: Electron.MenuItemConstructorOptions[] = [ { id: '1', label: 'one' @@ -599,6 +599,32 @@ describe('Menu module', function () { expect(menuTwo.items[2].label).to.equal('c'); }); + it('should only filter excess menu separators AFTER the re-ordering for before/after is done', () => { + const menuOne = Menu.buildFromTemplate([ + { + type: 'separator' + }, + { + type: 'normal', + label: 'Foo', + id: 'foo' + }, + { + type: 'normal', + label: 'Bar', + id: 'bar' + }, + { + type: 'separator', + before: ['bar'] + }]); + + expect(menuOne.items).to.have.length(3); + expect(menuOne.items[0].label).to.equal('Foo'); + expect(menuOne.items[1].type).to.equal('separator'); + expect(menuOne.items[2].label).to.equal('Bar'); + }); + it('should continue inserting items at next index when no specifier is present', () => { const menu = Menu.buildFromTemplate([ {