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

Incorrect DECRQM for Win32InputMode #17737

Open
Tracked by #17643
j4james opened this issue Aug 18, 2024 · 3 comments
Open
Tracked by #17643

Incorrect DECRQM for Win32InputMode #17737

j4james opened this issue Aug 18, 2024 · 3 comments
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty

Comments

@j4james
Copy link
Collaborator

j4james commented Aug 18, 2024

Windows Terminal version

Commit 9b21b78

Windows build number

10.0.19045.4780

Other Software

No response

Steps to reproduce

  1. Checkout and build a recent commit of Windows Terminal (I'm assuming it needs to be 450eec4 or later).
  2. Open a tab with a WSL bash shell.
  3. Toggle Win32 input mode with printf "\e[?9001h"; sleep 1; printf "\e[?9001l"; read
  4. You'll likely see the keyup event for the Enter key that you just pressed.
  5. Wait a second and press Enter again to return to the prompt.
  6. Execute showkey -a
  7. Try pressing Esc or Alt+[.

Expected Behavior

You should see the VT codes for those keys like this:

^[       27 0033 0x1b
^[[      27 0033 0x1b
         91 0133 0x5b

Actual Behavior

Neither of those keys are picked up. And I think the problem is the heuristic we're using here:

const auto win32 = _engine->EncounteredWin32InputModeSequence();
if (!win32 && run.size() <= 2 && run.front() == L'\x1b')

That works fine if win32 input mode is always on, or always off, but when it's toggled the _encounteredWin32InputModeSequence flag is never reset, so that branch is always skipped.

I don't think this was an issue prior to the new VT passthrough because we never forwarded win32 input mode changes to conpty, so it should have been permanently enabled (assuming it was supported at all).

@j4james j4james added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 18, 2024
@j4james
Copy link
Collaborator Author

j4james commented Aug 18, 2024

The more I think about this, the more I believe there may be a bigger problem with win32 input mode in general.

Prior to the new passthrough, the terminal would have appeared as if win32 input mode was not enabled. DECRQM would report the mode as reset, and ENABLE_VIRTUAL_TERMINAL_INPUT would generate standard VT input sequences. It was just the internal communication between conhost and Windows Terminal that had win32 input mode enabled.

With the new system, DECRQM now reports the mode as set by default, but doesn't actually behave that way. If you request ENABLE_VIRTUAL_TERMINAL_INPUT you're still going to receive standard VT input sequences, unless you explicitly enable win32 input mode yourself.

And apps that do enable win32 input mode will typically disable it when they exit. That's probably OK for WSL apps (assuming we fix the bug reported above), but if you're in a win32 shell, that's going to leave the system in a state where it doesn't report all the key combinations, which is not ideal.

But it's even worse if the app tries to restore the mode to what it detected at startup, because in that case it'll exit with win32 input mode enabled, which will make a WSL shell unusable. And while it may be a good thing for win32 console apps that read INPUT_RECORD events, it's going to break any apps that are using ENABLE_VIRTUAL_TERMINAL_INPUT, and which are expecting standard VT input sequences.

@zadjii-msft zadjii-msft added Area-Input Related to input processing (key presses, mouse, etc.) Area-VT Virtual Terminal sequence support labels Aug 20, 2024
@zadjii-msft zadjii-msft added this to the Terminal v1.22 milestone Aug 20, 2024
@lhecker
Copy link
Member

lhecker commented Aug 21, 2024

I believe what's missing was a call to InjectSequence in the W32IM_Win32InputMode handler. The large comment next to it even calls out how we shouldn't bubble up the sequence to the parent terminal.

@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Aug 21, 2024
@DHowett
Copy link
Member

DHowett commented Aug 21, 2024

(#17757 may not help DECRQM)

@lhecker lhecker removed the In-PR This issue has a related PR label Aug 21, 2024
lhecker added a commit that referenced this issue Aug 21, 2024
Does what it says on the tin.

Part of #17737

## Validation Steps Performed
* In WSL run
  `printf "\e[?9001h"; sleep 1; printf "\e[?9001l"; read`
* Wait 1s and press Enter
* Run `showkey -a`
* Esc works ✅
@carlos-zamora carlos-zamora added Product-Terminal The new Windows Terminal. Product-Conpty For console issues specifically related to conpty and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Product-Terminal The new Windows Terminal. labels Aug 21, 2024
@carlos-zamora carlos-zamora changed the title Toggling win32 input mode breaks the Esc key Incorrect DECRQM for Win32InputMode Aug 21, 2024
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.) Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

No branches or pull requests

5 participants