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

Key events involving the control key don't work as expected #17146

Closed
paulrouget opened this issue Jun 3, 2017 · 7 comments
Closed

Key events involving the control key don't work as expected #17146

paulrouget opened this issue Jun 3, 2017 · 7 comments

Comments

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jun 3, 2017

We realized that no keybindings on Windows work (the "reload"/Ctrl-R binding doesn't work for example).

This is because Glutin sends 2 things:

  • the character matching the keyboard layout (ReceivedCharacter(ch))
  • the key code and modifiers (KeyboardInput)

But the character that is sent when the Ctrl key is pressed is a different unicode character.
For exemple, Ctrl-L will send the "Form Feed" \u{c} character, not L.

A possible approach is to map the key code to the character. This is what is done in the DOM code here:

Cow::from(match key {

But this is wrong because the event's key property is not localized.

Another approach is to map the combo unicode character to its original key. For example:

'\u{c}' => 'l',
'\u{12}' => 'r',
…

Both the ports/glutin/window.rs and components/script/dom/keyboardevent.rs need this.

@jklepatch
Copy link
Contributor

@jklepatch jklepatch commented Jun 3, 2017

I am working on this

@atouchet
Copy link
Contributor

@atouchet atouchet commented Jun 3, 2017

Is #16459 related to this issue?

@jklepatch jklepatch mentioned this issue Jun 4, 2017
3 of 4 tasks complete
@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jun 4, 2017

Is #16459 related to this issue?

Very likely, yes.

@atouchet
Copy link
Contributor

@atouchet atouchet commented Mar 14, 2018

@paulrouget do you know what the current situation is regarding this issue? As far as I can tell control key input still isn't working properly after the Glutin update.

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Mar 14, 2018

We need to investigate see if that needs to be fixed in glutin or in servo.

bors-servo added a commit that referenced this issue Mar 22, 2018
fix(keyevent): do not emit default ignorable codepoint

<!-- Please describe your changes on the following line: -->
This PR intends to update `KeyEvent` emit behavior around #18130. Issue #17146 (comment) briefly explains what's happening in servo currently - there are KeyboardInput event emitted for separated key (modifier, and `V` in case of paste) and there are also `ReceivedCharacter` event corresponds to `\u{0016}`.

`0x0016` is unicode representation of `Synchronous Idle` (https://en.wikipedia.org/wiki/Synchronous_Idle), belong under category of `Default Ignorable` charater in unicode range doesn't have visual representation (http://www.unicode.org/L2/L2002/02368-default-ignorable.pdf). Currently servo forwards all of emitted event from `winit` including this, eventually leads into double execution of control event.

In this change try to omit default ignorable charater , if given char received is within range of ignorable do not dispatch `KeyEvent`. Once those are omitted, current event handling logic already takes care of key event with correct modifier state so duplicated event handling won't occur.

For implementation perspective, `std::char` in Rust doesn't seem to support `isIdentifierIgnorable` like other platform does (i.e: https://msdn.microsoft.com/en-us/library/aa285330(v=vs.60).aspx / https://docs.oracle.com/javase/7/docs/api/java/lang/Character.html) - so does quick, naïve range comparison check based on unicode range specified in spec, similar to halfbuzz and other does. (https://github.com/harfbuzz/harfbuzz/blob/master/src/hb-unicode-private.hh#L134)

Lastly, this is indeed behavior of `winit` to emit all characters by default. *Why not try to make upstream changes instead?* While I've been reading through issues in `winit`, issue like rust-windowing/winit#350 trying to emit ignorable character by its intention (delete key `ReceivedCharacter` is also under category of default ignorable) and let each consumer application handles it as needed. I assume it'll cause breaking changes in winit's design if it intends to omit those characters, instead tried to make application-level changes.

Couple of consideration for review

- Is it desired changes to not emit `KeyEvent` for default ignorable chars? Do we rather want mapping / or restoring back to original char as @paulrouget mentioned in #17146 (comment)?
- Any better, recommended approach to detect unicode char range?
- Maybe try to make upstream changes to `winit` still, like having configuratble way to opt-in(out) those char event?

---
<!-- 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 #18130 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____
- This PR has been locally tested on Windows, Linux machines.

<!-- 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/20327)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 22, 2018
…depoint (from kwonoj:fix-ignore); r=paulrouget

<!-- Please describe your changes on the following line: -->
This PR intends to update `KeyEvent` emit behavior around #18130. Issue servo/servo#17146 (comment) briefly explains what's happening in servo currently - there are KeyboardInput event emitted for separated key (modifier, and `V` in case of paste) and there are also `ReceivedCharacter` event corresponds to `\u{0016}`.

`0x0016` is unicode representation of `Synchronous Idle` (https://en.wikipedia.org/wiki/Synchronous_Idle), belong under category of `Default Ignorable` charater in unicode range doesn't have visual representation (http://www.unicode.org/L2/L2002/02368-default-ignorable.pdf). Currently servo forwards all of emitted event from `winit` including this, eventually leads into double execution of control event.

In this change try to omit default ignorable charater , if given char received is within range of ignorable do not dispatch `KeyEvent`. Once those are omitted, current event handling logic already takes care of key event with correct modifier state so duplicated event handling won't occur.

For implementation perspective, `std::char` in Rust doesn't seem to support `isIdentifierIgnorable` like other platform does (i.e: https://msdn.microsoft.com/en-us/library/aa285330(v=vs.60).aspx / https://docs.oracle.com/javase/7/docs/api/java/lang/Character.html) - so does quick, naïve range comparison check based on unicode range specified in spec, similar to halfbuzz and other does. (https://github.com/harfbuzz/harfbuzz/blob/master/src/hb-unicode-private.hh#L134)

Lastly, this is indeed behavior of `winit` to emit all characters by default. *Why not try to make upstream changes instead?* While I've been reading through issues in `winit`, issue like rust-windowing/winit#350 trying to emit ignorable character by its intention (delete key `ReceivedCharacter` is also under category of default ignorable) and let each consumer application handles it as needed. I assume it'll cause breaking changes in winit's design if it intends to omit those characters, instead tried to make application-level changes.

Couple of consideration for review

- Is it desired changes to not emit `KeyEvent` for default ignorable chars? Do we rather want mapping / or restoring back to original char as @paulrouget mentioned in servo/servo#17146 (comment)?
- Any better, recommended approach to detect unicode char range?
- Maybe try to make upstream changes to `winit` still, like having configuratble way to opt-in(out) those char event?

---
<!-- 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 #18130 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____
- This PR has been locally tested on Windows, Linux machines.

<!-- 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: 28c92db26837531e75327cff7727ed8bc1c70b48

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 2dcf517220e225cf478e056432d0b84c0b2b5d9b
bors-servo added a commit that referenced this issue Jul 30, 2018
Refactor winit key handling

This should improve keys input on Linux and Windows.
Should fix #17146 and #21161

Tested Mac, Windows and Linux. Basic typing works, combo work, text navigation works. I hit some strange issues where sometimes the text would be displayed late, but I believe it is unrelated to this PR.

If we land that now, we will hit a regression on Mac, we need a winit update that includes rust-windowing/winit#610.

@kwonoj and @atouchet I'd appreciate if you could look at this.

<!-- 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/21250)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 30, 2018
Refactor winit key handling

This should improve keys input on Linux and Windows.
Should fix #17146 and fix #21161

Tested Mac, Windows and Linux. Basic typing works, combo work, text navigation works. I hit some strange issues where sometimes the text would be displayed late, but I believe it is unrelated to this PR.

If we land that now, we will hit a regression on Mac, we need a winit update that includes rust-windowing/winit#610.

<!-- 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/21250)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 30, 2018
This should improve keys input on Linux and Windows.
Should fix #17146 and fix #21161

Tested Mac, Windows and Linux. Basic typing works, combo work, text navigation works. I hit some strange issues where sometimes the text would be displayed late, but I believe it is unrelated to this PR.

If we land that now, we will hit a regression on Mac, we need a winit update that includes rust-windowing/winit#610.

<!-- 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/21250)
<!-- Reviewable:end -->
@atouchet
Copy link
Contributor

@atouchet atouchet commented Jul 31, 2018

@paulrouget I've been testing your PR for this and it works somewhat but there are still some definite issues. On first launch using Ctrl + L to open the URL dialog works consistently but subsequent attempts at doing this (ex. load servo.org homepage -> press Ctrl + L to navigate to new page -> press Ctrl + L again to navigate to another new page) do not bring up the dialog again. It seems to work only once and then rarely or not at all afterwards. Should I file a new issue for this behaviour?

Edit: It's actually easier than that to reproduce. You can just exit the dialog box after bringing it up with Ctrl + L and you won't be able to bring up back up again easily.

Edit 2: I've now been able to bring up the URL dialog without even pressing Ctrl and L at the same time at one point. There's definitely something off here.

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jul 31, 2018

Should I file a new issue for this behaviour?

Yes please.

Thanks!

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 2, 2019
…depoint (from kwonoj:fix-ignore); r=paulrouget

<!-- Please describe your changes on the following line: -->
This PR intends to update `KeyEvent` emit behavior around #18130. Issue servo/servo#17146 (comment) briefly explains what's happening in servo currently - there are KeyboardInput event emitted for separated key (modifier, and `V` in case of paste) and there are also `ReceivedCharacter` event corresponds to `\u{0016}`.

`0x0016` is unicode representation of `Synchronous Idle` (https://en.wikipedia.org/wiki/Synchronous_Idle), belong under category of `Default Ignorable` charater in unicode range doesn't have visual representation (http://www.unicode.org/L2/L2002/02368-default-ignorable.pdf). Currently servo forwards all of emitted event from `winit` including this, eventually leads into double execution of control event.

In this change try to omit default ignorable charater , if given char received is within range of ignorable do not dispatch `KeyEvent`. Once those are omitted, current event handling logic already takes care of key event with correct modifier state so duplicated event handling won't occur.

For implementation perspective, `std::char` in Rust doesn't seem to support `isIdentifierIgnorable` like other platform does (i.e: https://msdn.microsoft.com/en-us/library/aa285330(v=vs.60).aspx / https://docs.oracle.com/javase/7/docs/api/java/lang/Character.html) - so does quick, naïve range comparison check based on unicode range specified in spec, similar to halfbuzz and other does. (https://github.com/harfbuzz/harfbuzz/blob/master/src/hb-unicode-private.hh#L134)

Lastly, this is indeed behavior of `winit` to emit all characters by default. *Why not try to make upstream changes instead?* While I've been reading through issues in `winit`, issue like rust-windowing/winit#350 trying to emit ignorable character by its intention (delete key `ReceivedCharacter` is also under category of default ignorable) and let each consumer application handles it as needed. I assume it'll cause breaking changes in winit's design if it intends to omit those characters, instead tried to make application-level changes.

Couple of consideration for review

- Is it desired changes to not emit `KeyEvent` for default ignorable chars? Do we rather want mapping / or restoring back to original char as paulrouget mentioned in servo/servo#17146 (comment)?
- Any better, recommended approach to detect unicode char range?
- Maybe try to make upstream changes to `winit` still, like having configuratble way to opt-in(out) those char event?

---
<!-- 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 #18130 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____
- This PR has been locally tested on Windows, Linux machines.

<!-- 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: 28c92db26837531e75327cff7727ed8bc1c70b48

UltraBlame original commit: 4e150d44fea43fada64c447fa5ebabaf84eb748a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 2, 2019
…depoint (from kwonoj:fix-ignore); r=paulrouget

<!-- Please describe your changes on the following line: -->
This PR intends to update `KeyEvent` emit behavior around #18130. Issue servo/servo#17146 (comment) briefly explains what's happening in servo currently - there are KeyboardInput event emitted for separated key (modifier, and `V` in case of paste) and there are also `ReceivedCharacter` event corresponds to `\u{0016}`.

`0x0016` is unicode representation of `Synchronous Idle` (https://en.wikipedia.org/wiki/Synchronous_Idle), belong under category of `Default Ignorable` charater in unicode range doesn't have visual representation (http://www.unicode.org/L2/L2002/02368-default-ignorable.pdf). Currently servo forwards all of emitted event from `winit` including this, eventually leads into double execution of control event.

In this change try to omit default ignorable charater , if given char received is within range of ignorable do not dispatch `KeyEvent`. Once those are omitted, current event handling logic already takes care of key event with correct modifier state so duplicated event handling won't occur.

For implementation perspective, `std::char` in Rust doesn't seem to support `isIdentifierIgnorable` like other platform does (i.e: https://msdn.microsoft.com/en-us/library/aa285330(v=vs.60).aspx / https://docs.oracle.com/javase/7/docs/api/java/lang/Character.html) - so does quick, naïve range comparison check based on unicode range specified in spec, similar to halfbuzz and other does. (https://github.com/harfbuzz/harfbuzz/blob/master/src/hb-unicode-private.hh#L134)

Lastly, this is indeed behavior of `winit` to emit all characters by default. *Why not try to make upstream changes instead?* While I've been reading through issues in `winit`, issue like rust-windowing/winit#350 trying to emit ignorable character by its intention (delete key `ReceivedCharacter` is also under category of default ignorable) and let each consumer application handles it as needed. I assume it'll cause breaking changes in winit's design if it intends to omit those characters, instead tried to make application-level changes.

Couple of consideration for review

- Is it desired changes to not emit `KeyEvent` for default ignorable chars? Do we rather want mapping / or restoring back to original char as paulrouget mentioned in servo/servo#17146 (comment)?
- Any better, recommended approach to detect unicode char range?
- Maybe try to make upstream changes to `winit` still, like having configuratble way to opt-in(out) those char event?

---
<!-- 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 #18130 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____
- This PR has been locally tested on Windows, Linux machines.

<!-- 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: 28c92db26837531e75327cff7727ed8bc1c70b48

UltraBlame original commit: 4e150d44fea43fada64c447fa5ebabaf84eb748a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 2, 2019
…depoint (from kwonoj:fix-ignore); r=paulrouget

<!-- Please describe your changes on the following line: -->
This PR intends to update `KeyEvent` emit behavior around #18130. Issue servo/servo#17146 (comment) briefly explains what's happening in servo currently - there are KeyboardInput event emitted for separated key (modifier, and `V` in case of paste) and there are also `ReceivedCharacter` event corresponds to `\u{0016}`.

`0x0016` is unicode representation of `Synchronous Idle` (https://en.wikipedia.org/wiki/Synchronous_Idle), belong under category of `Default Ignorable` charater in unicode range doesn't have visual representation (http://www.unicode.org/L2/L2002/02368-default-ignorable.pdf). Currently servo forwards all of emitted event from `winit` including this, eventually leads into double execution of control event.

In this change try to omit default ignorable charater , if given char received is within range of ignorable do not dispatch `KeyEvent`. Once those are omitted, current event handling logic already takes care of key event with correct modifier state so duplicated event handling won't occur.

For implementation perspective, `std::char` in Rust doesn't seem to support `isIdentifierIgnorable` like other platform does (i.e: https://msdn.microsoft.com/en-us/library/aa285330(v=vs.60).aspx / https://docs.oracle.com/javase/7/docs/api/java/lang/Character.html) - so does quick, naïve range comparison check based on unicode range specified in spec, similar to halfbuzz and other does. (https://github.com/harfbuzz/harfbuzz/blob/master/src/hb-unicode-private.hh#L134)

Lastly, this is indeed behavior of `winit` to emit all characters by default. *Why not try to make upstream changes instead?* While I've been reading through issues in `winit`, issue like rust-windowing/winit#350 trying to emit ignorable character by its intention (delete key `ReceivedCharacter` is also under category of default ignorable) and let each consumer application handles it as needed. I assume it'll cause breaking changes in winit's design if it intends to omit those characters, instead tried to make application-level changes.

Couple of consideration for review

- Is it desired changes to not emit `KeyEvent` for default ignorable chars? Do we rather want mapping / or restoring back to original char as paulrouget mentioned in servo/servo#17146 (comment)?
- Any better, recommended approach to detect unicode char range?
- Maybe try to make upstream changes to `winit` still, like having configuratble way to opt-in(out) those char event?

---
<!-- 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 #18130 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____
- This PR has been locally tested on Windows, Linux machines.

<!-- 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: 28c92db26837531e75327cff7727ed8bc1c70b48

UltraBlame original commit: 4e150d44fea43fada64c447fa5ebabaf84eb748a
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.

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