Skip to content

Commit

Permalink
Fix #7064: Ignore key events without scan code (#7145)
Browse files Browse the repository at this point in the history
Up until #4999 we deferred all key events to the character event handler
for which `ToUnicodeEx` returned a valid character and alternatively
those who aren't a special key combination as listed in
`TerminalInput`'s implementation.

Since #4999 we started acknowledging/handling all key events no matter
whether they're actually a known key combination. Given non-ASCII inputs
the Win32 `SendInput()` method generates certain sequences that aren't
recognizable combinations though and if they're handled by the key event
handler no follow up character event is sent containing the unicode
character.

This PR adds another condition and defers all key events without scan
code (i.e. those not representable by the current keyboard layout) to
the character event handler.

I'm absolutely not certain that this PR doesn't have a negative effect
on other kinds of inputs.

Is it common for key events to not contain a scan code? I personally
haven't seen it happen before AutoHotKey/SendInput.

Before this PR is merged it'd be nice to have a good testing plan in
place in order to ensure nothing breaks.

## Validation Steps Performed

Remapped `AltGr+8` to `»` using AutoHotKey using `<^>!8::SendInput {Raw}»`.
Ensured `»` is printed if `AltGr+8` is pressed.

Closes #7064
Closes #7120
  • Loading branch information
lhecker authored Aug 6, 2020
1 parent b759bdb commit b617c43
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 0 deletions.
13 changes: 13 additions & 0 deletions src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,19 @@ bool Terminal::SendKeyEvent(const WORD vkey,

_StoreKeyEvent(vkey, scanCode);

// As a Terminal we're mostly interested in getting key events from physical hardware (mouse & keyboard).
// We're thus ignoring events whose values are outside the valid range and unlikely to be generated by the current keyboard.
// It's very likely that a proper followup character event will be sent to us.
// This prominently happens using AutoHotKey's keyboard remapping feature,
// which sends input events whose vkey is 0xff and scanCode is 0.
// We need to check for this early, as _CharacterFromKeyEvent() always returns 0 for such invalid values,
// making us believe that this is an actual non-character input, while it usually isn't.
// GH#7064
if (vkey == 0 || vkey >= 0xff || scanCode == 0)
{
return false;
}

const auto isAltOnlyPressed = states.IsAltPressed() && !states.IsCtrlPressed();
const auto isSuppressedAltGrAlias = !_altGrAliasing && states.IsAltPressed() && states.IsCtrlPressed();

Expand Down
11 changes: 11 additions & 0 deletions src/cascadia/UnitTests_TerminalCore/InputTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ namespace TerminalCoreUnitTests

TEST_METHOD(AltShiftKey);
TEST_METHOD(AltSpace);
TEST_METHOD(InvalidKeyEvent);

void _VerifyExpectedInput(std::wstring& actualInput)
{
Expand Down Expand Up @@ -65,4 +66,14 @@ namespace TerminalCoreUnitTests
VERIFY_IS_FALSE(term.SendKeyEvent(L' ', 0, ControlKeyStates::LeftAltPressed, false));
VERIFY_IS_FALSE(term.SendCharEvent(L' ', 0, ControlKeyStates::LeftAltPressed));
}

void InputTest::InvalidKeyEvent()
{
// Certain applications like AutoHotKey and its keyboard remapping feature,
// send us key events using SendInput() whose values are outside of the valid range.
// We don't want to handle those events as we're probably going to get a proper followup character event.
VERIFY_IS_FALSE(term.SendKeyEvent(0, 123, {}, true));
VERIFY_IS_FALSE(term.SendKeyEvent(255, 123, {}, true));
VERIFY_IS_FALSE(term.SendKeyEvent(123, 0, {}, true));
}
}

0 comments on commit b617c43

Please sign in to comment.