rhysd / neovim-component

<neovim-editor> WebComponent to embed Neovim to your app with great ease
https://github.com/rhysd/NyaoVim
MIT License
193 stars 18 forks source link

KeyDown events trigger the wrong vim key #14

Closed romgrk closed 8 years ago

romgrk commented 8 years ago

The failing code is https://github.com/rhysd/neovim-component/blob/master/src/neovim/input.ts#L119

vim_input += (special_char || String.fromCharCode(event.keyCode).toLowerCase()) + '>';

Example: Pressing alt + ; generates <A-º> where neovim usually gets <A-;> screenshot from 2016-01-11 15-26-47

The input is off for all non-letter keys. I recall discussing something like this in https://github.com/rhysd/neovim-component/issues/6, and again I wouldnt know exactly how this could be solved. The problem here is that String.fromCharCode takes a character code as argument, and what is being fed to it is a keyCode. I had already run in this exact problem when implementing a package for Atom, the only workaround I found then was to have a map of keycodes: characters, but there was no check of keyboard layout whatsoever, US layout was assumed all the way. That causes evident problems for i18n. I'll check if there is any possibility to retrieve the user keyboard layout.

romgrk commented 8 years ago

This is interesting: https://github.com/Microsoft/node-native-keymap

rhysd commented 8 years ago

Thank you for your report. It must be a bug on catching user input.

Currently, <neovim-editor> catches user input by 2 ways.

input is used to catch normal character without modifiers. I use this because of IM (Input Method).

keydown is used for special characters and normal characters with modifiers. It requires to gain a character string from keyCode. It's difficult because key code is very complicated. I want to use code but latest Electron (Chromium 47) doesn't implement yet. (It seems that Chrome 48 beta implements it).

So hopefully KeyboardEvent.code will solve this character code related issue.

romgrk commented 8 years ago

Looks like it was merged 6 days ago :) https://github.com/atom/electron/pull/4014 https://github.com/atom/electron/commit/ae6c2d46b89a351110cd1b1bd73248b227adb6c1 Although I'm not sure if the features are enable by default. I'll run some tests.

romgrk commented 8 years ago

It works :D (electron 0.36.3)

rhysd commented 8 years ago

I updated key handling with KeyboardEvent.key. Finally I added a workaround on <A-x> input for OS X.

works-on-my-machine-starburst

romgrk commented 8 years ago

Doesn't seem to be working here (on linux). I'll run some tests first, but Alt seems to be never detected. Also on some cases "Unidentified" is inserted literally to neovim. (which is the value of KbEvent.key)

rhysd commented 8 years ago

Thank you for confirming. Hmm... Could you take a screenshot of console which shows the keydown event on the case?

It's weird that "Unidentified" is inserted because it should be ignored here. Anyway, I'll install Ubuntu 14.04 to VirtualBox on my machine and check it.

romgrk commented 8 years ago

Yes, indeed. (and I am well on commit 5c88083) Just a sec, I'll generate the events and post a screenshot. (meanwhile, I was wondering, do you have a ctags parsing tool for typescript?)

romgrk commented 8 years ago

Alt-o: screenshot from 2016-01-17 01-24-56

romgrk commented 8 years ago

Ctrl-semicolon: screenshot from 2016-01-17 01-26-36

romgrk commented 8 years ago

If of any utility, result of tests: screenshot from 2016-01-17 01-29-06

rhysd commented 8 years ago

Yes, some tests are not fixed yet, sorry. (I'll fix them today)

rhysd commented 8 years ago

(seeing screenshots)

Oh, ctrl key is considered in KeyboardEvent.key, but alt key is NOT considered in it... (When 'Alt+o' is input, the value will be "o".) I didn't know that. I'll fix it. Thank you.

romgrk commented 8 years ago

Wooo. Getting some really strange output. When pressing alt-;, the event.key == ";" When pressing ctrl-; the event.key == "Unidentified" Edit: seems related to https://code.google.com/p/chromium/issues/detail?id=527111&q=KeyboardEvent%20key&colspec=ID%20Pri%20M%20Stars%20ReleaseBlock%20Cr%20Status%20Owner%20Summary%20OS%20Modified

And btw I took a look at the code and what seems to be wrong is this: https://github.com/rhysd/neovim-component/blob/master/src/neovim/input.ts#L141

I don't know the code like you do, but from what I got it's just a matter of testing if altKey or ctrlKey are true. (And I do not know if it applies in some cases, but maybe metaKey).

rhysd commented 8 years ago

I installed Ubuntu 15.10 in virtual environment and I could reproduce what you had reported.

  1. We must input <A-{key}> instead of {key} when altKey is true.
  2. As you said, Ctrl+; is not the same behavior as others. The keycode was 186 and it's not for ;. In addition, Keyboard.key is "Unidentified" as you reported. I think it is good to make Ctrl+; fall back to ;. If we ignore Ctrl+; on keydown event, succeeding input event can catch it and handle input to <input> element. At least, "Unidentified" input to neovim is no good :sob:

I'll fix them.

rhysd commented 8 years ago

Both MacVim and gVim seem to handle <C-;> as ;. But browser can't know ; on <C-;> because key code in event is wrong in Linux. So currently <neovim-editor> ignores the sequence. I think it's acceptable.

romgrk commented 8 years ago

Yea, I have a local workaround but it would definitely not be clean enough to push. I guess it'll wait until chromium source fixes this. I'll be watching upstream.

rhysd commented 8 years ago

Thank you. I added 2 commits to fix them.

  1. Ignore Unidentified input on 'keydown' event
  2. Care alt key (currently e.g. Alt+a should work) also in Linux
rhysd commented 8 years ago

This fix is already added to NyaoVim(v0.0.11) and I've detected no issue. I close this because it seems to work.