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

Text input rendering previous keypress on Windows #12937

Closed
elahn opened this issue Aug 19, 2016 · 28 comments
Closed

Text input rendering previous keypress on Windows #12937

elahn opened this issue Aug 19, 2016 · 28 comments

Comments

@elahn
Copy link

@elahn elahn commented Aug 19, 2016

In this video, I clicked on the input, typed: mozilla.org[enter][enter]. Nothing appeared in the input until the 2nd key, 'o' was pressed and with each keypress, the state as of the previous keypress was rendered.

Windows 10 Home x64 with anniversary update on real hardware.

@jonathandturner
Copy link

@jonathandturner jonathandturner commented Mar 22, 2017

In case anyone is trying to work on this issue, I got this far:

  • components/script/textinput.rs has handle_keydown, which receives a event: &KeyboardEvent
  • When you press a key, this event seems to have the correct keycode field, but the printable from the previous key (initially this is none).
  • handle_keydown uses the printable field to determine what to do, so we end up always being one behind
@jonathandturner
Copy link

@jonathandturner jonathandturner commented Mar 22, 2017

A bit more info:

  • Issue seems to be in glutin. In ports/glutin/window.rs it assumes that Event::ReceivedCharacter will happen before Event::KeyboardInput.
  • This ordering is not what I see in practice, where KeyboardInput comes first.
  • The assumption happens with the self.pending_key_event_char not being set properly

After this happens, we're always one behind.

The fix seems to be to remove this assumption or ensure that glutin orders in the same way it assumes the events will come.

@jonathandturner
Copy link

@jonathandturner jonathandturner commented Mar 22, 2017

cc @tomaka, who might know why the keyboard events work like this ^

@jdm
Copy link
Member

@jdm jdm commented Mar 23, 2017

I believe the ReceivedCharacter/KeyboardInput ordering is correct in Glutin on Mac and Linux.

@codec-abc
Copy link

@codec-abc codec-abc commented Mar 23, 2017

I start to look at this issue too and put some println! ports/glutin/window.rs and found the following : the events in the functions handle_window_event are received in this order :

  • Event::KeyboardInput (probably key down)
  • Event::ReceivedCharacter
  • Event::KeyboardInput (probably key up)

The events appear to be reliably ordered.

But I don't received Event::KeyboardInput for some keys but only Event::ReceivedCharacte like / for example. I am using a azerty layout on a laptop which may make things more complicated. Since the code in the ports/glutin/window.rs seems to be common for Windows, Linux and Mac, can someone please tell if they encounter odd keyboard behavior in the others platform ?

@jonathandturner
Copy link

@jonathandturner jonathandturner commented Mar 23, 2017

@codec-abc - that's the order I was seeing too. Unfortunately, it looks like KeyboardInput checks for the character that ReceivedCharacter puts in pending_key_event_char. This means that we can't use it to type.

You could imagine using it for something like sending the character during a key up, but that'd different than people would expect.

@jonathandturner
Copy link

@jonathandturner jonathandturner commented Mar 23, 2017

It seems like we need translate the key to a printable character ourselves, rather than waiting for ReceivedCharacter. Then we could just ignore the ReceivedCharacter event.

TBH that these are separate events are confusing to me. Why not just have a single keydown and a single keyup? Having other states in the state machine just sounds more complicated.

@jdm
Copy link
Member

@jdm jdm commented Mar 23, 2017

My understanding is that it's common for UI frameworks to include separate events for "printable text you might care about" and "raw keyboard input". For example, GLFW does this too.

@jonathandturner
Copy link

@jonathandturner jonathandturner commented Mar 23, 2017

@jdm - yeah, I think that's where glutin gets it (since iirc, it's based on GLFW).

I did a quick look around, and Windows, SDL, macOS, qt, JS, and gtk all seem to be keydown/keyup with some also having a separate event for key holding. Granted, that was all of 5 minutes of me skimming documentation :).

Inside the key down event, things like JS allow you to get the printable char in case you want it.

@jonathandturner
Copy link

@jonathandturner jonathandturner commented Mar 23, 2017

Hit send too quickly. Anywho, my point with the last line is that this would actually make the fix pretty trivial if glutin worked that way instead of having a separate event for printable char.

@jdm
Copy link
Member

@jdm jdm commented Mar 23, 2017

#4144 has the background on us figuring this stuff out and then never following through on modifying glutin in ways that made sense.

@codec-abc
Copy link

@codec-abc codec-abc commented Mar 24, 2017

I have tried to understand the issue further. I agree with what @jonathandturner wrote earlier : "In ports/glutin/window.rs it assumes that Event::ReceivedCharacter will happen before Event::KeyboardInput". Reading some documentation about the Windows API it confirms that for key that can produce characters the order of events are :
Event::KeyboardInput - KeyDown
Event::ReceivedCharacter
Event::KeyboardInput - KeyUp

As a side note, I think we need to keep the 2 types of events. ReceivedCharacter are needed for input fields and KeyboardInput may be needed by javascript games for instance. More information here.

Back to the original subject. The logic of the Windows API does not seems to align with the one used by Servo. So, I think we need to see be sure in which order the events come in the other platforms (Linux & Mac) as they probably come in a different order. Then, we probably need to reorder them. It might be a good idea to look at implementation of cross platform libraries such as SDL, Qt, and more to see how they handle the problem.

EDIT : I think that they other issues in glutin and/or winit since they are keys that should create ReceivedCharacter that don't but I consider this a different issue.

@jonathandturner
Copy link

@jonathandturner jonathandturner commented Mar 27, 2017

I did a quick test.

macOS gives the events in this order:

  • ReceivedCharacter: 'h'
  • KeyboardInput: Pressed Some('h') H
  • KeyboardInput: Released Some('h') H

Windows:

  • KeyboardInput: Pressed None H
  • ReceivedCharacter: 'h'
  • KeyboardInput: Released None H

I'm guessing it's not feasible to try to reorder the events to make them the same because we won't always know what to wait for.

Going back to earlier thoughts, it seems we're trying to mix too much together, and we're linking events together we shouldn't be linking.

Can we perhaps use just the ReceivedCharacter event when we're in an input box?

@larsbergstrom larsbergstrom mentioned this issue Mar 27, 2017
11 of 11 tasks complete
@codec-abc
Copy link

@codec-abc codec-abc commented Mar 27, 2017

I agree with you on the last part. ReceivedCharacter may be enough for inputs field. Yet, I don't know enough to tell. But I tend to disagree with you and the reordering of events. We cannot do it on Windows since a KeyboardInput won't necessary lead to a ReceivedCharacter but the other way should work. I cannot see how we could have a ReceivedCharacter on macOS (or Linux) without a KeyboardInput just after. Moreover, the fact that the events does not came in the same order could theoretically leak to Js and it seems weird that the ordering is platform dependent. Testing this demo from this article give me the same events in the same order in macOs and Windows.

EDIT: I tested the link on macOs with Servo and it complies with the other browsers, meaning that the keydown event is triggered before the input event.

@jonathandturner
Copy link

@jonathandturner jonathandturner commented Mar 27, 2017

Huh, so are you saying that we're doing a reordering for macOS and Linux somewhere else in the code?

@codec-abc
Copy link

@codec-abc codec-abc commented Mar 27, 2017

I don't know how the Js events are created. If they are simply mapped from the keyboard ones then yes there is probably some reordering elsewhere in the code. But you can't take my word for it since I only look at the input and output and got no idea what happen in between.

@jonathandturner
Copy link

@jonathandturner jonathandturner commented Mar 28, 2017

Cool demo btw, it seems like there may be other keyboard event issues potentially (though maybe related to this issue we've been tracking down)

On Firefox (on my Mac):

screen shot 2017-03-28 at 3 04 31 pm

On Servo (on Windows):

screen shot 2017-03-28 at 3 04 15 pm

So we're seeing one onkeydown where we should be seeing an oninput, assuming I'm understanding the test correctly.

@codec-abc
Copy link

@codec-abc codec-abc commented Mar 28, 2017

The test is quite basic, it registers 2 events handlers that put textual description of the event below in the input field. The only particularity is that the oninput event handler remove the onkeydown event handler the first time it is triggered. On Windows since the events are in a different order we do not get the oninput event for the first character.

By the way, after looking more at the code I think I understand it a bit further. As I understand it, the whole thing happen in the function handle_window_event in the file ports\glutin\windows.rs. I think what we try to achieve in the code is to send events only for the keyboard but with an associated character if the key pressed has created one. In a way, we merge two event streams. Since on MacOs and Linux the character come before the key event we store it in pending_key_event_char and sent it with the key event that come right after. I think it is not possible to get the same behavior for Windows or a least not in a simple fashion (ie, not re-implementing an input method directly in Servo). If we do it in a reverse way (Pushing event as soon as they come but instead of storing a character we store the last virtual key code event instead) we would have something similar but with duplicate events. Typically if we do this for Windows if we type the character a we would get:

  1. Keyboard event / VirtualKeyCode a / Associated Character : None
  2. Keyboard event / VirtualKeyCode a / Associated Character : a

While on Linux & macOs we would only have

  1. Keyboard event / VirtualKeyCode a / Associated Character : a

So I think the only solution is to not merge event streams or at least at a later point in the code. IMHO, the first thing to do is see what happen in we send events for both CharacterInput & KeyboardInput in MacOs & Linux. It would probably force us to change code elsewhere since the previous test show we get 2 events on the Js side from a single WindowEvent::KeyEvent. In fact I just wonder if the sole purpose of the pending_key_event_char field is to align macOs & Linux with Js, doing in the same time some kind of hidden reordering just like I said in a previous message.

@codec-abc
Copy link

@codec-abc codec-abc commented Mar 29, 2017

Update: I have tested my previous idea ,ie keeping the last key pressed when receiving a keyboard KeyboardInput to send it later on when receiving a ReceivedCharacter. It does make the HTMLInputElement behave correctly on Windows. I am no more one character behind. Still, when testing the previous demo it shows that in the Js side we don't get the correct events.
At this point I see 2 solutions. The first is to send one more event so instead of having this (for Linux & MacOs) :

  1. Keydown event with optionally an associated character.
  2. Keyup event with no associated character.

we will get:

  1. Keydown event no associated character event there is a value in pending_key_event_char.
  2. Optionally Keydown event with a mandatory associated character in case the pending_key_event_char != None
  3. Keyup event with no associated character.

But it seems weird to me. The other solution is to reorder the events on MacOs and Linux and not correlate the key events to the character input events.

I will stop working on this issue until it is decided how we should tackle this issue.

@jonathandturner
Copy link

@jonathandturner jonathandturner commented Mar 29, 2017

@codec-abc - my priority is to get HTMLInputElement working so that we can get the nightly out there.

Getting the eventing in the right order is going to take more work. I noticed that, too.

Can you post your PR so we can review? We can talk with some of the other servo folks on the PR, but I think we should do this in stages:

  • Land something that fixes the input
  • Get nightly out
  • File separate issue for eventing

I suspect we'll end up having to rewrite a chunk of glutin (or possibly use another eventing system), but this should at least unblock us.

@codec-abc codec-abc mentioned this issue Mar 30, 2017
4 of 4 tasks complete
@codec-abc
Copy link

@codec-abc codec-abc commented Mar 30, 2017

@jonathandturner jonathandturner mentioned this issue Mar 31, 2017
4 of 5 tasks complete
jonathandturner pushed a commit to jonathandturner/servo that referenced this issue Apr 12, 2017
bors-servo added a commit that referenced this issue Apr 12, 2017
Fix windows glutin keydown

<!-- Please describe your changes on the following line: -->
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

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #12937 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because they are around Windows keyboard eventing.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16198)
<!-- Reviewable:end -->
@AshleyScirra
Copy link

@AshleyScirra AshleyScirra commented Apr 13, 2017

Is this meant to be fixed in the latest Windows Servo build? If I type "https://www.scirra.com" in the address bar, I get "htt/www.scirra.co".

@codec-abc
Copy link

@codec-abc codec-abc commented Apr 13, 2017

It seems that the PR that should fix this did not make it in the last nightly and there is another one in the way because of some error while cleaning the code. So I think the next nightly should have improvements concerning this bug.

@codec-abc
Copy link

@codec-abc codec-abc commented May 2, 2017

I had very little time for a few days but I managed to do a simple test of Glutin, SDL2 and GLFW on Windows and Linux for the keyboard ordering. My test was to open a window and type Hello (with a newline at end). My findings is that SDL2 and GLFW behave the same ways on Windows and Linux while Glutin behave differently depending of the OS. Moreover, SDL2 and GLFW follows this scheme (for a simple key that will trigger a character):

  1. Keydown event
  2. Character input
  3. Keyup event

With Glutin it is the same on Windows but on Linux (and probably Mac OS) it does the following:

  1. Character input
  2. Keydown event
  3. Keyup event

If you want to look at the code used my tests repositories are here: Glutin, SDL2 and GLFW, but there is really nothing to look at.

At this point I think someone should open a bug on Glutin to get it fixed but that means that Servo (and the other projects that use Glutin) may have to change some code at the next upgrade.

@jdm @jonathandturner Since you followed the issue quite closely, what are you thoughts on the matter?

@jdm
Copy link
Member

@jdm jdm commented May 2, 2017

Changing the event order in Glutin on Linux and Mac is straightforward: https://github.com/servo/glutin/blob/4c31b8b16f703766d4aa8d45762dab76f1684e86/src/api/cocoa/mod.rs#L1083-L1088 and https://github.com/servo/glutin/blob/4c31b8b16f703766d4aa8d45762dab76f1684e86/src/api/x11/input.rs#L154-L168 are fully in control of the event ordering, unlike the Windows implementation. The real solution is redesigning Servo's keyboard input implementation to interact better with this model of split event kinds, though.

@codec-abc
Copy link

@codec-abc codec-abc commented May 2, 2017

Thanks for the answer. Just to be sure, when you say "real" you mean difficult right? Because from my pov, there are still keyboard issues on Windows (but they are less visible thanks to the PR). Moreover, I think because Glutin aim to be a rust cross platform alternative to GLFW it should change its behavior to match GLFW. To conclude, I think both Servo and Glutin need to be updated in this regard. Anyway, I opened an issue in Glutin repository here.

@jdm
Copy link
Member

@jdm jdm commented May 2, 2017

What I meant by my comment was that changing the event order in glutin will only allow us to remove the per-platform code from the previous fix in Servo. It will not affect the fundamental limitations in the current model for processing keyboard input in Servo, which requires much more thought and effort.

@codec-abc
Copy link

@codec-abc codec-abc commented Oct 26, 2017

Just leaving this as a note. Glutin code has been updated - for quite a while actually - and now keyboard events are ordered in the same way no matter the underlying platform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants
You can’t perform that action at this time.