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

TerminalInput::HandleKey should never return "unhandled" for a key event #16509

Closed
j4james opened this issue Dec 31, 2023 · 0 comments · Fixed by #16511
Closed

TerminalInput::HandleKey should never return "unhandled" for a key event #16509

j4james opened this issue Dec 31, 2023 · 0 comments · Fixed by #16511
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase

Comments

@j4james
Copy link
Collaborator

j4james commented Dec 31, 2023

Description of the new feature/enhancement

If I've enabled VT input mode, I expect to get back a simple stream of characters representing the VT sequences for each key press. If using ReadConsoleInput, then I'd expect a single key event for each character in the sequence, with no virtual key code, and with the character values in the UnicodeChar field.

However, what I get now is a weird mix of the original keyboard events, interspersed with the VT sequence characters. For example, if I press Ctrl+A, I expect to get a single event like this:

bKeyDown wVirtualKeyCode UnicodeChar
DOWN 0x00 U+0001

But what I actually get is this:

bKeyDown wVirtualKeyCode UnicodeChar
DOWN VK_CONTROL U+0000
DOWN 0x00 U+0001
UP VK_A U+0001
UP VK_CONTROL U+0000

If I enter a character consisting of a surrogate pair, e.g. entering 😛 from the Win+. emoji picker, I get both up and down events for the two surrogate pairs, but the up event arrives before the down event for the first code point!

bKeyDown wVirtualKeyCode UnicodeChar
UP VK_OEM_PERIOD U+002E
UP 0x00 U+D83D
DOWN 0x00 U+D83D
DOWN 0x00 U+DE1B
UP 0x00 U+DE1B

And note that I also get the up event for the . key from triggering the emoji picker. All I expected to see were the two down events representing the emoji glyph:

bKeyDown wVirtualKeyCode UnicodeChar
DOWN 0x00 U+D83D
DOWN 0x00 U+DE1B

This all stems from the fact that we return an "unhandled" state from the TerminalInput::HandleKey method for key-up events, as well as key-down events that don't map to a VT sequence, and those events then get injected back into the input queue. I think it would be better if we just returned an empty string for those cases. That way the only key events that end up in the queue are the actual VT characters.

Proposed technical implementation details (optional)

As explained above, we should only return MakeUnhandled() if we receive something that isn't a KEY_EVENT. Otherwise we should return a blank string, i.e. MakeOutput({}), for any key events that aren't handled.

@j4james j4james added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Dec 31, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Dec 31, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Dec 31, 2023
@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase Help Wanted We encourage anyone to jump in on these. Area-VT Virtual Terminal sequence support and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jan 3, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed Needs-Tag-Fix Doesn't match tag requirements labels Jan 3, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants