Fix for dvorak-qwerty keyboard layout.

[donmccurdy]

...and any other layouts in which ctrl/alt/meta modify the keycode. Basically, event.which appears to be the more reliable identifier. Added a test case to catch any regression.

Gist:

• If event.which is valid ASCII, and the keyIdentifier is something mappable (hex values are, left/right/enter aren't), use event.which
• If the keyIdentifier was not mappable event.which was invalid but keyIdentifier was hex-ey, use the existing charCodeFromHexCharCode => keyFromCharCode mapping
• Otherwise, use the keyIdentifier as-is.

Other notes:

• in keydownEvent, I'm overriding the default event.which to undefined, which would otherwise be 0 due to a bug in Webkit. 0 is a valid keyCode, which is bad, and this seemed more reasonable than adding keycodes to all of your test cases. The override is skipped if the default event.which is not 0, on the off chance Webkit fixes this.
• I'm a little nervous about passing event.which through the keyFromCharCode function, but SO seems to think this is fine [1] [2] [3].

[nathansobo]

I feel like this line is a bit confusing, with the local variable named keyIsAscii and a function called isAscii. Is it necessarily the case that a keyIdentifier beginning with U+ is always an ASCII character?

[nathansobo]

Another idea for the variable: keyIdentifierIsHexCharCode. It's long but it's also explicit.

[donmccurdy]

Good point, thanks, i'll add that change. I was aiming to catch cases like keyIdentifier: 'Right', and in that case keyIdentifierIsHexCharCode is more accurate.

[nathansobo]

This is looking really good. Just tried it out locally and it's working for the Dvorak-Qwerty layout. One question remaining in my mind is whether we should be using which or keyCode, since which is apparently deprecated. Not sure how they differ or whether it really matters.

[donmccurdy]

Ah, that's what I get for using jQuery too much. (which is preferred there, since jQuery normalizes it). Was this a typo, about key?

Warning: This attribute is deprecated; you should use key instead, if available.

Going to assume that means keyCode.. In any case, keyCode seems to do equally well in handling the Dvorak-Qwerty layout, so no reason not to make the switch.

[nathansobo]

I assumed it was a typo as well. The entire Atom codebase is slowly recovering from an addiction to jQuery.

[donmccurdy]

Not sure what broke the build but I'm getting the same error on master, so I'm just going to leave this alone for now.

Some issue in the "Downloading latest Atom release" step apparently

[nathansobo]

Looks like a minor glitch in downloading Atom Shell. I restarted it and it's 🍏.

[izuzak]

🎉 This fixes atom/atom#1659, right?

[donmccurdy]

It does, yes! As well as: #12

[nathansobo]

Hi @donmccurdy. So unfortunately basing everything off the keyCode caused a lot of regressions on international keyboard layouts. I think we're going to need to go with a table-based approach.

To start with, it would be great to base behavior on a flag like atom.keymaps.useDvorakQwertyTranslation and just consult a lookup table if it's true. Then we can add some OS-level detection that auto enables the flag based on the user's current keymap. Or we need to dig into Chromium and fix it there, which sounds way harder. What do you think?

[donmccurdy]

It's too bad this wasn't as simple as it seemed, yes. Sorry for the trouble, but yeah reverting sounds reasonable if you're having to choose between the Dvorak niche and all the world's non-English speakers. :)

I'm nervous about trying to auto-detect the OS-specific settings – not sure how much OS X exposes, but generally I'd hope that layout is opaque to applications, and anyway that might break once Chromium has reliable event.key support.

Could this be done as a standalone plugin? That would at least let us keep the keyIdentifier -> keyIdentifier table out of your main key-map module. There might actually be some interest from Windows and Linux Dvorak users there, too, so no point in limiting it to OSX.

Chromium sounds like the harder option to me, but also more elegant (since none of this would be a problem if keyCode and keyIdentifier would just match.. I spent a little time trying to get Chromium to build, once; might be time to revisit that.

[nathansobo]

We could do it as a package, but where would you want to intercept events? Seems like having some sort of API for assigning a translation table could be a useful primitive in that situation.

[donmccurdy]

Not quite sure. I guess the question is what should be translated to what: Do we map keyIdentifiers to keyIdentifiers, or KeyboardEvents to KeyboardEvents?

Probably any intercept should be beneath KeymapManager's @keystrokeForKeyboardEvent in the call stack... I'm inclined to say that giving a helper function/table the chance to normalize the KeyboardEvent (i.e. change keyIdentifier if it doesn't match keyCode) might be least likely to complicate the logic in the keystrokeForKeyboardEvent helper function, and that wouldn't break if the atom-keymap module eventually switches to event.key or something.

[nathansobo]

That sounds reasonable, some kind of hook that can be added to normalize.

[davewhitley]

It looks like this issue has been closed. When will the fix be released?

[donmccurdy]

It was released in May, but then reverted – relying keyCode caused a number of issues on international keyboards. At the moment we don't have a good fix. Could probably reopen #12 or atom/atom#1659 I suppose.

[davewhitley]

That'd be great.

[cidevant]

Here you are! You are welcome!
Installation guide of DVORAK-QWERTY support in Atom.io

[donmccurdy]

Thanks @cidevant! could you post that on the original issue page (#12)? Should be helpful for anyone waiting for a patch to be merged.

Edit: never mind, looks like someone already reposted it. :)