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

implement keyboard shortcuts using KeyboardEvent.key api #3572

Merged
merged 1 commit into from
Jun 5, 2017

Conversation

tyrasd
Copy link
Member

@tyrasd tyrasd commented Nov 8, 2016

This is a try on improving compatibility across different keyboard layouts (e.g. used by different language groups): Use the new KeyboardEvent.key API (if available, but most browsers already have it, only Safari seems to be a bit behind on this – for which old-style keyCodes are used as a fallback).

I've tested it on Firefox (Linux) and Chrome and IE11 (both on Windows) and there it seems to work very well. But there's a strange bug in Chrome on Linux (see this ticket ticket (which will apparently be fixed in Chrome v56+) which renders the important undo/redo and zoom shortcuts Ctrl + z/Z and Ctrl + +/- unusable (well, one can still use them but they are behind different keys there: Ctrl + y/Y and Ctrl + ß/` ). Hmm…

this should improve compatibility across keyboard layouts (e.g. different languages). Old-style keycodes are still used for browsers that don't implement the new key property.
@tyrasd tyrasd force-pushed the tyrasd-patch-keybindings branch from a0051be to 5dcedd9 Compare November 8, 2016 15:52
@tyrasd
Copy link
Member Author

tyrasd commented Nov 14, 2016

Maybe we could also use https://github.com/marijnh/w3c-keyname for this which looks to have nice fallbacks for older browsers.

@@ -325,7 +325,7 @@ export function uiMapInMap(context) {
redraw();

var keybinding = d3keybinding('map-in-map')
.on(key, toggle);
.on([key, '⇧'+key], toggle);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not bind Shift-/ here, because on US keyboards that's ?, which we're saving for #1481

@bhousel
Copy link
Member

bhousel commented Jan 16, 2017

Hey @tyrasd just checking in with this.. The approach looks good, as long as it's supported in all the main browsers. While we wait for Chrome 56, is there anything else that we should do in the meantime?

@bhousel
Copy link
Member

bhousel commented Jun 5, 2017

I tested this today in Safari 10.1 on Mac and Chrome 58 on Windows, and both seem fine. I'm going to merge it 👍

@bhousel bhousel merged commit 32995e5 into master Jun 5, 2017
@bhousel bhousel deleted the tyrasd-patch-keybindings branch June 5, 2017 20:27
@bhousel
Copy link
Member

bhousel commented Jun 6, 2017

Thanks again for this @tyrasd!

I added a few commits after, to make matching with shiftKey less strict.

634002b Remove shifts from the keybinds that don't need them
94c705e Don't match shiftKey strictly, but prioritize shifted over unshifted

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

Successfully merging this pull request may close these issues.

2 participants