New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add accelerator indicators to context menus #15277

Merged
merged 1 commit into from Aug 15, 2017

Conversation

Projects
None yet
3 participants
@captbaritone
Contributor

captbaritone commented Aug 12, 2017

Electron allows us to pass an "accelerator" property for each menu item, which
is renders to the right of the menu item. We were already adding these for the
application level menus.

This pull request adds the accelerator property to regular context menu items,
which should make it easier for people to discover/recall key mappings for
actions which they usually take via a context menu.

Before

screen shot 2017-08-12 at 2 56 58 pm

After

screen shot 2017-08-12 at 2 56 36 pm

Add accelerator indicators to context menus
Electron allows us to pass an "accelerator" property for each menu item, which
is renders to the right of the menu item. We were already adding these for the
application level menus.

This pull request adds the accelerator property to regular context menu items,
which should make it easier for people to discover/recall key mappings for
actions which they usually take via a context menu.

@50Wliu 50Wliu added the needs-review label Aug 12, 2017

@nathansobo nathansobo merged commit 8e6497b into atom:master Aug 15, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Aug 15, 2017

Contributor

Thanks for this contribution. This will be a nice improvement.

Contributor

nathansobo commented Aug 15, 2017

Thanks for this contribution. This will be a nice improvement.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Aug 15, 2017

Contributor

@captbaritone I think I've found a bug or unexpected behavior.

If you right click an unstaged file in the Git panel, the "Discard" option has an accelerator of ctrl-backspace. This really should be bound, but as far as I can tell it is only bound in the diff view itself. Could you investigate this?

Contributor

nathansobo commented Aug 15, 2017

@captbaritone I think I've found a bug or unexpected behavior.

If you right click an unstaged file in the Git panel, the "Discard" option has an accelerator of ctrl-backspace. This really should be bound, but as far as I can tell it is only bound in the diff view itself. Could you investigate this?

@captbaritone

This comment has been minimized.

Show comment
Hide comment
@captbaritone

captbaritone Aug 15, 2017

Contributor

@nathansobo Sure. I'll take a look.

Contributor

captbaritone commented Aug 15, 2017

@nathansobo Sure. I'll take a look.

@captbaritone

This comment has been minimized.

Show comment
Hide comment
@captbaritone

captbaritone Aug 15, 2017

Contributor

@nathansobo From what I can tell, this is because the key bindings are attached to a node that either can't actually get keyboard focus, or only can under very specific circumstances. By adding tabIndex='-1' to the .github-FilePatchListView node, I was able to focus the node by clicking within it, and then trigger the keyboard shortcut.

The behavior you are seeing in the diff view is actually a separate binding of the same set of keys to a different selector and different (but similar) command: https://github.com/atom/github/blob/master/keymaps/git.cson#L45-L46

Contributor

captbaritone commented Aug 15, 2017

@nathansobo From what I can tell, this is because the key bindings are attached to a node that either can't actually get keyboard focus, or only can under very specific circumstances. By adding tabIndex='-1' to the .github-FilePatchListView node, I was able to focus the node by clicking within it, and then trigger the keyboard shortcut.

The behavior you are seeing in the diff view is actually a separate binding of the same set of keys to a different selector and different (but similar) command: https://github.com/atom/github/blob/master/keymaps/git.cson#L45-L46

@captbaritone

This comment has been minimized.

Show comment
Hide comment
@captbaritone

captbaritone Aug 15, 2017

Contributor

@nathansobo So, I think the real bug here is with the keybinding provided by the github package. That said, I wonder if there is something better we can do inside this accelerator logic to avoid surfacing similarly broken logic. For example, rather than using event.target when calling findKeyBinding, we could instead use document.activeElement. I'm no expert in how focus is handled, but a manual test seems to indicate that:

  1. document.activeElement is updated on right-click, before this code is called.
  2. Making this change correctly filters out this particular broken key binding.
  3. This change does not seem to break other accelerators in any obvious way.
Contributor

captbaritone commented Aug 15, 2017

@nathansobo So, I think the real bug here is with the keybinding provided by the github package. That said, I wonder if there is something better we can do inside this accelerator logic to avoid surfacing similarly broken logic. For example, rather than using event.target when calling findKeyBinding, we could instead use document.activeElement. I'm no expert in how focus is handled, but a manual test seems to indicate that:

  1. document.activeElement is updated on right-click, before this code is called.
  2. Making this change correctly filters out this particular broken key binding.
  3. This change does not seem to break other accelerators in any obvious way.

captbaritone added a commit to captbaritone/atom that referenced this pull request Aug 16, 2017

captbaritone added a commit to captbaritone/atom that referenced this pull request Aug 16, 2017

Base context menu accelerators on activeElement
Addresses issue pointed by out @nathansobo in atom#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.

captbaritone added a commit to captbaritone/atom that referenced this pull request Aug 16, 2017

Base context menu accelerators on activeElement
Addresses issue pointed by out @nathansobo in atom#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment