Base context menu accelerators on activeElement

Addresses issue pointed by out @nathansobo in #15277 where keybindings
for unfocusable nodes were being surfaced as accelerator indicators in
context menus.

When you right click in the DOM, your focus goes to the first focusable
ancestor of your click target. This change uses the ancestor that you
are actually focused on when looking for avaliable key bindings rather
than using the event target directly. This ensures that any surfaced key
bindings are actually reachable.
This commit is contained in:
Jordan Eldredge
2017-08-15 19:40:43 -07:00
parent e71d8d863c
commit 228f65da5f
2 changed files with 37 additions and 6 deletions

View File

@@ -11,12 +11,22 @@ describe "ContextMenuManager", ->
parent = document.createElement("div")
child = document.createElement("div")
grandchild = document.createElement("div")
parent.tabIndex = -1
child.tabIndex = -1
grandchild.tabIndex = -1
parent.classList.add('parent')
child.classList.add('child')
grandchild.classList.add('grandchild')
child.appendChild(grandchild)
parent.appendChild(child)
document.body.appendChild(parent)
afterEach ->
document.body.blur()
document.body.removeChild(parent)
describe "::add(itemsBySelector)", ->
it "can add top-level menu items that can be removed with the returned disposable", ->
disposable = contextMenu.add
@@ -259,6 +269,7 @@ describe "ContextMenuManager", ->
it "adds Electron-style accelerators to items that have keybindings", ->
child.focus()
dispatchedEvent = {target: child}
expect(contextMenu.templateForEvent(dispatchedEvent)).toEqual(
[
@@ -275,6 +286,7 @@ describe "ContextMenuManager", ->
])
it "adds accelerators when a parent node has key bindings for a given command", ->
grandchild.focus()
dispatchedEvent = {target: grandchild}
expect(contextMenu.templateForEvent(dispatchedEvent)).toEqual(
[
@@ -291,6 +303,7 @@ describe "ContextMenuManager", ->
])
it "does not add accelerators when a child node has key bindings for a given command", ->
parent.focus()
dispatchedEvent = {target: parent}
expect(contextMenu.templateForEvent(dispatchedEvent)).toEqual(
[
@@ -303,3 +316,20 @@ describe "ContextMenuManager", ->
}
]
])
it "adds accelerators based on focus, not context menu target", ->
grandchild.focus()
dispatchedEvent = {target: parent}
expect(contextMenu.templateForEvent(dispatchedEvent)).toEqual(
[
label: 'My Command',
command: 'test:my-command',
accelerator: 'Ctrl+A',
submenu: [
{
label: 'My Other Command',
command: 'test:my-other-command',
accelerator: 'Shift+B',
}
]
])

View File

@@ -147,19 +147,20 @@ class ContextMenuManager
currentTarget = currentTarget.parentElement
@pruneRedundantSeparators(template)
@addAccelerators(template, event.target)
@addAccelerators(template)
template
# Adds an `accelerator` property to items that have key bindings. Electron
# uses this property to surface the relevant keymaps in the context menu.
addAccelerators: (template, target) ->
addAccelerators: (template) ->
for id, item of template
keymaps = @keymapManager.findKeyBindings({command: item.command, target})
accelerator = MenuHelpers.acceleratorForKeystroke(keymaps?[0]?.keystrokes)
item.accelerator = accelerator if accelerator
if item.command
keymaps = @keymapManager.findKeyBindings({command: item.command, target: document.activeElement})
accelerator = MenuHelpers.acceleratorForKeystroke(keymaps?[0]?.keystrokes)
item.accelerator = accelerator if accelerator
if Array.isArray(item.submenu)
@addAccelerators(item.submenu, target)
@addAccelerators(item.submenu)
pruneRedundantSeparators: (menu) ->
keepNextItemIfSeparator = false