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.

Sign up for GitHub

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

Jump to bottom

ConPTY does not translate arrow keys with +ENHANCED_KEY in their modifiers #2397

Closed
SummerGift opened this issue Aug 12, 2019 · 11 comments · Fixed by #5021
Closed

ConPTY does not translate arrow keys with +ENHANCED_KEY in their modifiers #2397

SummerGift opened this issue Aug 12, 2019 · 11 comments · Fixed by #5021
Assignees
@carlos-zamora
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
Terminal v1.0

Comments

@SummerGift
Copy link

Today I updated Windows to 1903 and found that a python program I had written earlier ran incorrectly.

The brief test code is as follows:

import msvcrt
ch1 = msvcrt.getch()
ch2 = msvcrt.getch()
print(ch1)
print(ch2)

When I run the above code in native powershell, press ← key when collecting characters, and it will print b'\xe0' B 'K', and the same code running in the terminal returns b'\x00' b'K'.
This problem didn't appear in the last version of Windows (18.xx).

Steps to Reproduce:

  1. Run this code on the powershell terminal of vscode using Python3
  2. press ← key，record print information
  3. Run this code directly from your powershell on Windows using Python3
  4. press ← key，record print information
  5. Look at different outputs

A related issue on vscode：

microsoft/vscode#78682

Thank you for your attention.
@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 Aug 12, 2019
@SummerGift SummerGift mentioned this issue Aug 12, 2019
Closed
@zadjii-msft zadjii-msft added Area-Input Related to input processing (key presses, mouse, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty labels Aug 12, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 12, 2019
@DHowett-MSFT DHowett-MSFT added Help Wanted We encourage anyone to jump in on these. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 12, 2019
@SummerGift
Copy link
Author

Any help?

@zadjii-msft
Copy link
Member

We believe this is a bug, but we're not really sure what the root cause is. Presumably something about how ConPTY builds INPUT_RECORDs compared to how we build them from window input directly.

We're also very confused that ch1 in the above sample is b'\xe0' b'K' - we really didn't expect that. So we just need someone to really debug into this.

@zadjii-msft zadjii-msft added this to the 20H1 milestone Aug 19, 2019
@SummerGift
Copy link
Author

Before the 1903 version of Windows, the return value was b'\xe0' b'K', which became b'\x00' b'K' when upgraded to 1903, causing some programs to fail.

@DHowett-MSFT DHowett-MSFT added the Priority-1 A description (P1) label Sep 5, 2019
@bitcrazed bitcrazed mentioned this issue Sep 9, 2019
Closed
@DHowett-MSFT DHowett-MSFT mentioned this issue Oct 1, 2019
Closed
@DHowett-MSFT
Copy link
Contributor

Eureka!

Under normal circumstances:

  • ↓ wch:0x0000 '\0' mod:EnhancedKey (0x00000100) repeat:0x0001 vk:0x0025 vsc:0x004b

Under ConPTY:

  • ↓ wch:0x0000 '\0' mod:None (0x00000000) repeat:0x0001 vk:0x0025 vsc:0x004b

Critical here is mod:EnhancedKey. ConPTY isn't setting it. I'm not sure how it could know to set it, because we usually derive its value from the Win32 window proc...
ConPTY only sets it if there's a modifier on the arrow key.

The documentation on ENHANCED_KEY says:

Enhanced keys for the IBM® 101- and 102-key keyboards are the INS, DEL, HOME, END, PAGE UP, PAGE DOWN, and direction keys in the clusters to the left of the keypad; and the divide (/) and ENTER keys in the keypad.

@DHowett-MSFT DHowett-MSFT changed the title An error occurred while calling python msvcrt module to retrieve characters from terminal (windows10 1903) ConPTY does not translate arrow keys with +ENHANCED_KEY in their modifiers Oct 1, 2019
@DHowett-MSFT
Copy link
Contributor

And now that we have a root cause, I'm finally renaming this issue.

@DHowett-MSFT DHowett-MSFT added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 1, 2019
@DHowett-MSFT
Copy link
Contributor

Yanking the triage tag after a discussion with @carlos-zamora and @zadjii-msft. Carlos is down to look at this 😄

@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 Oct 1, 2019
@DHowett-MSFT DHowett-MSFT removed the Help Wanted We encourage anyone to jump in on these. label Oct 1, 2019
@zadjii-msft zadjii-msft mentioned this issue Oct 31, 2019
Closed
@wmcbrine
Copy link

wmcbrine commented Nov 3, 2019

This also prevents PDCurses from recognizing the cursor keys, as I just discovered.

@SummerGift
Copy link
Author

This also prevents PDCurses from recognizing the cursor keys, as I just discovered.

So, the program written before runs unnormal.

@cinnamon-msft cinnamon-msft removed this from the 20H1 milestone Nov 14, 2019
@cinnamon-msft cinnamon-msft added this to the 21H1 milestone Nov 14, 2019
@DHowett-MSFT DHowett-MSFT mentioned this issue Nov 18, 2019
Closed
@DHowett-MSFT DHowett-MSFT modified the milestones: 21H1, Terminal v1.0 Nov 18, 2019
@DHowett-MSFT DHowett-MSFT mentioned this issue Dec 8, 2019
Closed
@zadjii-msft zadjii-msft mentioned this issue Jan 8, 2020
Closed
@DHowett-MSFT DHowett-MSFT mentioned this issue Jan 22, 2020
Closed
@cinnamon-msft cinnamon-msft added Priority-2 A description (P2) v1-Scrubbed and removed Priority-1 A description (P1) labels Jan 28, 2020
@DHowett-MSFT DHowett-MSFT mentioned this issue Jan 28, 2020
Closed
@DHowett-MSFT DHowett-MSFT mentioned this issue Feb 10, 2020
Closed
@malxau malxau mentioned this issue Feb 11, 2020
Closed
@carlos-zamora carlos-zamora mentioned this issue Mar 19, 2020
Merged
1 task
@ghost ghost added the In-PR This issue has a related PR label Mar 19, 2020
@ghost ghost closed this as completed in #5021 Mar 24, 2020
@ghost ghost removed the In-PR This issue has a related PR label Mar 24, 2020
ghost pushed a commit that referenced this issue Mar 24, 2020
@carlos-zamora
Add enhanced key support for ConPty (#5021)
403069c 
## Summary of the Pull Request
ConPty did not set the ENHANCED_KEY flag when generating new input. This change helps detect when it's supposed to do so, and sends it.

## References
[Enhanced Key Documentation](https://docs.microsoft.com/en-us/windows/console/key-event-record-str)

## PR Checklist
* [X] Closes #2397

## Detailed Description of the Pull Request / Additional comments
| KEY_EVENT_RECORD modifiers | VT encodable? | Detectable on the way out? |
|----------------------------|---------------|----------------------------|
| CAPSLOCK_ON                | No            | No                         |
| ENHANCED_KEY               | No            | Yes**                      |
| LEFT_ALT_PRESSED           | Yes*          | Yes*                       |
| LEFT_CTRL_PRESSED          | Yes*          | Yes*                       |
| NUMLOCK_ON                 | No            | No                         |
| RIGHT_ALT_PRESSED          | Yes*          | Yes*                       |
| RIGHT_CTRL_PRESSED         | Yes*          | Yes*                       |
| SCROLLLOCK_ON              | No            | No                         |
| SHIFT_PRESSED              | Yes           | Yes                        |
```
* We can detect Alt and Ctrl, but not necessarily which one
** Enhanced Keys are limited to the following:
    - off keypad: INS, DEL, HOME, END, PAGE UP, PAGE DOWN, direction keys
    - on keypad: / and ENTER
   Since we can't detect the keypad keys, those will _not_ send the ENHANCED_KEY modifier.
   For the following CSI action codes, we can assume that they are Enhanced Keys:
     case CsiActionCodes::ArrowUp:
     case CsiActionCodes::ArrowDown:
     case CsiActionCodes::ArrowRight:
     case CsiActionCodes::ArrowLeft:
     case CsiActionCodes::Home:
     case CsiActionCodes::End:
     case CsiActionCodes::CSI_F1:
     case CsiActionCodes::CSI_F3:
     case CsiActionCodes::CSI_F2:
     case CsiActionCodes::CSI_F4:
   These cases are handled in ActionCsiDispatch
```
## Validation Steps Performed
Followed bug repro steps. It now matches!
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Mar 24, 2020
@ghost
Copy link

ghost commented Apr 22, 2020

🎉This issue was addressed in #5021, which has now been successfully released as Windows Terminal Preview v0.11.1121.0.:tada:

Handy links:

@thomthom
Copy link

Thank you! I can finally get rid of cmder now and switch to Terminal full time! 🎉

@DHowett-MSFT
Copy link
Contributor

Great!

This was referenced Jul 28, 2020
Closed
Merged
ghost pushed a commit that referenced this issue Jul 31, 2020
@DHowett
Send ENHANCED_KEY in Win32 input mode in the wpf/uwp controls (#7106)
dd0f7b7 
When we added support for win32 input mode, we neglected to pass
`ENHANCED_KEY` through the two surfaces that would generate events. This
broke arrow keys in much the same way was #2397, but in a different
layer.

While I was working on the WPF control, I took a moment to refactor the
message cracking out into a helper. It's a lot easier on the eyes than
four lines of bit shifting repeated three times.

Fixes #7074
DHowett added a commit that referenced this issue Jul 31, 2020
@DHowett
Send ENHANCED_KEY in Win32 input mode in the wpf/uwp controls (#7106)
4a03c20 
When we added support for win32 input mode, we neglected to pass
`ENHANCED_KEY` through the two surfaces that would generate events. This
broke arrow keys in much the same way was #2397, but in a different
layer.

While I was working on the WPF control, I took a moment to refactor the
message cracking out into a helper. It's a lot easier on the eyes than
four lines of bit shifting repeated three times.

Fixes #7074

(cherry picked from commit dd0f7b7)
DHowett added a commit that referenced this issue Jul 31, 2020
@DHowett
Send ENHANCED_KEY in Win32 input mode in the wpf/uwp controls (#7106)
1394147 
When we added support for win32 input mode, we neglected to pass
`ENHANCED_KEY` through the two surfaces that would generate events. This
broke arrow keys in much the same way was #2397, but in a different
layer.

While I was working on the WPF control, I took a moment to refactor the
message cracking out into a helper. It's a lot easier on the eyes than
four lines of bit shifting repeated three times.

Fixes #7074

(cherry picked from commit dd0f7b7)
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@carlos-zamora carlos-zamora

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
Milestone
Terminal v1.0
Development

Successfully merging a pull request may close this issue.

Add enhanced key support for ConPty microsoft/terminal
7 participants
@thomthom @wmcbrine @carlos-zamora @DHowett-MSFT @zadjii-msft @SummerGift @cinnamon-msft