Don’t handle or replay keystrokes during IME composition

[nathansobo]

Closes #172
Closes #177

In #144, we completely overhauled translation of keyboard events to keystrokes, inadvertently fixing a bug in keystroke resolution that was causing all keystrokes to be recognized as å when an IME dialog was visible. Unfortunately, this previous bug was actually a feature in that it prevented any key bindings from matching during an IME editing session. By enabling correct keystroke resolution during IME editing, #144 actually caused interference with the IME user interface... the arrow keys not only change the IME selection, they also move the cursor... enter inserts a new line instead of selecting a character... etc.

In addition, replaying input events for previous partial match prefixes yields incorrect behavior when the IME dialog is visible, because in those cases we really want previous keystrokes to combine with new keystrokes to build up a composed character. For now, if we detect that the IME interface is visible, we suppress replay entirely.

Getting IME and multi-stroke bindings to interact perfectly is a really nuanced problem. For example, it's possible that some of the keystrokes need to be replayed but not all of them. Also, what if a partially matched multi-stroke binding prevented the IME interface from popping up to begin with. Since our replay mechanism doesn't work at the browser level (maybe it should?) we won't open the IME dialog in those cases.

Because compositionstart events do not occur until after keydown events, I've needed to defer terminating the pending state until the keyup event following the keydown where we previously terminated. This ensures that the compositionstart event has been received so we can suppress replay if a composition is in progress. @iolsen this seems like it will work, but you've had your head wrapped around this system recently as well. Do you foresee any issues I'm missing?

For now, I think we should draw the line here. IME users will just need to be careful about conflicts with multi-stroke unmodified bindings. This is a pretty extreme edge case, and going further is going to involve a complete rethink of our approach to replay that we don't have time for right now.

[iolsen]

I think this is a sane, nicely pragmatic approach. I do have a couple questions.

[iolsen]

Where do these actually get called? The only call sites I see are in specs.

[nathansobo]

They'll have to get called in Atom with some new event handlers.

[iolsen]

What's the reasoning behind this change? I did some testing with my recent changes and it seems to work fine but I couldn't figure out why you added this.

[nathansobo]

Because compositionstart happens after keydown, we need to wait until keyup to know whether or not replay should be suppressed.

[iolsen]

Oh, duh. Connecting that now with what you said in the PR description. Cool.

[ungb]

@nathansobo I was able to verify both the IME issues look like they are working. #172 for the japanese layout, using atom stable with vim-mode-plus, I was getting extra characters when typing, but works fine with your branch.

For the other issue #177 with Chinese Pinyin Layout, it looks like it worked fine in stable, and works with the new build too… did this break in beta?

[winstliu]

@ungb I was testing on the Electron 1.4.x branch. So it could be something with the new Electron version or just my computer.

[ungb]

I tested the change on atom/atom#13233. I just built his branch and installed it on windows.

[t9md]

I also want to check with my mac.
Question, is the cached-build for macOS available via 3rd party CI test result? I couldn't find for mac on Travis, Circle CI.
The windows and linux build was found on appveyer result.
Do I have to build on my side?

[nathansobo]

@t9md You should be able to pull a build from Circle CI. Here is the link: https://1886-3228505-gh.circle-artifacts.com/0/Users/distiller/atom/out/atom-mac.zip

[t9md]

@nathansobo Tried the above link, and result was NOT good.
The behavior was changed, but not got better.

• 1.13-beta: first j is handled by keymap system the output of j i is jい.
• the-build: first j just seem to be ignored, the out oput of j i is い only. To input じ, keystroke j i is required, but only i seems to be treated so, what I got is い char(same as when I type i solely).

[t9md]

Just in case you want to reproduce it by your side, I think it just installing this google IME and stat IME by cmd-space(not sure I set it by myself or not).

I use Mac(EL capitan) with US keyboard with english language environment, so I believe similar environment with you.

[t9md]

Observed event by this gist.
Of course google IME is enabled, and keystroke is j i enter to input じ and confirm by enter.

version

Downloads/build% ./Atom.app/Contents/MacOS/Atom --version
Atom    : 1.14.0-dev-3f788eb
Electron: 1.3.6
Chrome  : 52.0.2743.82
Node    : 6.3.0

1.13.0-beta3 [OK case] without j k keymap

keyup event is fired on each j and k stroke.

1.14.0-dev-3f788eb [OK case] without j k keymap

Strange part is keyup event for first keystroke j is delayed, but still j fire compositionstart event.

1.14.0-dev-3f788eb [NG case] with j k keymap(entering pending state case)

What the problem is first j keystroke don't fire compositionstart event.
So @compositionInProgress not be updated on this timing.

[ungb]

I tested on Windows installing the google IME with version: 1.14.0-dev-a54c4fb from: https://ci.appveyor.com/project/Atom/atom/build/4137/job/cw2tn6wegh563wuh/artifacts

Seeing similar issues as you @t9md. Before I was using windows ime, but I'm seeing issues on there now too. I'll follow up with Nathan on his PR: atom/atom#13233

[nathansobo]

@t9md I think I understand what's happening. Even though we aren't replaying keystrokes when the IME is enabled, we still have a problem because we're calling preventDefault on the j keystroke so the IME never sees it. I think we'll need to find a way to completely disable single character keystrokes for layouts that employ an IME interface.

[t9md]

If the original event was compositionstart, then bypass the keymap system, clear the pending keystroke, I'm not sure things are such simple.

If you want me do further check, I can do that.

[nathansobo]

Looks like we can use the old API and look for a KeyboardEvent.keyCode value equal to 227 and just not handle keystrokes with that value. Every time a keydown event is involved in composition this ends up being the keyCode that's reported. The keyCode API is deprecated but it's been around since the dawn of the web and probably won't be removed or changed any time soon, so I think it could be safe.

[t9md]

That's good, it can work.

But how about your original intention, let the Atom set keymap.compositionInProgress = true to translate compositionstart to keymapManager's state.

If the event was compositionstart then let atom update atom.keymap.compositionInProgress to true is not work? Or atom keymap call preventDefault regardless of event type and no chance to differentiate behavior based on event type?

[t9md]

@nathansobo

Checked with build in branch 1.13-releases associated with this commit, atom/atom@b77f18c.

And result was GOOD, OK, FIXED.
Thanks for very quick investigation and fix!! I appreciate your work as always, thanks a lot!!

• compsitionstart event is fired by first j keystroke.
• I can input じ by j i, even if when I set j k custom keymap.
• When IME was enabled, j k no longer dispatched to command via keymap system, or down, up arrow key no longer move cursor while selecting candidate in IME popup window.

$ ./Atom\ Beta.app/Contents/MacOS/Atom\ Beta --version
Atom    : 1.13.0-beta4
Electron: 1.3.6
Chrome  : 52.0.2743.82
Node    : 6.3.0