Fix various KKP encoding issues by lhecker · Pull Request #20052 · microsoft/terminal

lhecker · 2026-04-02T11:02:25Z

NOTE:
This commit changes our input model to treat all
Ctrl-Alt combinations as an AltGr modifier by default.
Ctrl+Alt+/ will not produce "\x1b\x1f" anymore.

This fixes the following issues:

Base-key report of non-ASCII keys was broken
AltGr on international keyboards would spam
Ctrl/Alt up/down events repeatedly forever
KKP doesn't report modifier key repeats
AltGr+Key combinations were reported as Ctrl+Alt+key
but should've been just plain Key-But-Modified

Closes #19977

Validation Steps Performed

This cannot be easily tested in unit tests, due to
the dependence on international keyboard layouts.
It appears to work correctly from what I can tell.

src/terminal/input/terminalInput.cpp

src/terminal/adapter/ut_adapter/kittyKeyboardProtocol.cpp

lhecker · 2026-04-02T11:06:54Z

src/terminal/input/terminalInput.cpp

    }

    // Keep track of key repeats.
-    key.keyRepeat = _lastVirtualKeyCode == key.virtualKey;


Placing the detection outside a if (key.keyDown) block meant that a single key up/down would already be detected as a key repeat.

lhecker · 2026-04-02T11:07:09Z

src/terminal/input/terminalInput.cpp

+        if (const auto flags = _controlKeyStateFromVirtualKey(key.virtualKey, key.controlKeyState))
+        {
+            key.keyRepeat = (_previousControlKeyState & flags) != 0;
+        }


These 3 lines fix the AltGr key repeat.

lhecker · 2026-04-02T11:08:12Z

src/terminal/input/terminalInput.cpp

+            // Suppress modifier key events at all times - they aren't reported in any protocol.
+            (key.virtualKey >= VK_SHIFT && key.virtualKey <= VK_MENU) ||
+            (key.virtualKey >= VK_LSHIFT && key.virtualKey <= VK_RMENU) ||


KKP doesn't report key repeats for modifier keys.

lhecker · 2026-04-02T11:08:34Z

src/terminal/input/terminalInput.cpp

+            (key.virtualKey >= VK_LSHIFT && key.virtualKey <= VK_RMENU) ||
+            (_kittyFlags != 0 ?
+                 // If KKP is enabled, we only report repeats if ReportEventTypes is enabled.
+                 WI_IsFlagClear(_kittyFlags, KittyKeyboardProtocolFlags::ReportEventTypes) :


We also shouldn't report key repeats at all if the flag is off.

lhecker · 2026-04-02T11:10:25Z

src/terminal/input/terminalInput.cpp

+    key.altGrPressed = anyAltPressed && anyCtrlPressed;
+    key.ctrlPressed = key.altGrPressed ? (bothCtrlPressed || (rightAltPressed && leftCtrlIsReallyPressed)) : anyCtrlPressed;
+    key.altPressed = key.altGrPressed ? bothAltPressed : anyAltPressed;
+    key.shiftPressed = WI_IsFlagSet(key.controlKeyState, SHIFT_PRESSED);


To make reliable AltGr detection possible throughout the code, I've made a bit of a more controversial change and removed the entire key.controlKeyState reliance throughout the code.

This crucially means that even the legacy key encoding will now treat AltGr distinct from other modifiers.

lhecker · 2026-04-02T11:11:42Z

src/terminal/input/terminalInput.cpp

    // NOTE: MSDN states that `lpSrcStr == lpDestStr` is valid for LCMAP_LOWERCASE.
-    len = LCMapStringW(LOCALE_INVARIANT, LCMAP_LOWERCASE, &buf[0], len, &buf[0], ARRAYSIZE(buf));
-    // NOTE: LCMapStringW returns the length including the null terminator.
-    len -= 1;


len -= 1 was wrong and only applies to encoding null-terminated strings. I have switched to a length-based buffer a long time ago and missed fixing this. 🙈

lhecker · 2026-04-02T11:12:23Z

src/terminal/input/terminalInput.cpp

+    // In the context of KKP, AltGr acts more like a keyboard "layer" toggle.
+    // It's not a modifier that's ever transmitted as-is and instead modifies the actual base key code.
+    if (key.altGrPressed)
+    {
+        controlKeyState |= LEFT_CTRL_PRESSED | RIGHT_ALT_PRESSED;
+    }


This new if condition ensures we treat AltGr like a layer shift that changes the base encoding of the key.

lhecker · 2026-04-03T16:19:25Z

src/terminal/adapter/ut_adapter/kittyKeyboardProtocol.cpp

-        { L"D Shift+Alt+a", L"\x1b[97;4u", D, true, 'A', 0x1E, L'A', Shift | Alt },
-        { L"D Shift+a", L"A", D, true, 'A', 0x1E, L'A', Shift },
+        { L"D Ctrl+a", L"\x1b[97;5u", D, true, 'A', 0x10, 0, Ctrl },
+        { L"D Ctrl+Alt+a", L"æ", D, true, 'A', 0x10, L'æ', Ctrl | Alt },


This has been rewritten to use French AZERTY events.

all of them ??? is that safe? ... are we guaranteed that it exists on OneCore?

should we have tests in US-104 for the most part and allow a TestCase to specify a keyboard layout to set?

src/terminal/adapter/ut_adapter/TestHook.cpp

src/terminal/adapter/ut_adapter/kittyKeyboardProtocol.cpp

src/terminal/input/terminalInput.cpp

DHowett · 2026-04-03T17:18:55Z

src/terminal/adapter/ut_adapter/inputTest.cpp

    Log::Comment(L"Sending every possible VKEY at the input stream for interception during key DOWN.");

-    BYTE vkey = LOBYTE(OneCoreSafeVkKeyScanW(0));
+    BYTE vkey = LOBYTE(VkKeyScanExW(0, layout));


We still need to use OneCoreSaveVkKeyScanW; this test will fail to run (or, maybe the entire fixture will fail to load?) if we link VkKeyScanW.

But it depends on LoadKeyboardLayout and ToUnicodeEx now. Won't all of these fail to work anyway? Do they have to work in the first? The TerminalInput logic is platform-independent with the exception of a single ToUnicodeEx call.

Okay, so here's what I have come to understand.

We refer to these APIs using an extension APIset.

ext-ms-win-ntuser-keyboard-l1-1-0 GetKeyState GetKeyboardLayout MapVirtualKeyW ToUnicodeEx VkKeyScanW

OneCoreSafeEtc runs those through coniosrv or equivalent rather than ntuser.
In theory, we could have done some effort to compose onecore with a new extension apiset that fills the same contract. Instead we used runtime dynamic dispatch. Eh.

As long as the implementation calls OneCoreSafeEtc, we're fine.

We will need to prevent the kitty tests from running on OneCore, so that we do not experience test gate failures. :)

lhecker · 2026-04-03T18:36:11Z

It is rather annoying that the /alternatename linker thing doesn't seem to work for x86.
And some of the KeyPressTests don't seem to work in CI which is extraordinarily annoying. This may mean that we have to either disable the KKP tests or remove them.

DHowett · 2026-04-03T19:41:37Z

It is rather annoying that the /alternatename linker thing doesn't seem to work for x86. And some of the KeyPressTests don't seem to work in CI which is extraordinarily annoying. This may mean that we have to either disable the KKP tests or remove them.

Unfortunately, the deal with TestHook is that the no-op implementation must be in a separate .obj in the LIB-under-test. It cannot be an alternatename or some other weak definition kind of deal.

This type of test hook relies on the classic model of linking where objects are only pulled in even from libs if they are referred to for an unresolved symbol. TerminalInput.lib!TestHook.obj must be able to be skipped wholesale because it does not contribute an unresolved symbol to the test DLL.

DHowett · 2026-04-03T19:42:10Z

(Whereas you have TestHook.obj in the UT project; you rather need TestHook.obj in TerminalInput which contains the no-op implementation; then you can implement the hook in the UT in whatever object file you want, including the test file that relies on the overridden hook)

Fix various KKP encoding issues

446f0df

github-advanced-security bot found potential problems Apr 2, 2026

View reviewed changes

src/terminal/input/terminalInput.cpp Fixed Show fixed Hide fixed

src/terminal/input/terminalInput.cpp Fixed Show fixed Hide fixed

src/terminal/input/terminalInput.cpp Fixed Show fixed Hide fixed

src/terminal/input/terminalInput.cpp Fixed Show fixed Hide fixed

This comment has been minimized.

Sign in to view

Spel

e27f76d

lhecker commented Apr 2, 2026

View reviewed changes

lhecker added 3 commits April 3, 2026 18:16

Rethought how AltGr works

f0e3432

Fixed TerminalInputModifierKeyTests

fcda6e9

Added test hooks, Use French AZERTY for tests

0e4d6b8

lhecker commented Apr 3, 2026

View reviewed changes

github-advanced-security bot found potential problems Apr 3, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

SPELL CHECK

552a79f

DHowett reviewed Apr 3, 2026

View reviewed changes

Conversation

lhecker commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Validation Steps Performed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker commented Apr 3, 2026

Uh oh!

DHowett commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lhecker commented Apr 2, 2026 •

edited

Loading

DHowett commented Apr 3, 2026 •

edited

Loading

DHowett commented Apr 3, 2026 •

edited

Loading