Skip to content

Commit

Permalink
Refactor VT terminal input (#16511)
Browse files Browse the repository at this point in the history 
The primary reason for this refactoring was to simplify the management
of VT input sequences that vary depending on modes, adding support for
the missing application keypad sequences, and preparing the way for
future extensions like `S8C1T`.

However, it also includes fixes for a number of keyboard related bugs,
including a variety of missing or incorrect mappings for the `Ctrl` and
`Ctrl`+`Alt` key combinations, 

## References and Relevant Issues

This PR also includes a fix for #10308, which was previously closed as a
duplicate of #10551. I don't think those bugs were related, though, and
although they're both supposed to be fixed in Windows 11, this PR fixes
the issue in Windows 10.

## Detailed Description of the Pull Request / Additional comments

The way the input now works, there's a single keyboard map that takes a
virtual key code combined with `Ctrl`, `Alt`, and `Shift` modifier bits
as the lookup key, and the expected VT input sequence as the value. This
map is initially constructed at startup, and then regenerated whenever a
keyboard mode is changed.

This map takes care of the cursor keys, editing keys, function keys, and
keys like `BkSp` and `Return` which can be affected by mode changes. The
remaining "graphic" key combinations are determined manually at the time
of input.

The order of precedence looks like this:

1. If the virtual key is `0` or `VK_PACKET`, it's considered to be a
   synthesized keyboard event, and the `UnicodeChar` value is used
   exactly as given.

2. If it's a numeric keypad key, and `Alt` is pressed (but not `Ctrl`),
   then it's assumedly part of an Alt-Numpad composition, so the key
   press is ignored (the generated character will be transmitted when
   the `Alt` is released).

3. If the virtual key combined with modifier bits is found in the key
   map described above, then the matched escape sequence will be used
   used as the output.

4. If a `UnicodeChar` value has been provided, that will be used as the
   output, but possibly with additional Ctrl and Alt modifiers applied:

   a. If it's an `AltGr` key, and we've got either two `Ctrl` keys
      pressed or a left `Ctrl` key that is distinctly separate from a
      right `Alt` key, then we will try and convert the character into
      a C0 control code.

   b. If an `Alt` key is pressed (or in the case of an `AltGr` value,
      both `Alt` keys are pressed), then we will convert it into an
      Alt-key sequence by prefixing the character with an `ESC`.

5. If we don't have a `UnicodeChar`, we'll use the `ToUnicodeEx` API to
   check whether the current keyboard state reflects a dead key, and if
   so, return nothing.

6. Otherwise we'll make another `ToUnicodeEx` call but with any `Ctrl`
   and `Alt` modifiers removed from the state to determine the base key
   value. Once we have that, we can apply the modifiers ourself.

   a. If the `Ctrl` key is pressed, we'll try and convert the base value
      into a C0 control code. But if we can't do that, we'll try again
      with the virtual key code (if it's alphanumeric) as a fallback.

   b. If the `Alt` key is pressed, we'll convert the base value (or
      control code value) into an Alt-key sequence by prefixing it with
      an `ESC`.

For step 4-a, we determine whether the left `Ctrl` key is distinctly
separate from the right `Alt` key by recording the time that those keys
are pressed, and checking for a time gap greater than 50ms. This is
necessary to distinguish between the user pressing `Ctrl`+`AltGr`, or
just pressing `AltGr` alone, which triggers a fake `Ctrl` key press at
the same time.

## Validation Steps Performed

I created a test script to automate key presses in the terminal window
for every relevant key, along with every Ctrl/Alt/Shift modifier, and
every relevant mode combination. I then compared the generated input
sequences with XTerm and a DEC VT240 terminal. The idea wasn't to match
either of them exactly, but to make sure the places where we differed
were intentional and reasonable.

This mostly dealt with the US keyboard layout. Comparing international
layouts wasn't really feasible because DEC, Linux, and Windows keyboard
assignments tend to be quite different. However, I've manually tested a
number of different layouts, and tried to make sure that they were all
working in a reasonable manner.

In terms of unit testing, I haven't done much more than patching the
ones that already existed to get them to pass. They're honestly not
great tests, because they aren't generating events in the form that
you'd expect for a genuine key press, and that can significantly affect
the results, but I can't think of an easy way to improve them.

## PR Checklist
- [x] Closes #16506
- [x] Closes #16508
- [x] Closes #16509
- [x] Closes #16510
- [x] Closes #3483
- [x] Closes #11194
- [x] Closes #11700
- [x] Closes #12555
- [x] Closes #13319
- [x] Closes #15367
- [x] Closes #16173
- [x] Tests added/passed
  • Loading branch information
@j4james
j4james committed Jan 30, 2024
1 parent 86c30bd commit dccc1f4
Show file tree
Hide file tree
Showing 6 changed files with 661 additions and 634 deletions.
2 changes: 2 additions & 0 deletions .github/actions/spelling/expect/expect.txt
View file
Expand Up @@ -319,6 +319,7 @@ ctlseqs
CTRLEVENT
CTRLFREQUENCY
CTRLKEYSHORTCUTS
Ctrls
CTRLVOLUME
Ctxt
CUF
Expand Down Expand Up @@ -401,6 +402,7 @@ DECECM
DECEKBD
DECERA
DECFI
DECFNK
DECFRA
DECIC
DECID
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/TermControl.cpp
View file
Expand Up @@ -1336,7 +1336,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// Alt, so we should be ignoring the individual keydowns. The character
// will be sent through the TSFInputControl. See GH#1401 for more
// details
if (modifiers.IsAltPressed() &&
if (modifiers.IsAltPressed() && !modifiers.IsCtrlPressed() &&
(vkey >= VK_NUMPAD0 && vkey <= VK_NUMPAD9))
{
e.Handled(true);
Expand Down
218 changes: 162 additions & 56 deletions src/terminal/adapter/ut_adapter/inputTest.cpp
View file
Expand Up @@ -66,6 +66,32 @@ class Microsoft::Console::VirtualTerminal::InputTest
{
return WI_IsFlagSet(uiKeystate, SHIFT_PRESSED);
}

static void TestKey(const TerminalInput::OutputType& expected, TerminalInput& input, const unsigned int uiKeystate, const BYTE vkey, const wchar_t wch = 0)
{
Log::Comment(NoThrowString().Format(L"Testing key, state =0x%x, 0x%x", vkey, uiKeystate));

INPUT_RECORD irTest = { 0 };
irTest.EventType = KEY_EVENT;
irTest.Event.KeyEvent.wRepeatCount = 1;
irTest.Event.KeyEvent.bKeyDown = TRUE;

// If we want to test a key with the Right Alt modifier, we must generate
// an event for the Alt key first, otherwise the modifier will be dropped.
if (WI_IsFlagSet(uiKeystate, RIGHT_ALT_PRESSED))
{
irTest.Event.KeyEvent.wVirtualKeyCode = VK_MENU;
irTest.Event.KeyEvent.dwControlKeyState = uiKeystate | ENHANCED_KEY;
VERIFY_ARE_EQUAL(TerminalInput::MakeOutput({}), input.HandleKey(irTest));
}

irTest.Event.KeyEvent.dwControlKeyState = uiKeystate;
irTest.Event.KeyEvent.wVirtualKeyCode = vkey;
irTest.Event.KeyEvent.uChar.UnicodeChar = wch;

// Send key into object (will trigger callback and verification)
VERIFY_ARE_EQUAL(expected, input.HandleKey(irTest), L"Verify key was handled if it should have been.");
}
};

void InputTest::TerminalInputTests()
Expand All @@ -86,7 +112,8 @@ void InputTest::TerminalInputTests()
irTest.Event.KeyEvent.bKeyDown = TRUE;
irTest.Event.KeyEvent.uChar.UnicodeChar = LOWORD(OneCoreSafeMapVirtualKeyW(vkey, MAPVK_VK_TO_CHAR));

TerminalInput::OutputType expected;
// Unhandled keys are expected to return an empty string.
TerminalInput::OutputType expected = TerminalInput::MakeOutput({});
switch (vkey)
{
case VK_TAB:
Expand All @@ -113,6 +140,9 @@ void InputTest::TerminalInputTests()
case VK_LEFT:
expected = TerminalInput::MakeOutput(L"\x1b[D");
break;
case VK_CLEAR:
expected = TerminalInput::MakeOutput(L"\x1b[E");
break;
case VK_HOME:
expected = TerminalInput::MakeOutput(L"\x1b[H");
break;
Expand Down Expand Up @@ -167,11 +197,36 @@ void InputTest::TerminalInputTests()
case VK_F12:
expected = TerminalInput::MakeOutput(L"\x1b[24~");
break;
case VK_F13:
expected = TerminalInput::MakeOutput(L"\x1b[25~");
break;
case VK_F14:
expected = TerminalInput::MakeOutput(L"\x1b[26~");
break;
case VK_F15:
expected = TerminalInput::MakeOutput(L"\x1b[28~");
break;
case VK_F16:
expected = TerminalInput::MakeOutput(L"\x1b[29~");
break;
case VK_F17:
expected = TerminalInput::MakeOutput(L"\x1b[31~");
break;
case VK_F18:
expected = TerminalInput::MakeOutput(L"\x1b[32~");
break;
case VK_F19:
expected = TerminalInput::MakeOutput(L"\x1b[33~");
break;
case VK_F20:
expected = TerminalInput::MakeOutput(L"\x1b[34~");
break;
case VK_CANCEL:
expected = TerminalInput::MakeOutput(L"\x3");
break;
default:
if (irTest.Event.KeyEvent.uChar.UnicodeChar != 0)
const auto synthesizedKeyPress = vkey == VK_PACKET || vkey == 0;
if (irTest.Event.KeyEvent.uChar.UnicodeChar != 0 || synthesizedKeyPress)
{
expected = TerminalInput::MakeOutput({ &irTest.Event.KeyEvent.uChar.UnicodeChar, 1 });
}
Expand All @@ -194,7 +249,7 @@ void InputTest::TerminalInputTests()
irTest.Event.KeyEvent.bKeyDown = FALSE;

// Send key into object (will trigger callback and verification)
VERIFY_ARE_EQUAL(TerminalInput::MakeUnhandled(), input.HandleKey(irTest), L"Verify key was NOT handled.");
VERIFY_ARE_EQUAL(TerminalInput::MakeOutput({}), input.HandleKey(irTest), L"Verify output is blank.");
}

Log::Comment(L"Verify other types of events are not handled/intercepted.");
Expand Down Expand Up @@ -259,13 +314,7 @@ void InputTest::TerminalInputModifierKeyTests()

auto fExpectedKeyHandled = true;
auto fModifySequence = false;
INPUT_RECORD irTest = { 0 };
irTest.EventType = KEY_EVENT;
irTest.Event.KeyEvent.dwControlKeyState = uiKeystate;
irTest.Event.KeyEvent.wRepeatCount = 1;
irTest.Event.KeyEvent.wVirtualKeyCode = vkey;
irTest.Event.KeyEvent.bKeyDown = TRUE;
irTest.Event.KeyEvent.uChar.UnicodeChar = LOWORD(OneCoreSafeMapVirtualKeyW(vkey, MAPVK_VK_TO_CHAR));
wchar_t ch = LOWORD(OneCoreSafeMapVirtualKeyW(vkey, MAPVK_VK_TO_CHAR));

if (ControlPressed(uiKeystate))
{
Expand All @@ -282,16 +331,13 @@ void InputTest::TerminalInputModifierKeyTests()
}
}

TerminalInput::OutputType expected;
// Unhandled keys are expected to return an empty string.
TerminalInput::OutputType expected = TerminalInput::MakeOutput({});
switch (vkey)
{
case VK_BACK:
// Backspace is kinda different from other keys - we'll handle in another test.
case VK_OEM_2:
// VK_OEM_2 is typically the '/?' key
continue;
// expected = TerminalInput::MakeOutput(L"\x7f");
break;
case VK_PAUSE:
expected = TerminalInput::MakeOutput(L"\x1a");
break;
Expand All @@ -311,6 +357,10 @@ void InputTest::TerminalInputModifierKeyTests()
fModifySequence = true;
expected = TerminalInput::MakeOutput(L"\x1b[1;mD");
break;
case VK_CLEAR:
fModifySequence = true;
expected = TerminalInput::MakeOutput(L"\x1b[1;mE");
break;
case VK_HOME:
fModifySequence = true;
expected = TerminalInput::MakeOutput(L"\x1b[1;mH");
Expand Down Expand Up @@ -383,6 +433,55 @@ void InputTest::TerminalInputModifierKeyTests()
fModifySequence = true;
expected = TerminalInput::MakeOutput(L"\x1b[24;m~");
break;
case VK_F13:
fModifySequence = true;
expected = TerminalInput::MakeOutput(L"\x1b[25;m~");
break;
case VK_F14:
fModifySequence = true;
expected = TerminalInput::MakeOutput(L"\x1b[26;m~");
break;
case VK_F15:
fModifySequence = true;
expected = TerminalInput::MakeOutput(L"\x1b[28;m~");
break;
case VK_F16:
fModifySequence = true;
expected = TerminalInput::MakeOutput(L"\x1b[29;m~");
break;
case VK_F17:
fModifySequence = true;
expected = TerminalInput::MakeOutput(L"\x1b[31;m~");
break;
case VK_F18:
fModifySequence = true;
expected = TerminalInput::MakeOutput(L"\x1b[32;m~");
break;
case VK_F19:
fModifySequence = true;
expected = TerminalInput::MakeOutput(L"\x1b[33;m~");
break;
case VK_F20:
fModifySequence = true;
expected = TerminalInput::MakeOutput(L"\x1b[34;m~");
break;
case VK_PACKET:
case 0:
// VK_PACKET and 0 virtual keys are used for synthesized key presses.
expected = TerminalInput::MakeOutput({ &ch, 1 });
break;
case VK_RETURN:
if (AltPressed(uiKeystate))
{
const auto str = ControlPressed(uiKeystate) ? L"\x1b\n" : L"\x1b\r";
expected = TerminalInput::MakeOutput(str);
}
else
{
const auto str = ControlPressed(uiKeystate) ? L"\n" : L"\r";
expected = TerminalInput::MakeOutput(str);
}
break;
case VK_TAB:
if (AltPressed(uiKeystate))
{
Expand All @@ -398,8 +497,35 @@ void InputTest::TerminalInputModifierKeyTests()
expected = TerminalInput::MakeOutput(L"\t");
}
break;
case VK_OEM_2:
case VK_OEM_3:
case VK_OEM_4:
case VK_OEM_5:
case VK_OEM_6:
case VK_OEM_102:
// OEM keys require special case handling when combined with a Ctrl
// modifier, but otherwise work the same way as regular keys.
if (ControlPressed(uiKeystate))
{
continue;
}
[[fallthrough]];
default:
auto ch = irTest.Event.KeyEvent.uChar.UnicodeChar;
if (ControlPressed(uiKeystate) && (vkey >= '1' && vkey <= '9'))
{
// The C-# keys get translated into very specific control
// characters that don't play nicely with this test. These keys
// are tested in the CtrlNumTest Test instead.
continue;
}

if (vkey >= VK_NUMPAD0 && vkey <= VK_NUMPAD9)
{
// Numpad keys have the same complications as numeric keys
// when used with a Ctrl modifier, and with Alt they're used
// for Alt-Numpad composition, so it's best we skip them.
continue;
}

// Alt+Key generates [0x1b, Ctrl+key] into the stream
// Pressing the control key causes all bits but the 5 least
Expand All @@ -408,28 +534,25 @@ void InputTest::TerminalInputModifierKeyTests()
{
const wchar_t buffer[2]{ L'\x1b', gsl::narrow_cast<wchar_t>(ch & 0b11111) };
expected = TerminalInput::MakeOutput({ &buffer[0], 2 });
ch = 0;
break;
}

// Alt+Key generates [0x1b, key] into the stream
if (AltPressed(uiKeystate) && !ControlPressed(uiKeystate) && ch != 0)
if (AltPressed(uiKeystate) && ch != 0)
{
const wchar_t buffer[2]{ L'\x1b', ch };
expected = TerminalInput::MakeOutput({ &buffer[0], 2 });
if (ControlPressed(uiKeystate))
{
ch = 0;
}
break;
}

if (ControlPressed(uiKeystate) && (vkey >= '1' && vkey <= '9'))
{
// The C-# keys get translated into very specific control
// characters that don't play nicely with this test. These keys
// are tested in the CtrlNumTest Test instead.
continue;
}

if (ch != 0)
{
expected = TerminalInput::MakeOutput({ &irTest.Event.KeyEvent.uChar.UnicodeChar, 1 });
expected = TerminalInput::MakeOutput({ &ch, 1 });
break;
}

Expand All @@ -446,8 +569,7 @@ void InputTest::TerminalInputModifierKeyTests()
str[str.size() - 2] = L'1' + (fShift ? 1 : 0) + (fAlt ? 2 : 0) + (fCtrl ? 4 : 0);
}

// Send key into object (will trigger callback and verification)
VERIFY_ARE_EQUAL(expected, input.HandleKey(irTest), L"Verify key was handled if it should have been.");
TestKey(expected, input, uiKeystate, vkey, ch);
}
}

Expand Down Expand Up @@ -491,22 +613,6 @@ void InputTest::TerminalInputNullKeyTests()
VERIFY_ARE_EQUAL(TerminalInput::MakeOutput(L"\x1b\0"sv), input.HandleKey(irTest), L"Verify key was handled if it should have been.");
}

static void TestKey(const TerminalInput::OutputType& expected, TerminalInput& input, const unsigned int uiKeystate, const BYTE vkey, const wchar_t wch = 0)
{
Log::Comment(NoThrowString().Format(L"Testing key, state =0x%x, 0x%x", vkey, uiKeystate));

INPUT_RECORD irTest = { 0 };
irTest.EventType = KEY_EVENT;
irTest.Event.KeyEvent.dwControlKeyState = uiKeystate;
irTest.Event.KeyEvent.wRepeatCount = 1;
irTest.Event.KeyEvent.wVirtualKeyCode = vkey;
irTest.Event.KeyEvent.bKeyDown = TRUE;
irTest.Event.KeyEvent.uChar.UnicodeChar = wch;

// Send key into object (will trigger callback and verification)
VERIFY_ARE_EQUAL(expected, input.HandleKey(irTest), L"Verify key was handled if it should have been.");
}

void InputTest::DifferentModifiersTest()
{
Log::Comment(L"Starting test...");
Expand Down Expand Up @@ -556,9 +662,9 @@ void InputTest::DifferentModifiersTest()
// C-/ -> C-_ -> 0x1f
uiKeystate = LEFT_CTRL_PRESSED;
vkey = LOBYTE(OneCoreSafeVkKeyScanW(L'/'));
TestKey(TerminalInput::MakeOutput(L"\x1f"), input, uiKeystate, vkey, L'/');
TestKey(TerminalInput::MakeOutput(L"\x1f"), input, uiKeystate, vkey);
uiKeystate = RIGHT_CTRL_PRESSED;
TestKey(TerminalInput::MakeOutput(L"\x1f"), input, uiKeystate, vkey, L'/');
TestKey(TerminalInput::MakeOutput(L"\x1f"), input, uiKeystate, vkey);

// M-/ -> ESC /
uiKeystate = LEFT_ALT_PRESSED;
Expand All @@ -572,26 +678,26 @@ void InputTest::DifferentModifiersTest()
Log::Comment(NoThrowString().Format(L"Checking C-?"));
// Use SHIFT_PRESSED to force us into differentiating between '/' and '?'
vkey = LOBYTE(OneCoreSafeVkKeyScanW(L'?'));
TestKey(TerminalInput::MakeOutput(L"\x7f"), input, SHIFT_PRESSED | LEFT_CTRL_PRESSED, vkey, L'?');
TestKey(TerminalInput::MakeOutput(L"\x7f"), input, SHIFT_PRESSED | RIGHT_CTRL_PRESSED, vkey, L'?');
TestKey(TerminalInput::MakeOutput(L"\x7f"), input, SHIFT_PRESSED | LEFT_CTRL_PRESSED, vkey);
TestKey(TerminalInput::MakeOutput(L"\x7f"), input, SHIFT_PRESSED | RIGHT_CTRL_PRESSED, vkey);

// C-M-/ -> 0x1b0x1f
Log::Comment(NoThrowString().Format(L"Checking C-M-/"));
uiKeystate = LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED;
vkey = LOBYTE(OneCoreSafeVkKeyScanW(L'/'));
TestKey(TerminalInput::MakeOutput(L"\x1b\x1f"), input, LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey, L'/');
TestKey(TerminalInput::MakeOutput(L"\x1b\x1f"), input, RIGHT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey, L'/');
TestKey(TerminalInput::MakeOutput(L"\x1b\x1f"), input, LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey);
TestKey(TerminalInput::MakeOutput(L"\x1b\x1f"), input, RIGHT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey);
// LEFT_CTRL_PRESSED | RIGHT_ALT_PRESSED is skipped because that's AltGr
TestKey(TerminalInput::MakeOutput(L"\x1b\x1f"), input, RIGHT_CTRL_PRESSED | RIGHT_ALT_PRESSED, vkey, L'/');
TestKey(TerminalInput::MakeOutput(L"\x1b\x1f"), input, RIGHT_CTRL_PRESSED | RIGHT_ALT_PRESSED, vkey);

// C-M-? -> 0x1b0x7f
Log::Comment(NoThrowString().Format(L"Checking C-M-?"));
uiKeystate = LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED;
vkey = LOBYTE(OneCoreSafeVkKeyScanW(L'?'));
TestKey(TerminalInput::MakeOutput(L"\x1b\x7f"), input, SHIFT_PRESSED | LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey, L'?');
TestKey(TerminalInput::MakeOutput(L"\x1b\x7f"), input, SHIFT_PRESSED | RIGHT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey, L'?');
TestKey(TerminalInput::MakeOutput(L"\x1b\x7f"), input, SHIFT_PRESSED | LEFT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey);
TestKey(TerminalInput::MakeOutput(L"\x1b\x7f"), input, SHIFT_PRESSED | RIGHT_CTRL_PRESSED | LEFT_ALT_PRESSED, vkey);
// LEFT_CTRL_PRESSED | RIGHT_ALT_PRESSED is skipped because that's AltGr
TestKey(TerminalInput::MakeOutput(L"\x1b\x7f"), input, SHIFT_PRESSED | RIGHT_CTRL_PRESSED | RIGHT_ALT_PRESSED, vkey, L'?');
TestKey(TerminalInput::MakeOutput(L"\x1b\x7f"), input, SHIFT_PRESSED | RIGHT_CTRL_PRESSED | RIGHT_ALT_PRESSED, vkey);
}

void InputTest::CtrlNumTest()
Expand Down Expand Up @@ -674,13 +780,13 @@ void InputTest::AutoRepeatModeTest()
VERIFY_ARE_EQUAL(TerminalInput::MakeOutput(L"A"), input.HandleKey(down));
VERIFY_ARE_EQUAL(TerminalInput::MakeOutput({}), input.HandleKey(down));
VERIFY_ARE_EQUAL(TerminalInput::MakeOutput({}), input.HandleKey(down));
VERIFY_ARE_EQUAL(TerminalInput::MakeUnhandled(), input.HandleKey(up));
VERIFY_ARE_EQUAL(TerminalInput::MakeOutput({}), input.HandleKey(up));

Log::Comment(L"Sending repeating keypresses with DECARM enabled.");

input.SetInputMode(TerminalInput::Mode::AutoRepeat, true);
VERIFY_ARE_EQUAL(TerminalInput::MakeOutput(L"A"), input.HandleKey(down));
VERIFY_ARE_EQUAL(TerminalInput::MakeOutput(L"A"), input.HandleKey(down));
VERIFY_ARE_EQUAL(TerminalInput::MakeOutput(L"A"), input.HandleKey(down));
VERIFY_ARE_EQUAL(TerminalInput::MakeUnhandled(), input.HandleKey(up));
VERIFY_ARE_EQUAL(TerminalInput::MakeOutput({}), input.HandleKey(up));
}

0 comments on commit dccc1f4

Please sign in to comment.