[Keybinding] Normalize key sequences to US layout

[svenefftinge]

Some bindings, i.e. 'Toggle Line Comment' or 'Open Terminal' are
bound to a sequence that are not directly reachable on many
non-US keyboard layouts. For instance 'ctrlcmd+/' to toggle line
comments doesn't work on german kb layouts, because the slash
is above the 7 and therefore the shortcut is seen as cmdctrl+shift+7

This change translates the given chord to
a normalized US keysequence if the character is one of the US layout
keys. I.e. for cmdctrl+shift+7 we also know the typed character is /
so if there is no direct keybinding we test against a US-keyboard
layouted version.

Fixes #1244

Signed-off-by: Sven Efftinge [email protected]

[svenefftinge]

fixes #2200

[svenefftinge]

The underlying issue is that we have no way to find out what the keyboard layout of a user looks like, when running in the browser. VS Code does this through a native node library, but obviously this is not available in the case of browser-only. See microsoft/monaco-editor#1233

[spoenemann]

This is a pragmatic solution for cases where the shift key is involved, but it does not work when additionally the alt key is involved. For example, if a keybinding is defined as alt+/, I would have to press alt+shift+7 to get that on my keyboard. The resulting character is \, so the normalizeToUsLayout() function would translate that to alt+\, which is wrong.

I propose to add the guard !this.alt in normalizeToUsLayout() so at least it works for the shift-only case.

[spoenemann]

Ah, you updated the code in the meantime. The problem is, if the user presses cmd+alt+shift+7 on a German Mac keyboard, we cannot know whether he meant cmd+alt+/ or cmd+\ on a US keyboard layout.

[svenefftinge]

Yes, I changed it now so that both 'shift+alt' are removed and also that at least one other modifier has to be pressed.

[spoenemann]

But I'm fine with keeping this solution until we run into actual problems. The only alternative I can think of at the moment is to change the problematic keybindings.

One additional thing we could do is to hard-code a list of problematic keybindings such as alt+/ or alt+\ and log a warning (or even an error?) when a command is registered with such a binding.

[svenefftinge]

I think that would be an unrelated check, because even without this change those bindings are problematic as they are used to type a character.

[spoenemann]

Ok, but cmd+alt+/ and cmd+alt+\ are problematic, too.

[spoenemann]

I'll create a separate issue for that.

[spoenemann]

Looks good to me. However,

• I can toggle line comments with ctrl+shift+7 due to [osx][keybinding][monaco] Modifier keys are incorrectly mapped on macOS #3834,
• I cannot toggle line comments with cmd+shift+7, but the search box in the Help menu is activated instead.

[svenefftinge]

I can toggle line comments with ctrl+shift+7 due to #3834,
I cannot toggle line comments with cmd+shift+7, but the search box in the Help menu is activated instead.

thanks, both issues are addressed through #3835.

[svenefftinge]

@kittaakos do you know if the appveyor failure is possibly related to my changes? I cannot tell from the output, unfortunately.

[kittaakos]

I cannot tell from the output, unfortunately.

I have seen the same error before.

Extracted from the AppVeyor log and formatted a bit:

An unknown server-side error occurred while processing the command.
    at elements(".theia-preload") - C:\\projects\\theia\node_modules\\webdriverio\\build\\lib\\commands\\isExisting.js:46:17
    at isExisting(".theia-preload") -     at C:\\projects\\theia\node_modules\\wdio-sync\\build\\index.js:293:3

root ERROR Uncaught Exception:  TypeError: Cannot set property 'state' of undefined
root ERROR TypeError: Cannot set property 'state' of undefined
    at ReporterStats.testFail (C:\\projects\\theia\node_modules\\webdriverio\\build\\lib\\utils\\ReporterStats.js:459:29)
    at BaseReporter.<anonymous> (C:\\projects\\theia\node_modules\\webdriverio\\build\\lib\\utils\\BaseReporter.js:159:25)
    at emitOne (events.js:116:13)
    at BaseReporter.emit (events.js:211:7)
    at BaseReporter.handleEvent