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

Proposal for new module: shortcuts #20964

Open
poiru opened this issue Nov 5, 2019 · 6 comments
Open

Proposal for new module: shortcuts #20964

poiru opened this issue Nov 5, 2019 · 6 comments

Comments

@poiru
Copy link
Contributor

poiru commented Nov 5, 2019

Right now, we have the globalShortcuts module for registering shortcuts that apply to the entire system even when the Electron app is not focused.

I'd like to propose a new shortcuts module and deprecating the existing globalShortcuts module. The API would look like so:

const { shortcut } = require('electron');

let alreadyRegistered = shortcut.register('normal', 'CmdOrCtrl+L', () => {
  // Normal priority shortcut, handled after web content. Web content may preventDefault().
});

let alreadyRegistered = shortcut.register('high', 'CmdOrCtrl+L', () => {
  // High priority shortcut, handled before web content.
});

let alreadyRegistered = shortcut.register('global', 'CmdOrCtrl+L', () => {
  // Similar to existing globalShortcuts.register(), handled before web content.
});

let registed = shortcut.isRegistered('CmdOrCtrl+L');

shortcut.unregister('CmdOrCtrl+L');

There is clear demand for this feature, see #1334 and https://github.com/parro-it/electron-localshortcut

Implementing this within Electron would be much better because the accelerators would be parsed and handled like we already handle Menu accelerators. Properly processing the accelerators is tricky and error-prone - electron-localshortcut has several bugs and we have had multiple bugs in our own custom implementation as well.

I'm happy to implement this, but I wanted to hear your thoughts first!

cc @electron/wg-api

@Kilian
Copy link
Member

Kilian commented Nov 6, 2019

Would this also be a place to look at #19747 ?

In-webContent shortcuts (using keypress) work correctly since they use chromium's key handling, but they only work with the webContents focused and have no support for e.g. function keys like the current globalShortcuts do.

@JamesCoyle
Copy link

I feel a syntax such as the following would be nicer:

let shortcut = shortcut.register('CmdOrCtrl+L', () => {
  // Normal priority shortcut, handled after web content. Web content may preventDefault().
});

let shortcutIntercept = shortcut.register('CmdOrCtrl+L', () => {
  // High priority shortcut, handled before web content.
}, true);

let shortcutGlobal = shortcut.registerGlobal('CmdOrCtrl+L', () => {
  // Similar to existing globalShortcuts.register(), handled before web content.
});

@sindresorhus
Copy link
Contributor

I think this would be the most readable:

let shortcut = shortcut.registerLocal('CmdOrCtrl+L', () => {
  // Normal priority shortcut, handled after web content. Web content may preventDefault().
});

let shortcutIntercept = shortcut.registerLocal('CmdOrCtrl+L', () => {
  // High priority shortcut, handled before web content.
}, {
  highPriority: true
});

let shortcutGlobal = shortcut.registerGlobal('CmdOrCtrl+L', () => {
  // Similar to existing globalShortcuts.register(), handled before web content.
});

It's now clear what they do even when registerLocal and registerGlobal are alone what they do exactly. I would also recommend using an options object over a boolean trap.


Another idea would be to make the shortcut itself a type. This would make it nicer to compare shortcuts.

const fooShortcut = shortcut('CmdOrCtrl+L');

fooShortcut.registerLocal(() => {
  // Normal priority shortcut, handled after web content. Web content may preventDefault().
});

fooShortcut.registerLocal(() => {
  // High priority shortcut, handled before web content.
}, {
  highPriority: true
});

fooShortcut.registerGlobal(() => {
  // Similar to existing globalShortcuts.register(), handled before web content.
});

fooShortcut.isRegistered;

fooShortcut.unregister();

fooShortcut === shortcut('CmdOrCtrl+L');
//=> true

@fabiospampinato
Copy link
Contributor

fabiospampinato commented Jul 5, 2020

Maybe it would be more appropriate and web-friendly to use a third-party library for this, like this one: https://github.com/fabiospampinato/shortcuts or this other one: https://github.com/ccampbell/mousetrap

Like what feature could Electron offer that third-party libraries can't offer too? And if this is implementable completely via third-party libraries why going for an Electron-only version which would be painful to move from once/if the app will also ever run in the browser?

@mathieulj
Copy link

@fabiospampinato Maybe I misunderstood something here but aren't the libraries you mentioned only able to capture events when focus is within the web view? globalShortcuts is specifically for handling keyboard shortcuts regardless of if or not the application has focus and it is meant to work even with the application minimized.

@fabiospampinato
Copy link
Contributor

@mathieulj precisely, but globalShortcuts we have already, the proposal in this issue is to add support for capturing local shortcuts too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants