Fix windows glutin keydown

[jonathandturner]

This fixes #12937 where keypresses arrive in a one-off fashion, effectively delaying each keystroke. This PR is based heavily on @codec-abc's #16193 PR, with some further fixes and cleanup. From the original PR comments:

There are 2 types of events associated with keyboard: Key event (up or down) and Character Input.

• A character is not necessary created one a key is pressed (like the home key).
• A character may be created only using a combination of several key. For example, using a Qwerty International layout you have to type then e to get é. The first key press on will create no character.
• In servo, we currently merge the 2 events together, meaning that we store a Character Input event in a field to fire a single event later on when we get a Key event.

The order of events seems to be system dependent. For example, let's say that we type the A key on a Qwerty Layout. In Linux and MacOs we will get this:

• Character Input for character A
• Key down event with virtual key code of the key A
• Key up event with virtual key code of the key A

while in Windows we get:

• Key down event with virtual key code of the key A
• Character Input for character A
• Key up event with virtual key code of the key A

It seems that glutin make no attempt to reorder the event to make the order independent of the Operating System. I think it would be easier for Servo if it were handled by the library.

This fix is a stopgap for the current state of glutin. We may want to look into a deeper fix in the future. For now, this is hopefully adequate solution until a more permanent one can be found.

This is the last remaining issue for the Windows nightly blockers meta-issue #12125

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix #12937 (github issue number if applicable).

• There are tests for these changes OR
• These changes do not require tests because they are around Windows keyboard eventing.

---

This change is 

[codec-abc]

Tested it quickly. I cannot type some characters like / or : for instance but backspace work as expected.

[jonathandturner]

@codec-abc - good catch. Looking into it now

[jonathandturner]

Looks like the VirtualKeyCode isn't getting passed correctly when hitting /. It's coming back as None, assuming what I'm seeing in the debugger is correct. This is causing it to take the path that says "Keyboard input without virtual key", which it shouldn't take.

[jonathandturner]

Keys like '/' don't show up in the translation that Servo is using: https://github.com/servo/glutin/blob/servo/src/api/win32/event.rs

This means they don't get a VirtualKeyCode, and then we don't print them, since we expect to have one we can use to translate to a Key that we later use in ReceivedCharacter.

For US Keyboards, the / key is event VK_OEM_2 (see: https://msdn.microsoft.com/en-us/library/windows/desktop/dd375731(v=vs.85).aspx). We don't currently translate this one to a VirtualKeyCode. That code is currently commented out. I think this makes sense, at least somewhat, because using this is specific to US keyboards.

If we want this approach to work, we may end up needing to have keyboard specific things we detect, which seems bad.

Another solution might be to let ReceivedCharacter handle everything, and on the case where it didn't have a Key from before, then check if the character it receives is printable itself.

[jonathandturner]

I'm not entirely sure this is the direction we want to go, but this works for the tests I tried, including fixing the issue @codec-abc pointed out.

That said, because of the changing assumptions about how keyboard events work, it'd be good to talk through how we want things to work in the future as well.

[jonathandturner]

Btw - not a fan of the dummy arguments I used. This was just to get things working. I also wasn't sure how deeply we wanted this change to go. I was shooting for a "get it working" PR that we could follow up with a tracking issue for cleanup once we knew how we wanted it to work.

[codec-abc]

I agree with you on everything except for the commenting out of some keys in glutin. From my point of view, it should forward every key event that come and the user may choose to use them or not. As this code start to be tricky I think someone should test it in Linux or Mac before merging to be sure it won't break the keyboard handling on those platforms.

By the way, funny typo in the title 😄

[jonathandturner]

@codec-abc - yeah that commented out section was there before. I agree it feels a bit weird. I think they're trying to abstract over the user's keymap.

[jonathandturner]

Pinging @jdm (who I'm not sure has seen the PR yet)

[jdm]

First question - is it possible to encapsulate this per-platform logic in a separate structure? Something like:

#[cfg(target_os = "windows")]
type KeyBuffer = windows::KeyBuffer;

mod windows {
    struct KeyBuffer {
        last_pressed_key: Cell<Option<constellation_msg::Key>>,
    }
    impl KeyBuffer {
        fn process_character_event(...) -> Option<WindowEvent> {
            ...
        }
        fn process_keyboard_event(...) -> Option<WindowEvent> {
            ...
        }
    }
}

This would make it easier for me to reason about the per-platform logic.

[jdm]

I am confused by the addition of this line. Is this intentional?

[codec-abc]

Yes, it is to align the behavior between platform. In Windows, we get the KeyboardInput event before the ReceivedCharacter which lead to a KeyEvent being fired with no character until we get the associated ReceivedCharacter (if any). In the other OSs the order is reversed. With that line, we reorder the events on non Windows platforms. Said differently, you can see there is a KeyEvent fired in both ReceivedCharacter and KeyboardInput branches in Windows, while no event are fired in the ReceivedCharacter branch for the other platforms, which force us to fire 2 event in the KeyboardInput branch to get the same behavior.

[jonathandturner]

@jdm - I did a bit of refactoring to hopefully make the code a bit more readable

[jdm]

Thanks for tackling this! As discussed on IRC, I think we can address this in glutin with much less effort, and the result will be easier to reason about: http://logs.glob.uno/?c=mozilla%23servo#c648119

[jonathandturner]

@jdm - Poked around with the PeekMessage idea a bit. Unfortunately, I'm starting to wonder if we want to go this direction.

Let's say the user hits two keys immediately after each other, like shift followed by 'a'. The shift even will check PeekMessage, and may see the WM_CHAR when it looks (let's say there are a few keyboard events waiting), so it decides to delay sending out the keydown event. The result would be that we get a CHAR for 'a' and a keydown for 'a', which is not what we want.

There seemed like there were other race possibilities, that was just the first that came to mind.

Even barring that, which I'm not sure we can easily work around, I can't see PeekMessage working as expected with my naive setup. There may be other steps we need to do before we can see the WM_CHAR events at all.

[codec-abc]

I will try to find some time to make a sample on Windows & Mac with SDL2 nad GLFW to see if the events are in the same count and order. With any luck, a library will get it right, so we can see how it handle it under the hood.

EDIT : I am not able to run a SDL2 sample on a Linux VM because the library complain that it cannot find a video device which is a bit unfortunate. I will try with GLFW next.

[jdm]

Remove this.

[jdm]

!ch.is_control()

[jdm]

Duplicate https://dxr.mozilla.org/servo/rev/065f50014f321466c979120967da720e88ae2b29/components/script/dom/keyboardevent.rs#344-438 and ignore the stuff about key location.

[jdm]

It's not just good; it's good enough! Squash the commits and we can merge them.
@bors-servo: delegate+

[bors-servo]

✌️ @jonathandturner can now approve this pull request

[jonathandturner]

Squashed

@bors-servo r=jdm

[bors-servo]

📌 Commit c9a091a has been approved by jdm

[bors-servo]

⌛ Testing commit c9a091a with merge 4c85192...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: jdm
Pushing 4c85192 to master...

[codec-abc]

@jdm @jonathandturner Can one of you update me on the current status of the keyboard handling? to be honest, I am a little bit lost. Moreover, If I recall correctly Servo's behavior on Windows on this test page was incorrect. I can't test it right now so maybe the result is different with the lasts commits. Anyway are there some things to fix/change related to the keyboard events handling and is it needed to open an issue?

[jonathandturner]

@codec-abc - what we ended up at was making Windows keyboard events work for basic things, but we don't have them event-accurate with what is expected on that test page. We took out the double events on non-Windows platform.

Ultimately, we want a different solution, but this should suffice for the time-being.

[jonathandturner]

@jdm - not entirely sure how, but it seems like I managed to squash the wrong patch set (switching back and forth between VMs might have done it)

Working on updated PR now.