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

Shift key not working properly in some situations #2771

Closed
iCodeSometime opened this issue Sep 16, 2019 · 6 comments · Fixed by #2836
Closed

Shift key not working properly in some situations #2771

iCodeSometime opened this issue Sep 16, 2019 · 6 comments · Fixed by #2836
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@iCodeSometime
Copy link

iCodeSometime commented Sep 16, 2019

Environment

Windows build number: 10.0.18362.0
Windows Terminal version (if applicable): 0.4.2382.0

Any other software? wsl + emacs

Steps to reproduce

  1. Open emacs within wsl in Windows Terminal.
  2. Type either of the following:
    • M-> (alt + greater than) to move to the end of the file
    • M-< (alt + less than) to move to the beginning of the file
  3. Verify the key emacs is seeing, by typing C-h k (Ctrl+h, k) for describe key - then type either of the above key combinations.

Expected behavior

M-> moves to the end of the file.
M-< moves to the beginning of the file.
C-h k M-> displays M-> as the command run.
C-h k M-< displays M-< as the command run.

Actual behavior

M-> does not move to the end of the file.
M-< does not move to the beginning of the file.
C-h k M-> displays M-. as the command run.
C-h k M-< displays M-, as the command run.

@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 Sep 16, 2019
@iCodeSometime
Copy link
Author

To be clear, this all works properly when I use my wsl instance from outside of the new windows terminal app.

@zadjii-msft zadjii-msft added Area-Input Related to input processing (key presses, mouse, etc.) Area-VT Virtual Terminal sequence support Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty labels Sep 16, 2019
@zadjii-msft zadjii-msft self-assigned this Sep 16, 2019
@zadjii-msft zadjii-msft added this to the 20H1 milestone Sep 16, 2019
@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 16, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 16, 2019
@zadjii-msft zadjii-msft removed their assignment Sep 19, 2019
@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. and removed Product-Conpty For console issues specifically related to conpty labels Sep 19, 2019
@zadjii-msft zadjii-msft modified the milestones: 20H1, Terminal v1.0 Sep 19, 2019
@zadjii-msft
Copy link
Member

I'm moving this to the Terminal 1.0 milestone because it seems this only repros in the Windows Terminal. It doesn't repro if you use WSL to launch cmd.exe, then launch wsl emacs. That set of steps would activate conpty. Since it doesn't repro with conhost acting as the terminal, we can rule our conpty as the source of the bug.

It'll still get fixed, but we don't need to worry about getting it into the OS for 20H1.

IIRC @lhecker did some work in this area previously.

@zadjii-msft zadjii-msft added the Help Wanted We encourage anyone to jump in on these. label Sep 19, 2019
@lhecker
Copy link
Member

lhecker commented Sep 19, 2019

@zadjii-msft Yeah this problem is basically tangential to the AltGr issues.
We have the vkey and modifier states in the UWP code readily available, but when we map it for the old NT console subsystem we throw away the modifier states here:

ch = static_cast<wchar_t>(LOWORD(MapVirtualKey(vkey, MAPVK_VK_TO_CHAR)));

This causes the logic here to malfunction where the assumption is made that the char data is already pretranslated:
_SendEscapedInputSequence(keyEvent.GetCharData());

Due to this I added the circumvention logic for AltGr in the UWP side of things. I guess it's time to find a proper solution for this?

@lhecker
Copy link
Member

lhecker commented Sep 19, 2019

I read up on this a bit and it appears to me that we can map the vkey to the pretranslated character using a combination of MapVirtualKey(vkey, MAPVK_VK_TO_VSC) with ToUnicode(...).
Afterwards we should be able to remove the special handling for AltGr.

(I still don't quite understand the full architecture, but I keep wondering for what technical reason the terminalInput class is being used. If it's just for code sharing purposes, we should probably split the input handling up, so we don't have to rely on the above.)

@DHowett-MSFT
Copy link
Contributor

Fixed in #2836.

@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. labels Oct 1, 2019
@ghost
Copy link

ghost commented Oct 4, 2019

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

Handy links:

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 Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal. 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