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

Ctrl+Alt+Shift+2 doesn't send NUL in VT input mode #5205

Closed
zadjii-msft opened this issue Apr 1, 2020 · 11 comments · Fixed by #5272
Closed

Ctrl+Alt+Shift+2 doesn't send NUL in VT input mode #5205

zadjii-msft opened this issue Apr 1, 2020 · 11 comments · Fixed by #5272
Assignees
Labels
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-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@zadjii-msft
Copy link
Member

zadjii-msft commented Apr 1, 2020

@zadjii-msft:

Okay so Ctrl+Alt+Space was fixed for conhost, but it looks like it's still broken in the Terminal 😢 Now it's just generating a ^@, a single NUL.

I think the Terminal is synthesizing the right sequence, but maybe conpty is generating the wrong input for it now, or maybe the input that's generated by conpty doesn't get re-translated back to ^[^@ correctly. I can try to keep investigating to figure out where the miscommunication is, if you need help.

@lhecker:

@zadjii-msft Yeah I was just about to say that as well.

As I wrote above the most puzzling thing for me though is if you change this if condition (line 510) to a simple ch != 0 - which should be correct as well.
If you attach a debugger you'll see that TerminalInput definitely invokes _pfnWriteEvents with a \x1b\x00 sequence.
But then showkey shows a ^[^R sequence instead. How can this happen? > I don't understand how such a benign change to the if condition can cause this. 😞

Okay I maybe saw what happened here.

  1. On a en-us keyboard, '@' is shift+2.
  2. When conpty sees a \x1b\x0, it converts it to [Shift_down, Ctrl_down, Alt_down, 2_down, 2_up, Alt_up, Ctrl_up, Shift_up].
    • I did not pay a ton of attention to the keys that got synthesized here. I need to re-investigate the [2_down, 2_up]
  3. When we get back to TerminalInput::HandleKey, we end up skipping that branch, because the ch was 0n50, which is 0x32, which is '2'.

I'm not sure what's exactly the right behavior here. I'd think we probably should send a different key, but I'll need to investigate more. It's possible that the ch != 0 change would work just fine, but I want to make sure that the upstream input from conpty is actually correct.
I've got a braindead fix that would fix showkey -a, but that doesn't really resolve the issue here for Win32 applications unfortunately, so I'm gonna file a separate issue to track this investigation.

Originally posted by @zadjii-msft in #4192 (comment)

@ghost ghost 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 Apr 1, 2020
@zadjii-msft zadjii-msft added Area-Input Related to input processing (key presses, mouse, etc.) Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty Issue-Bug It either shouldn't be doing this or needs an investigation. labels Apr 1, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Apr 1, 2020
@zadjii-msft zadjii-msft self-assigned this Apr 1, 2020
@zadjii-msft
Copy link
Member Author

zadjii-msft commented Apr 1, 2020

master, Conhost+conpty, Ctrl+alt+space, (i/o modes):(0x 1f7, 0x f) (Win32 input)

Down: 1 Repeat: 1 KeyCode: 0x10 ScanCode: 0x2a Char: \0 (0x0) KeyState: 0x10
Down: 1 Repeat: 1 KeyCode: 0x12 ScanCode: 0x38 Char: \0 (0x0) KeyState: 0x12
Down: 1 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x1a
Down: 1 Repeat: 1 KeyCode: 0x32 ScanCode: 0x3 Char: \0 (0x0) KeyState: 0x1a
Down: 0 Repeat: 1 KeyCode: 0x32 ScanCode: 0x3 Char: \0 (0x0) KeyState: 0x1a
Down: 0 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x12
Down: 0 Repeat: 1 KeyCode: 0x12 ScanCode: 0x38 Char: \0 (0x0) KeyState: 0x10
Down: 0 Repeat: 1 KeyCode: 0x10 ScanCode: 0x2a Char: \0 (0x0) KeyState: 0x0

The interesting input here is Down: 1 Repeat: 1 KeyCode: 0x32 ScanCode: 0x3 Char: \0 (0x0) KeyState: 0x1a

master, Conhost+conpty, Ctrl+alt+space, (i/o modes):(0x 3f7, 0x f) (VT input)

Down: 1 Repeat: 1 KeyCode: 0x10 ScanCode: 0x2a Char: \0 (0x0) KeyState: 0x10
Down: 1 Repeat: 1 KeyCode: 0x12 ScanCode: 0x38 Char: \0 (0x0) KeyState: 0x12
Down: 1 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x1a
Down: 1 Repeat: 1 KeyCode: 0x32 ScanCode: 0x0 Char: \0 (0x0) KeyState: 0x1a
Down: 0 Repeat: 1 KeyCode: 0x32 ScanCode: 0x3 Char: \0 (0x0) KeyState: 0x1a
Down: 0 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x12
Down: 0 Repeat: 1 KeyCode: 0x12 ScanCode: 0x38 Char: \0 (0x0) KeyState: 0x10
Down: 0 Repeat: 1 KeyCode: 0x10 ScanCode: 0x2a Char: \0 (0x0) KeyState: 0x0

master, Conhost, Ctrl+alt+space, (i/o modes):(0x 1f7, 0x f) (Win32 input)

Down: 1 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x28
Down: 1 Repeat: 1 KeyCode: 0x12 ScanCode: 0x38 Char: \0 (0x0) KeyState: 0x2a
Down: 1 Repeat: 1 KeyCode: 0x20 ScanCode: 0x39 Char: \0 (0x0) KeyState: 0x2a
Down: 0 Repeat: 1 KeyCode: 0x20 ScanCode: 0x39 Char: \0 (0x0) KeyState: 0x2a
Down: 0 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x22
Down: 0 Repeat: 1 KeyCode: 0x12 ScanCode: 0x38 Char: \0 (0x0) KeyState: 0x20

master, Conhost, Ctrl+alt+space, (i/o modes):(0x 3f7, 0x f) (VT input)

Down: 1 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x28
Down: 1 Repeat: 1 KeyCode: 0x12 ScanCode: 0x38 Char: \0 (0x0) KeyState: 0x2a
Down: 1 Repeat: 1 KeyCode: 0x0 ScanCode: 0x0 Char: ^[ (0x1b) KeyState: 0x0
Down: 1 Repeat: 1 KeyCode: 0x0 ScanCode: 0x0 Char: \0 (0x0) KeyState: 0x0
Down: 0 Repeat: 1 KeyCode: 0x20 ScanCode: 0x39 Char: \0 (0x0) KeyState: 0x2a
Down: 0 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x22
Down: 0 Repeat: 1 KeyCode: 0x12 ScanCode: 0x38 Char: \0 (0x0) KeyState: 0x20

master, Conhost, Ctrl+alt+shift+2, (i/o modes):(0x 1f7, 0x f) (Win32 input)

Down: 1 Repeat: 1 KeyCode: 0x10 ScanCode: 0x2a Char: \0 (0x0) KeyState: 0x30
Down: 1 Repeat: 1 KeyCode: 0x11 ScanCode: 0x1d Char: \0 (0x0) KeyState: 0x38
Down: 1 Repeat: 1 KeyCode: 0x12 ScanCode: 0x38 Char: \0 (0x0) KeyState: 0x3a
Down: 1 Repeat: 1 KeyCode: 0x32 ScanCode: 0x3 Char: \0 (0x0) KeyState: 0x3a
Down: 0 Repeat: 1 KeyCode: 0x32 ScanCode: 0x3 Char: \0 (0x0) KeyState: 0x3a
Down: 0 Repeat: 1 KeyCode: 0x10 ScanCode: 0x2a Char: \0 (0x0) KeyState: 0x20

NOTE: this keystroke is what conpty is attempting to replicate

@zadjii-msft
Copy link
Member Author

Okay I see what the problem is - I think it is the think @lhecker called out in #4192. Even in straight-up conhost, Ctrl+alt+shift+2 does not generate \x1b\x0.

IMO, @lhecker if you want to do the fix proposed in #4192, the ch != 0 fix, then I say go for it, and add this to the growing list of bugs that PR fixes 😆

I'll submit the other thing I found during the course of this investigation, but that will effectively hide the showkey -a repro case in the Terminal.

@zadjii-msft zadjii-msft changed the title Investigate Ctrl+Alt+Space in conpty Ctrl+Alt+Shift+2 doesn't send NUL in VT input mode Apr 1, 2020
@lhecker
Copy link
Member

lhecker commented Apr 1, 2020

@zadjii-msft I believe we're misunderstanding each other.

If I press Ctrl+Alt+Space or Ctrl+Alt+Shift+2, showkey -a will show the following output:

Ctrl+Alt condition showkey -a output
ch >= 0x40 && ch < 0x7F ^@
ch != 0 ^[^R

But as far as I can see it should be ^[^@, right?
Or am I misunderstanding you and ^[^R is actually correct? (...because, as you mentioned above, there's another bug involved here, which distorts the showkey output?)

That said, would you'd like me to modify #4192 to use a ch != 0 condition?
I can't decide by myself if I should. 😄

@zadjii-msft
Copy link
Member Author

Oh jeez, you should be seeing ^[^@. Now I'm not sure what the right fix is.

@lhecker
Copy link
Member

lhecker commented Apr 1, 2020

I figured it out. I didn't realize I had to manually attach myself to OpenConsole.exe for debugging. 🙈
You basically already said all the right things (especially steps 2 and 3 above), but I didn't understand you, because I've been lacking the knowledge how all the code works together.

Anyways... I fixed the problem. (lol)
Turns out if you use MapVirtualKeyW you're probably doing something wrong. I guess? 😄

I replaced this:

auto wchPressedChar = gsl::narrow_cast<wchar_t>(MapVirtualKeyW(keyEvent.GetVirtualKeyCode(), MAPVK_VK_TO_CHAR));

with a call to ToUnicodeEx, similar to Terminal::_CharacterFromKeyEvent.
This call allows us to reliably turn the input event Alt+Ctrl+Shift+2 into an Alt+Ctrl+@ event, which then gets translated to Alt+0x00 and sent away. 🙂

Would you like me to add it to #4192 or open a separate PR afterwards?
I believe it makes sense to fix it separately.

@zadjii-msft
Copy link
Member Author

I didn't realize I had to manually attach myself to OpenConsole.exe for debugging.

Literally my least favorite thing about using VS. JUST ATTACH TO CHILD PROCESSES. Or at least give me an option to auto-attach, sheesh.

Turns out if you use MapVirtualKeyW you're probably doing something wrong

You're sure right about that 😄 Glad you got that figured out!

Would you like me to add it to #4192 or open a separate PR afterwards?

Let's open another PR, since this has gotten bigger than a oneline fix ☺️

@zadjii-msft zadjii-msft assigned lhecker and unassigned zadjii-msft Apr 1, 2020
@lhecker
Copy link
Member

lhecker commented Apr 1, 2020

@zadjii-msft
Copy link
Member Author

MY HERO

@DHowett-MSFT
Copy link
Contributor

Time to e-mail the entire team this finding

ghost pushed a commit that referenced this issue Apr 1, 2020
…e's a problem in Win32 quite yet. (#5208)

## Summary of the Pull Request

When conpty is in VT input mode, we pass through all the input we receive. This includes all the other `Action*Dispatch` methods, but missed this one.

## References
* Missed during #4856 
* Discovered during the course of the #4192 review
* #5205 Also investigated part of the issue, but found a different bug.

## PR Checklist
* [x] Doesn't close anything, just related to above things.
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments
This will fix the <kbd>ctrl+alt+space</kbd> in the Terminal thing mentioned in #4192, but doesn't actually resolve the root cause of that bug (which is tracked in #5205).
@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.x milestone Apr 3, 2020
@DHowett-MSFT
Copy link
Contributor

Milestoning this for 1.x and taking off Triage

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Apr 3, 2020
@ghost ghost added the In-PR This issue has a related PR label Apr 7, 2020
@lhecker lhecker assigned lhecker and unassigned lhecker Aug 11, 2020
@cinnamon-msft cinnamon-msft removed this from the Terminal v1.x milestone Sep 29, 2020
@cinnamon-msft cinnamon-msft added this to the Terminal v2.0 milestone Sep 29, 2020
@ghost ghost closed this as completed in #5272 Feb 8, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Feb 8, 2021
ghost pushed a commit that referenced this issue Feb 8, 2021
## Summary of the Pull Request

Fixes #5205, by replacing another use of `MapVirtualKeyW` with `ToUnicodeEx`.
The latter just seems to be much more consistent at translating key combinations in general.
In this particular case though it fixes the issue, because there's no differentiation in `MapVirtualKeyW` for whether it failed to return a character (`'\0'`) or succeeded in turning `^@` into `'\0'`.
`ToUnicodeEx` on the other hand returns the success state separately from the translated character.

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #5205
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [ ] Requires documentation to be updated
* [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #5205

## Detailed Description of the Pull Request / Additional comments

This PR changes the behavior of the `Ctrl+Alt+Key` handling slightly:
⚠️ `ToUnicodeEx` returns unshifted characters. ⚠️
For instance `Ctrl+Alt+a` is now turned into `^[^a`. Due to how ASCII works this is essentially the same though because `'A' & 0b11111` and `'a' & 0b11111` are the same.

## Validation Steps Performed

* Run `showkey -a`
* Ensured `Ctrl+Alt+Space` as well as `Ctrl+Alt+Shift+2` are turned into `^[^@`
* Ensured other, random `Ctrl+Alt+Key` combination behave identical to the current master
DHowett added a commit that referenced this issue Feb 11, 2021
Dustin L. Howett (3)
* Move CharToKeyEvents (and friends) into InteractivityBase (GH-9106)
* Update Cascadia Code to 2102.03 (GH-9088)
* verison: bump to 1.7 on main

Josh Soref (1)
* ci: update to Spell check to 0.0.17a (CC-9014)

Leonard Hecker (3)
* Fixed GH-5205: Ctrl+Alt+2 doesn't send ^[^@ (CC-5272)
* Fix issues in tests.xml and OpenConsole.psm1 (CC-9011)
* Fix GH-8458: Handle all Ctrl-key combinations (CC-8870)

Mike Griese (1)
* Add support for running a commandline in another WT window (GH-8898)

Michael Niksa (1)
* Teach the renderer to keep thread alive if engine requests it (GH-9091)

Lachlan Picking (1)
* Fix shader time input (CC-8994)

PankajBhojwani (1)
* Separate runtime TerminalSettings from profile-TerminalSettings (CC-8602)

Chester Liu (2)
* Add support for paste filtering and bracketed paste mode (CC-9034)
* Add support for chaining OSC 10-12 (CC-8999)

Related work items: MSFT-31692939
@ghost
Copy link

ghost commented Mar 1, 2021

🎉This issue was addressed in #5272, which has now been successfully released as Windows Terminal Preview v1.7.572.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants