Skip to content
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

Display multi-keystroke key bindings in menu item's label #19423

Merged
merged 7 commits into from Jun 3, 2019

Conversation

Projects
None yet
2 participants
@as-cii
Copy link
Member

commented May 31, 2019

Closes #5343

This pull request introduces a workaround to a limitation in Electron's accelerator system, which refutes to display shortcuts for a menu item when it is a multi-keystroke key binding.

I performed a brief investigation of what it would take to fix this upstream in Electron, but unfortunately the shortcut code lives within libchromiumcontent and it would be a major undertaking to replace Chromium's system with a custom one.

Instead, with these changes, multi-keystroke keybindings will be displayed next to the item's label:

Screen Shot 2019-05-31 at 15 10 57

This is clearly a suboptimal choice in terms of how these shortcuts will be rendered, but there is at least one precedent for it in Visual Studio Code (see microsoft/vscode#30617). Considering that it does not impact the UX in any way (shortcuts will continue to work as they did before), I think this is an acceptable workaround that will help users find which keystrokes a menu item maps to.

@as-cii as-cii requested review from nathansobo and rafeca May 31, 2019

@as-cii as-cii self-assigned this May 31, 2019

@as-cii as-cii force-pushed the as/multi-stroke-accelerators branch from dbefbbc to b04695e May 31, 2019

@rafeca

rafeca approved these changes Jun 3, 2019

Copy link
Contributor

left a comment

LGTM!

The solution is not ideal, but it's a nice workaround.

I could not find any related issue on Electron about this, should we create one and link to it on the comment?

Show resolved Hide resolved src/context-menu-manager.coffee Outdated

@as-cii as-cii merged commit 6b9b4f9 into master Jun 3, 2019

0 of 2 checks passed

Atom Pull Requests in progress
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

@as-cii as-cii deleted the as/multi-stroke-accelerators branch Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.