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

Fix keyboard on windows issue 12937 #16193

Closed

Conversation

@codec-abc
Copy link

codec-abc commented Mar 30, 2017

This is an attempt to fix the behavior of the input elements on Windows to fix #12937. Now, the input fields mostly work for the most part but the backspace key has a weird behavior and there is probably a couple of funkyness elsewhere By the way, I tried to align the keyboard events so we got the same in MacOs & Linux and Windows. A couple of things to notes:

  1. There are 2 types of events associated with keyboard: Key event (up or down) and Character Input.
  2. A character is not necessary created one a key is pressed (like the home key).
  3. 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.
  4. 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.
  5. 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
  1. 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.
  2. The Js event generated by Servo seems to be correct when testing on this page but not on Windows (before or after my changes). They may be no more correct in MacOs and Linux after my changes since I tried to align the event to a kind of middle ground. By the way this page may be serve a basic for a good test for input event order.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix (partially) #12937.
  • There are NO tests for these changes because I don't know how to write them and what to write.

This change is Reviewable

EDIT: I do not like the code I have written because there is quite a lot of platform dependent code in it. It would probably be proper create a file per platform so the logic behind may be encapsulated in those files.

@jonathandturner
Copy link

jonathandturner commented Mar 30, 2017

Just gave this a spin. While yeah, looks like we're getting the keys when we press them, I'm seeing things like backspace no longer work. It's not immediately obvious to me what's going wrong looking at the code, but I suspect non-printable characters aren't getting the same way they were before and we're missing a step, potentially.

@codec-abc
Copy link
Author

codec-abc commented Mar 30, 2017

Yeah, it seems weird since if I remember correctly when I pressed backspace the character was erased but then a square appear right away. Now while I think of it I remember a warning about not using the filter_nonprintable function and looking at the code it seems it is not used on Windows.

EDIT: After more looking at the code, I think the problem come from not using the filter_nonprintable function. I think the way to use it on Windows is to check if the value of the last last_pressed_key field is non printable when receiving a CharacterInput event. If the virtual keycode is non printable then the event should not be fired.

@jonathandturner
Copy link

jonathandturner commented Mar 30, 2017

Yeah, I'd believe that. When I was looking around before, I didn't see a way to check if the key was printable in the keydown event. Do you know of one?

@codec-abc
Copy link
Author

codec-abc commented Mar 30, 2017

I am not sure If I follow you since in the keydown event you should get the virtual key code (except if I missed something) and could use (almost as it is) the filter_nonprintable function.

@jonathandturner
Copy link

jonathandturner commented Mar 30, 2017

@codec-abc - Ahh, I misread what approach you were trying to take. I'm curious if your way works. If it does, it seems like the right way to do it.

@codec-abc
Copy link
Author

codec-abc commented Mar 30, 2017

Go ahead if you want to test it because I will not have time tomorrow.

@jonathandturner jonathandturner mentioned this pull request Mar 31, 2017
4 of 5 tasks complete
@jonathandturner
Copy link

jonathandturner commented Mar 31, 2017

@codec-abc - done: #16198

I agree, still not a perfect solution, but this one appears to work decently in practice and may be enough to get us going until we come up with a better solution.

@codec-abc
Copy link
Author

codec-abc commented Mar 31, 2017

Thanks! I will close this one since your PR goes a step further and fix more bugs.

@codec-abc codec-abc closed this Mar 31, 2017
bors-servo added a commit that referenced this pull request 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 -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 12, 2017
…r:fix_windows_gluten_keydown); r=jdm

<!-- 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 servo/servo#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 servo/servo#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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 4c851925fbbd8446f6bfc36ff6836e9b24ad635b

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : ed025584e25c5979861919d48ddedfe656f32729
Manishearth pushed a commit to Manishearth/gecko-dev that referenced this pull request Apr 21, 2017
…r:fix_windows_gluten_keydown); r=jdm

<!-- 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 servo/servo#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 servo/servo#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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 4c851925fbbd8446f6bfc36ff6836e9b24ad635b
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
…r:fix_windows_gluten_keydown); r=jdm

<!-- 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 servo/servo#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 servo/servo#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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 4c851925fbbd8446f6bfc36ff6836e9b24ad635b

UltraBlame original commit: 550323ea0dc566308f969a918b7fa17e08d9c7e2
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
…r:fix_windows_gluten_keydown); r=jdm

<!-- 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 servo/servo#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 servo/servo#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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 4c851925fbbd8446f6bfc36ff6836e9b24ad635b

UltraBlame original commit: 550323ea0dc566308f969a918b7fa17e08d9c7e2
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
…r:fix_windows_gluten_keydown); r=jdm

<!-- 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 servo/servo#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 servo/servo#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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 4c851925fbbd8446f6bfc36ff6836e9b24ad635b

UltraBlame original commit: 550323ea0dc566308f969a918b7fa17e08d9c7e2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

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