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

Replace event.which with event.key in shortcut handlers #724

Closed
wants to merge 5 commits into from

Conversation

hpohlmeyer
Copy link

@hpohlmeyer hpohlmeyer commented Nov 23, 2017

Since event.which has some issues with non-english keyboard layouts and is deprected, I replaced all instances of event.which with event.key.

Fixes #723 and some other broken shortcuts. For example I could not use the ? shortcut to get to the help menu, which is possible now.

I have tested it with a german and an english keyboard layout.

Fixes issues with foreign keyboard layouts
@hpohlmeyer hpohlmeyer changed the title Update handleKeydownSuperEvent to use event.key Replace event.which with event.key in shortcut handlers Nov 23, 2017
@Thibaut
Copy link
Member

Thibaut commented Dec 3, 2017

Hey - thanks a lot for this PR.

Unfortunately browser support for event.key is not at 100% just yet. The Android browser doesn't support it, which could be annoying for tablet users, and all versions of Microsoft Edge have major bugs (but seems a fix is on the way).

Not sure if we should just roll with this / ignore browser support, or switch to an implementation that checks both event.key and event.which.

@hpohlmeyer
Copy link
Author

I thought about checking both to support older browsers, but I was not aware of the flawed Edge support. As you said, there seems to be a fix on its way, but the tablet issue would still be a problem.

I think we really have to check both – event.key and event.which, but I am not sure if we should work around the flawed implementation of Edge and IE11 or just wait for the fix in Edge and ditch IE11 support altogether.

@j-f1
Copy link
Contributor

j-f1 commented Dec 4, 2017

A cursory search turned up cvan/keyboardevent-key-polyfill, which appears to polyfill event.key, giving better support.

@Thibaut
Copy link
Member

Thibaut commented Jan 31, 2018

Sorry for the delay on this. I was ready to merge and ignore the Android / Edge compatibility issues (given their low impact), but after testing I found that the alt shortcuts don't work on Firefox / macOS (only browser I tested, so probably others too). The problem is that for, e.g., an alt+o event, event.key ends up being ø (what typing alt+o outputs) instead of o. Seems we could maybe use event.code instead, but I'm unsure about browser support.

@hpohlmeyer
Copy link
Author

Oh, I see …
event.code brings back problems with translated keys. Let’s say we have z registered as a shortcut with the code KeyZ. If you use a QWERTZ-keyboard you have to press y to hit the right button. So this will bring back the problems from #723.

The best way to somehow get the original key value without modification. But I don’t know if that is possible at all.

@jmerle
Copy link
Contributor

jmerle commented Jun 11, 2019

(extremely late response, I'm aware)

I don't really know what to do with this. As far as I can see it's basically like this:

  • The current implementation uses deprecated properties and causes problems in certain situations.
  • The implementation proposed here does not use deprecated properties but still causes problems in certain situations.

The current implementation seems to work correctly on English keyboards on all browsers (even though there are still issues like #946), so I'd vote to close this PR until we come up with a better solution. I think it's highly unlikely that KeyboardEvent.which will be removed in the near future, so that wouldn't be a problem.

@j-f1 what do you think?

@hpohlmeyer
Copy link
Author

Oh yeah this is an old one and not much has changed since my last post.
As far as I can see, there still is no way to get the unmodified character from a key while taking the keyboard layout into account.

Even if we could, detecting keypresses like ? would be really hard to get right. On my german QWERTZ layout for example, you need to press Shift + ß to get the question mark. Without the modifier, we would just receive ß here, which would not help at all.

It gets even more confusing when you try to assign a key combination like Cmd + ?. In order to get that you would need to press Cmd + Shift + ß. The Keyboard event that is triggered has a key property of / and a code of Minus. Again: No way to find out which character we tried to press.

Long story short, I think the current keyboard events are really bad to do shortcuts that depend on specific characters instead of specific key locations. Until we have a better choice, I think closing the issue is the right thing to do. :/

@jmerle
Copy link
Contributor

jmerle commented Jun 11, 2019

It's indeed confusing. Maybe in the future we can implement custom keyboard shortcuts with a library like Mousetrap which has the ability to record keyboard sequences, but that's quite a big change and out of scope for this PR.

I'll close this PR for now. Feel free to open a new one if you (or anyone else) has a better solution for this issue.

@jmerle jmerle closed this Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Safari] CMD+< shortcut opens settings page
4 participants