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

Win32 input make/break events are generated at the same time #8440

Closed
o-sdn-o opened this issue Nov 29, 2020 · 6 comments · Fixed by #15130
Closed

Win32 input make/break events are generated at the same time #8440

o-sdn-o opened this issue Nov 29, 2020 · 6 comments · Fixed by #15130
Labels
Area-Input Related to input processing (key presses, mouse, etc.) In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Milestone

Comments

@o-sdn-o
Copy link

o-sdn-o commented Nov 29, 2020

Environment

Windows build number: 10.0.19041.572
Windows Terminal version (if applicable): master

In the classic windows console, you can track alphanumeric key combinations (for example, tracking the keys w, a, s, d simultaneously). This tracking cannot be done in Windows Terminal.

Steps to reproduce

Run the following code
Press a couple of alphanumeric keys at the same time, e.g. q+w+e

#include <iostream>
#include <unordered_set>
#include <Windows.h>

int main()
{
    DWORD                    Count;
    INPUT_RECORD             Reply;
    std::unordered_set<WORD> State;

    auto Input = GetStdHandle(STD_INPUT_HANDLE);

    while (WAIT_OBJECT_0 == WaitForSingleObject(Input, INFINITE))
    {
        ReadConsoleInput(Input, &Reply, 1, &Count);
        switch (Reply.EventType)
        {
            case KEY_EVENT:
            {
                //std::cout
                //    << " Down: "     << Reply.Event.KeyEvent.bKeyDown
                //    << " Repeat: "   << Reply.Event.KeyEvent.wRepeatCount
                //    << " KeyCode: "  << Reply.Event.KeyEvent.wVirtualKeyCode
                //    << " ScanCode: " << Reply.Event.KeyEvent.wVirtualScanCode
                //    << " Char: "     << Reply.Event.KeyEvent.uChar.UnicodeChar
                //    << " KeyState: " << Reply.Event.KeyEvent.dwControlKeyState
                //    << std::endl << std::flush;

                auto ScanCode = Reply.Event.KeyEvent.wVirtualScanCode;
                auto KeyDown = Reply.Event.KeyEvent.bKeyDown;
                auto Changed = KeyDown ? State.emplace(ScanCode).second
                                       : State.erase(ScanCode);
                if (Changed)
                {
                    std::cout << "chord:";
                    for (auto Key : State) std::cout << " " << Key;
                    std::cout << std::endl << std::flush;
                }
                break;
            }
            default:
                break;
        }
    }
}

Expected behavior

Key combinations are tracked (classic console behavior)

D:\sources\temp\ConsoleApplication1\Debug>ConsoleApplication1.exe
chord: 16
chord: 16 17
chord: 16 17 18
chord: 16 17
chord: 16
chord:
chord: 16
chord: 16 17
chord: 16 17 18
chord: 17 18
chord: 18
chord:

Actual behavior

Single keystrokes are tracked only (Windows Terminal behavior)

D:\sources\temp\ConsoleApplication1\Debug>ConsoleApplication1.exe
chord: 16
chord:
chord: 17
chord:
chord: 18
chord:
chord: 18
chord:
chord: 18
chord:
chord: 18
chord:
chord: 18
chord:
...
@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 Nov 29, 2020
@zadjii-msft
Copy link
Member

Yep, this is unfortunately true. Since the Terminal is using the UWP XAML Input stack, we only get three types of input events: key downs, key ups, and character received events. Since we don't get full unicode character input from just the key down/ups, we have to rely on the character events for that. But there's no character up/down events, only received. So we've got to send a fake key down/up pair immediately. I'm sure if there was a way to avoid doing this, @lhecker would have thought of it by now 😜

For the sake of linking: #4999 dealt with a lot of similar issues. At least conpty is now capable of theoretically handling this, if only the Terminal was.

@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-Terminal The new Windows Terminal. Resolution-Answered Related to questions that have been answered labels Nov 30, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 30, 2020
@lhecker
Copy link
Member

lhecker commented Nov 30, 2020

We could integrate the UWP based WT more deeply with our Win32 entrypoint.
That would allow us to use something like our old approach documented here.

@DHowett DHowett changed the title Unable to track alphanumeric key combinations Win32 input make/break events are generated at the same time Dec 3, 2020
@DHowett DHowett added Issue-Task It's a feature request, but it doesn't really need a major design. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Answered Related to questions that have been answered labels Dec 3, 2020
@DHowett DHowett added this to the Icebox milestone Dec 3, 2020
@DHowett
Copy link
Member

DHowett commented Dec 3, 2020

I've renamed this and iceboxed it. There's some ways in which we'll want to be compatible with the console, and a great many in which we will not.

This one requires enough work (and subversion of the "modern" input stack) that I would bin it into the "not" category, but... that's me.

@j4james
Copy link
Collaborator

j4james commented Sep 1, 2022

I've just run into this issue while trying to get certain VT input modes to work in Windows Terminal, and I'm trying to work out what the actual problem is here. I get that the key down event doesn't include character data, but we've already got code in place that can generate that character data automatically (see the _CharacterFromKeyEvent method). Are there known cases where that doesn't work correctly?

But lets assume the _CharacterFromKeyEvent method isn't good enough. Couldn't we at least use the CharacterReceived handler to generate the key down events (as we're doing now), but then still use the KeyUp handler for the corresponding key up events? There may then be edge cases where the character data could be incorrect for the key up, but would that actually affect anyone? Because a typical WM_KEYUP message wouldn't include that info anyway.

And if that really is still a concern, another approach we could take would be to save the most recent character in the CharacterReceived handler (just store it in an array mapped to the scan code), and then the KeyUp handler could easily look up the corresponding character data for a particular scan code when the key is released. Even with multiple keys pressed simultaneously I would think the scan code should suffice as a unique id.

Are there more factors at play here that I'm overlooking? Or does anyone have suggestions for sample keystrokes I should be testing that would be expected to fail? Because everything I've tried so far seemed to work OK (or at least no worse than the current implementation).

carlos-zamora pushed a commit that referenced this issue Oct 10, 2022
This PR adds support for the `DECARM` (Auto Repeat Mode) sequence, which
controls whether a keypress automatically repeats if you keep it held
down for long enough.

Note that this won't fully work in Windows Terminal until issue #8440 is
resolved.

Every time we receive a `KeyDown` event, we record the virtual key code
to track that as the last key pressed. If we receive a `KeyUp` event the
matches that last key code, we reset that field. Then if the Auto Repeat
Mode is reset, and we receive a `KeyDown` event that matches the last
key code, we simply ignore it.

## Validation Steps Performed

I've manually tested the `DECARM` functionality in Vttest and confirmed
that it's working as expected. Although note that in Windows Terminal
this only applies to non-alphanumeric keys for now (e.g. Tab, BackSpace,
arrow keys, etc.)

I've also added a basic unit test that verifies that repeated key
presses are suppressed when the `DECARM` mode is disabled.

Closes #13919
@DHowett
Copy link
Member

DHowett commented Apr 6, 2023

Are there more factors at play here that I'm overlooking?

Probably there aren't -- I think you've by this point investigated it deeper than the rest of us in recent history. We just got another dupe for it, and I am wondering if there's something we can do about it...

While we're breaking input, and all. 😁

@j4james
Copy link
Collaborator

j4james commented Apr 6, 2023

I've got an old branch with my initial simple proposal (relying on _CharacterFromKeyEvent to generate the character data), which I can turn into a PR tomorrow if you'd like. It's only a couple of lines, so it's not a big deal if we need to drop it in favor of something more complicated.

@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Apr 6, 2023
DHowett pushed a commit that referenced this issue Apr 11, 2023
When win32-input-mode is enabled, we generate an input sequence for both
key down and key up events. However, in the initial implementation the
key up sequence for most keypresses would be faked - they were generated
at the same time as the key down sequence, regardless of when the key
was actually released. After this PR, we'll only generate the key up
sequence once a key has actually been released.

## References and Relevant Issues

The initial implementation of win32-input-mode was in PR #6309.
The spec for win32-input-mode was in PR #5887.

## Validation Steps Performed

I've manually tested this with an app that polls `ReadConsoleInput` in a
loop and logs the results. With this PR applied, I can now see the key
up events as a key is released, rather than when it was first pressed.

When compared with conhost, though, there are some differences. When
typing a shifted key, e.g. `Shift`+`A`, WT generates key down and key up
events for both the `Shift` and the `A`, while conhost only generates
both events for the `Shift` - the `A` won't generate a key up event
unless you release the `Shift` before the `A`. That seems more like a
conhost flaw though.

Another case I tested was the Japanese Microsoft IME, which in conhost
will generate a key down event for the Japanese character being inserted
followed by a key up event for for `Return`. WT generates key up events
for the ASCII characters being typed in the IME, then both a key down
and key up event for the inserted Japanese character, and finally a key
up event for `Return`. Both of those seem weird, but they still appear
to work OK. 

The current version of WT actually produces the most sensible behavior
for the IME - it just generates key up and key down events for the
inserted character. But that's only because it's dropping most of the
system generated key up events.

Closes #8440
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Apr 11, 2023
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.) In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants