Allow holding key on mac to send multiple keys to terminal

[epicfaace]

Fixes #265 - Holding down a key only sends a single key on Mac
Also see microsoft/vscode#65436

[Tyriar]

Seems to work, but all other editors I try to do this in does this:

Isn't that the behavior we're after? Do I need to flip a switch somewhere to disable this popup?

[epicfaace]

@Tyriar I'm not sure what you mean. We agree that this code changes the behavior from popup showing -> sending multiple keys, right? Are you seeing the popup when using xterm.js?

[epicfaace]

@Tyriar additionally, for me, the popup shows up only when using a text editor, but not when using the Terminal window. I would think that xterm.js should emulate the behavior of Terminal by not showing the popup.

[Tyriar]

the popup shows up only when using a text editor, but not when using the Terminal window

You're right, silly me didn't check actual terminals 😅

[Tyriar]

@mofux @jerch looks like this fixes it but it seems to make all the keypress code unnecessary. Do either of you know why keypress would be handled differently? It seems like it's been this way from the very beginning: https://github.com/chjj/term.js/blob/548af6e0a15c5978a2b62a882f7db03ec2ae2850/src/term.js#L2850

[mofux]

The difference between keypress and keydown is that keydown will fire for any key that is down - even if it's only the shift key. keypress will only fire if the pressed key results in a character (the shift key alone won't trigger a keypress event). I'm not sure if the hook to keypress was done intentionally to leverage this behaviour. Maybe we can merge this PR and create a follow-up issue that deals with it?

[jerch]

The key event handling dates back to the original impl from 2011. In those days it was a nightmare to get a somewhat reliable key handling across browsers and platforms. For many keys and combinations you would have to deal with both events due to a lack of specification (http://unixpapa.com/js/key.html).

Not sure which event should be used these days, a summary can be found here https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode.

[epicfaace]

I think we may need to use both keypress and keydown but in a different way -- as @mofux said, keypress only fires if the pressed key results in a character, so we can use that to detect if the key should result in a character (rather than the ev.keyCode >= 48 && ev.keyCode !== 144 && ev.keyCode !== 145 logic I have in this PR right now).

Additionally, instead of the logic that's there now, we should use https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key instead (and use a polyfill - https://github.com/cvan/keyboardevent-key-polyfill - if we need to support older browsers).

Refactoring the code to do those is probably a separate issue unrelated to this one though, so I think we should merge this and then work on that issue.

[epicfaace]

@Tyriar is this all set? Are there any changes you'd like to see before this PR can be merged?

[Tyriar]

I just tried and that popup I mentioned earlier does appear now (I recently upgrades to Mojave):

Even cancelling the event doesn't seem to stop this unfortunately 🙁

Also, I tried removing the keypress event on top of this change, but then space doesn't work.

Created #1862 for moving to key and code.

[Tyriar]

Let's merge this in, it doesn't seem likely that it'll cause issues after investigating a bit. Thanks @epicfaace 👍

[juancampa]

This causes an issue on Linux when the Meta key is pressed. Fix: #1885