Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve handling of Space key combinations #16645

Merged
merged 1 commit into from Feb 6, 2024

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Feb 1, 2024

Summary of the Pull Request

This fixes two issues where the Space key wasn't being handled
correctly:

  • Keyboards with an AltGr+Space mapping were not generating the
    expected character.
  • Pressing a dead key followed by Space is supposed to generate the
    accent character associated with that key, but it wasn't doing so.

References and Relevant Issues

These were both regressions from the keyboard refactor in PR #16511.

Detailed Description of the Pull Request / Additional comments

The problem was that we were treating VK_SPACE as a "functional" key,
which means it gets hardcoded VT mappings which take precedence over
whatever is in the keyboard layout. This was deemed necessary to deal
with the fact that many keyboards incorrectly map Ctrl+Space as a
SP character, when it's expected to be NUL.

I've now dropped VK_SPACE from the functional mapping table and allow
it be handled by the default mapping algorithm for "graphic" keys.
However, I've also introduced a special case check for Ctrl+Space
(and other modifier variants), so we can bypass any incorrect keyboard
layouts for those combinations.

Validation Steps Performed

I couldn't test with a French-BEPO keyboard layout directly, because the
MS Keyboard Layout Creator wouldn't accept a Space key mapping that
wasn't whitespace. However, if I remapped the AltGr+Space combo to
LF, I could confirm that we are now generating that correctly.

I've also tested the dead key Space combination on various keyboard
layouts and confirmed that that is now working correctly, and checked
that the Ctrl+Space combinations are still working too.

PR Checklist

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Input Related to input processing (key presses, mouse, etc.) Area-VT Virtual Terminal sequence support Product-Terminal The new Windows Terminal. labels Feb 1, 2024
@DHowett DHowett added this to To Cherry Pick in 1.20 Servicing Pipeline via automation Feb 1, 2024
@j4james j4james marked this pull request as ready for review February 1, 2024 19:46
@j4james
Copy link
Collaborator Author

j4james commented Feb 1, 2024

We really need some automated tests for international keyboard layouts, but I have no idea how we can do that.

@lhecker
Copy link
Member

lhecker commented Feb 1, 2024

We could load any arbitrary HKL with LoadKeyboardLayoutW and activate it on the current thread. The problem is just that it's currently hardcoded to always use the current foreground window. It may truly be worthwhile to set up the test code for this, since it would probably help with development a lot.

BTW I've been thinking about ToUnicodeEx again and I wonder if it's truly necessary to use it, especially since it relies on global keyboard state, while the given INPUT_RECORD may actually be synthetic and unrelated to any keyboards. We only transform characters via _makeCtrlChar when they're basic ASCII which makes me wonder if MapVirtualKeyW wouldn't still be sufficient since it circumvents that issue. ToUnicodeEx's handling of dead keys has me a little confused as well as I've assumed that dead keys should not trigger calls into TerminalInput in the first place (since they wouldn't be dead otherwise).

@j4james
Copy link
Collaborator Author

j4james commented Feb 1, 2024

We could load any arbitrary HKL with LoadKeyboardLayoutW and activate it on the current thread.

Would that not require that all the keyboard layouts are already installed on the test machine?

We only transform characters via _makeCtrlChar when they're basic ASCII which makes me wonder if MapVirtualKeyW wouldn't still be sufficient since it circumvents that issue. ``

Here's an example: On my UK keyboard, VK_OEM_3 is ', and the shifted character is ?. When I press Ctrl+Shift+OEM3, that needs to map to ^?. How would you accomplish that? MapVirtualKeyW can tell you that VK_OEM_3 is ', but it can't tell you the shifted mapping (as far as I know), and that's what is needed to determine the correct control character.

And that's not the only modifier that matters. Shift, Alt, and CapsLock can all potentially contribute to how the key is interpreted.

@lhecker
Copy link
Member

lhecker commented Feb 1, 2024

Would that not require that all the keyboard layouts are already installed on the test machine?

If the test machine is similar to a standard desktop Windows OS, then all standard layouts should already be installed. You can find the list at HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Keyboard Layouts and the files at C:\Windows\System32\kbd*. I don't know why Windows forces you to install language packs to use them and it's been a mystery for me since forever. It's definitely not because they don't work. For instance, try this and you'll get proper Persian input:

const auto hkl = LoadKeyboardLayoutW(L"00000429", KLF_ACTIVATE);
PostMessageW(GetConsoleWindow(), WM_INPUTLANGCHANGEREQUEST, 0, (LPARAM)hkl);

char buffer[1024];
fgets(&buffer[0], 1024, stdin);

UnloadKeyboardLayout(hkl);

FYI: If you fail to call UnloadKeyboardLayout (for instance because you pressed Ctrl+C, etc.), then the language will be stuck in the taskbar with no way to remove it. To fix this, just do a quick LoadKeyboardLayoutW + UnloadKeyboardLayout and it will be gone. (This was broken in Windows 8 and has remained like that since. sigh)

Here's an example: On my UK keyboard [...]

That's such a great example and a great test case too! I don't mind if you don't do this, but I think it would be really helpful for the future if you added this as a comment next to the ToUnicode code, even if it's just a verbatim copy of what you wrote. I feel like this, plus the reason why we need to get the HKL from the foreground window and not the current thread, is not immediately obvious, in particular if one is used to only using US keyboards.

@DHowett DHowett merged commit ec91be5 into microsoft:main Feb 6, 2024
13 of 15 checks passed
@j4james
Copy link
Collaborator Author

j4james commented Feb 6, 2024

@lhecker Sorry, I forgot to reply so you here. Thank you for the info about the keyboard layouts being preinstalled. That's very useful, and I'm quite keen to try and make some tests for the international layouts now. I can't promise I'll be able to do anything soon, but it's definitely on my TODO list.

@j4james j4james deleted the fix-space-key branch February 10, 2024 12:50
@DHowett DHowett moved this from To Cherry Pick to Cherry Picked in 1.20 Servicing Pipeline Feb 21, 2024
DHowett pushed a commit that referenced this pull request Feb 21, 2024
This fixes two issues where the `Space` key wasn't being handled
correctly:

* Keyboards with an `AltGr`+`Space` mapping were not generating the
  expected character.
* Pressing a dead key followed by `Space` is supposed to generate the
  accent character associated with that key, but it wasn't doing so.

## References and Relevant Issues

These were both regressions from the keyboard refactor in PR #16511.

## Detailed Description of the Pull Request / Additional comments

The problem was that we were treating `VK_SPACE` as a "functional" key,
which means it gets hardcoded VT mappings which take precedence over
whatever is in the keyboard layout. This was deemed necessary to deal
with the fact that many keyboards incorrectly map `Ctrl`+`Space` as a
`SP` character, when it's expected to be `NUL`.

I've now dropped `VK_SPACE` from the functional mapping table and allow
it be handled by the default mapping algorithm for "graphic" keys.
However, I've also introduced a special case check for `Ctrl`+`Space`
(and other modifier variants), so we can bypass any incorrect keyboard
layouts for those combinations.

## Validation Steps Performed

I couldn't test with a French-BEPO keyboard layout directly, because the
MS Keyboard Layout Creator wouldn't accept a `Space` key mapping that
wasn't whitespace. However, if I remapped the `AltGr`+`Space` combo to
`LF`, I could confirm that we are now generating that correctly.

I've also tested the dead key `Space` combination on various keyboard
layouts and confirmed that that is now working correctly, and checked
that the `Ctrl`+`Space` combinations are still working too.

Closes #16641
Closes #16642

(cherry picked from commit ec91be5)
Service-Card-Id: 91738880
Service-Version: 1.20
@DHowett DHowett moved this from Cherry Picked to Shipped in 1.20 Servicing Pipeline Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Projects
4 participants