Fix #7064: Ignore key events without scan code by lhecker · Pull Request #7145 · microsoft/terminal

Leonard Hecker (lhecker) · 2020-08-01T14:23:45Z

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

Mike Griese (zadjii-msft) · 2020-08-03T12:57:46Z

This seems reasonable to me, but I also don't really know if there are other kinds of inputs without scan codes. I guess I haven't seen it, but I'm also not usually looking for that.

I'm not sure how we could be more pre-emptive about other input methods that we don't know, but we could make sure to add tests to cover this case, so that we don't regress it again in the future. Maybe something like just testing that HandleKey with a 0 scancode returns false? I know that doesn't check that we still get a character event afterwards, but it's something...

Leonard Hecker (lhecker) · 2020-08-03T16:38:49Z

I‘ll add a unit test as soon as I can. 🙂

Michael Niksa (miniksa) · 2020-08-04T18:24:15Z

I don't really see a usage of 0 scan code that would turn into something we could know how to handle without further user32 intervention.

So I'm okay with this being "it's better than it was and if something proves us wrong, we'll refine further then."

Leonard Hecker (lhecker) · 2020-08-04T22:00:45Z

PR fixed. 👍

Mike Griese (zadjii-msft)

Sure let's try it out, and refine if someone screams in the future ☺️

Dustin L. Howett (DHowett) · 2020-08-06T01:00:20Z

Okay, let's scream test this!

Deleted user (ghost) · 2020-08-06T01:03:44Z

Hello Dustin L. Howett (@DHowett)!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Dustin L. Howett (DHowett) · 2020-08-06T01:04:38Z

As always, thank you Leonard Hecker (@lhecker)!

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 (cherry picked from commit b617c43)

Deleted user (ghost) · 2020-08-13T18:54:12Z

🎉Windows Terminal Preview v1.2.2234.0 has been released which incorporates this pull request.:tada:

Handy links:

#7145 introduced a check so that we wouldn't dispatch keys unless they actually had a scancode. Our synthetic events actually _didn't_ have scancodes. Not because they couldn't--just because they didn't. Fixes #7297

#7145 introduced a check so that we wouldn't dispatch keys unless they actually had a scancode. Our synthetic events actually _didn't_ have scancodes. Not because they couldn't--just because they didn't. Fixes #7297 (cherry picked from commit aecd99e)

…virtual keycode and a scan code of 0 (#15753) ## Summary Applications like PowerToys, with their keyboard remapping features frequently (i.e whenever a remapped shortcut is triggerred) send `KeyEvent` with out-of-range virtual keycode values (E.g. 0xFF). This is fixed for WT in #7145, we just needed it in our good ol' `conhost`. After this PR, Key events with an invalid virtual keycode and scancode==0 are ignored, and are not added to the `InputBuffer`. Incase, only virtual keycode is valid but not scancode, we will try to infer the correct scancode using the virtual keycode mapping. ## References and Relevant Issues #7145 #7064 ## Validation Steps Performed - Triggered a remapped shortcut and verified that `showkey -a` doesn't output `^@` unexpectedly. - Key events with an Invalid virtual Keycode and Scancode == 0 are ignored. - This PR doesn't include any changes for `WM_[SYS][DEAD]CHAR` messages, they are left unchanged.

ghost added Area-Input Related to input processing (key presses, mouse, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Aug 1, 2020

Leonard Hecker (lhecker) mentioned this pull request Aug 1, 2020

Autohotkey AltGr+7, AltGr+8, AltGr+9, and AltGr+0 reassignments lost in Windows Terminal Preview and Windows Terminal #7064

Closed

Fix #7064: Ignore key events without scan code

7d8f3a6

Mike Griese (zadjii-msft) approved these changes Aug 5, 2020

View reviewed changes

Mike Griese (zadjii-msft) added the Needs-Second It's a PR that needs another sign-off label Aug 5, 2020

ghost requested review from Dustin L. Howett (DHowett), Carlos Zamora (carlos-zamora), Leon Liang (leonMSFT) and Michael Niksa (miniksa) August 5, 2020 14:56

ghost requested a review from PankajBhojwani August 6, 2020 01:01

Dustin L. Howett (DHowett) approved these changes Aug 6, 2020

View reviewed changes

ghost requested a review from Dustin L. Howett (DHowett) August 6, 2020 01:03

Dustin L. Howett (DHowett) added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 6, 2020

ghost merged commit b617c43 into microsoft:master Aug 6, 2020

ghost mentioned this pull request Aug 13, 2020

Windows Terminal 1.1 breaks remapped CapsLock key inside WSL #7120

Closed

Leonard Hecker (lhecker) deleted the issue-7064-fix branch August 13, 2020 21:02

This was referenced Aug 14, 2020

F7 tunneling broken #7297

Closed

Pass the scancode in our tunneled DirectKey event #7298

Merged

Tushar Singh (tusharsnx) mentioned this pull request Jul 24, 2023

ConHost: Ignore WM_[SYS]KEYDOWN/WM_[SYS]KEYUP events with an invalid virtual keycode and a scan code of 0 #15753

Merged

This pull request was closed.

Navigation Menu

Repository navigation

Fix #7064: Ignore key events without scan code - #7145

Fix #7064: Ignore key events without scan code#7145
1 commit merged into
microsoft:masterfrom
lhecker:issue-7064-fix

Leonard Hecker (lhecker) commented Aug 1, 2020 •

edited by DHowett

Loading

Uh oh!

Mike Griese (zadjii-msft) commented Aug 3, 2020

Uh oh!

Leonard Hecker (lhecker) commented Aug 3, 2020

Uh oh!

Michael Niksa (miniksa) commented Aug 4, 2020

Uh oh!

Leonard Hecker (lhecker) commented Aug 4, 2020

Uh oh!

Mike Griese (zadjii-msft) left a comment

Uh oh!

Dustin L. Howett (DHowett) commented Aug 6, 2020

Uh oh!

Deleted user (ghost) commented Aug 6, 2020

Uh oh!

Dustin L. Howett (DHowett) commented Aug 6, 2020

Uh oh!

Deleted user (ghost) commented Aug 13, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Leonard Hecker (lhecker) commented Aug 1, 2020 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Validation Steps Performed

Uh oh!

Mike Griese (zadjii-msft) commented Aug 3, 2020

Uh oh!

Leonard Hecker (lhecker) commented Aug 3, 2020

Uh oh!

Michael Niksa (miniksa) commented Aug 4, 2020

Uh oh!

Leonard Hecker (lhecker) commented Aug 4, 2020

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Dustin L. Howett (DHowett) commented Aug 6, 2020

Uh oh!

Deleted user (ghost) commented Aug 6, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Dustin L. Howett (DHowett) commented Aug 6, 2020

Uh oh!

Deleted user (ghost) commented Aug 13, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Leonard Hecker (lhecker) commented Aug 1, 2020 •

edited by DHowett

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.