Support non-QWERTY keyboards #11950

jdm · 2016-06-30T08:35:09Z

Using the ReceivedCharacter event from glutin, we can obtain the actual key characters that the user is pressing and releasing. This gets passed to the script thread along with the physical key data, since KeyboardEvent needs both pieces of information, where they get merged into a single logical key that gets processed by clients like TextInput without any special changes.

Tested by switching my macbook keyboard to dvorak and looking at the output of keypress/keyup/keydown event listeners, as well as playing with tests/html/textarea.html. Non-content keybindings like reload work as expected, too - the remapped keybinding triggers the reload action.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #4144
These changes do not require tests because I can't think of a way to test remapped keyboard input

Fixes #11991.

This change is

highfive · 2016-06-30T08:35:13Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs
@KiChjang: components/script/dom/document.rs, components/script/dom/keyboardevent.rs, components/script/textinput.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/script_thread.rs, components/script/dom/bindings/str.rs, components/script_traits/lib.rs, components/script_traits/lib.rs

highfive · 2016-06-30T08:35:14Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

nox · 2016-06-30T08:45:45Z

Mac French keyboard here, 'a' works, but not shift etc to input '.' with 'shift-;'.

paulrouget · 2016-06-30T09:25:11Z

Mac French keyboard here, 'a' works, but not shift etc to input '.' with 'shift-;'.

What happens if you press "Shift + a"?


        Auto merge of #11955 - nox:azerty-ftw, r=<try>

Support non-QWERTY keyboards, take 2 See #11950.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11955)

jdm · 2016-06-30T16:43:24Z

Tested against a french keyboard and everything seems to be working as expected.

mbrubeck · 2016-06-30T19:15:19Z

ports/glutin/window.rs

@@ -233,7 +241,13 @@ impl Window {

    fn handle_window_event(&self, event: glutin::Event) -> bool {
        match event {
-            Event::KeyboardInput(element_state, _scan_code, Some(virtual_key_code)) => {
+            Event::ReceivedCharacter(ch) => {
+                assert!(self.pending_key_event_char.get().is_none());


This assertion panics if I try to type a character using the Compose key (e.g. Compose+A+E to type æ).

mbrubeck · 2016-06-30T19:16:32Z

My Linux/X11 dvorak key mapping still works with this patch, except for one corner case noted above.

nox · 2016-07-01T16:50:37Z

@jdm It seems you didn't include my bincode bump that fixes the char serialization.

jdm · 2016-07-01T17:01:30Z

@nox It's... right there? In the PR?

nox · 2016-07-01T17:17:55Z

Oh right, didn't look at the right commit.

nox · 2016-07-02T13:08:48Z

components/script/dom/keyboardevent.rs

@@ -69,6 +70,7 @@ impl KeyboardEvent {
               cancelable: bool,
               view: Option<&Window>,
               _detail: i32,
+               ch: Option<char>,


Why do we need both this and char_code?

Because char_code isn't present in all key events, per spec.

pcwalton · 2016-07-02T17:40:47Z

@jdm Who would you like to review this?

jdm · 2016-07-02T17:55:03Z

Someone who wants to learn more about how we do keyboard input? I don't have a name in mind, unfortunately.

bors-servo · 2016-07-03T16:58:07Z

☔ The latest upstream changes (presumably #12178) made this pull request unmergeable. Please resolve the merge conflicts.

nox · 2016-07-04T09:03:47Z

SGTM. What do we need for this to get landed? What's blocking this to be reviewed?

jdm · 2016-07-04T13:23:38Z

Nothing's blocking it that I'm aware.

emilio · 2016-07-04T23:42:54Z

Looks good to me modulo the nits, the ipc-channel/bincode update, and the compose key, that is servo/glutin#101

-S-awaiting-review +S-needs-code-changes

Reviewed 12 of 12 files at r1.
Review status: 4 of 17 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

components/constellation/constellation.rs, line 809 [r1] (raw file):

            FromScriptMsg::SendKeyEvent(key, key_state, key_modifiers, ch) => {
                // FIXME: https://github.com/servo/ipc-channel/issues/84
                let ch = ch.map(|ch| ch as char);

This can be removed now right?

components/constellation/constellation.rs, line 1636 [r1] (raw file):

                };
                for (key, mods, state) in cmd {
                    let event = CompositorEvent::KeyEvent(key, state, mods, None);

Can we be consistent with the order here?

Either (ch, key, state, mods) or (key, state, mods, ch).

ports/glutin/window.rs, line 245 [r3] (raw file):

Previously, mbrubeck (Matt Brubeck) wrote…

This assertion panics if I try to type a character using the Compose key (e.g. Compose+A+E to type æ).

As Matt, I also happen to hit this assertion whenever I type a spanish character with the compose key (e.g. Compose + ' + a -> `á`).

It seems that glutin doesn't handle the specific compose key. I have a patch to add VirtualKeyCode::Compose to glutin, and then we should handle that in the key modifiers ignoring any character that arrives while pressing Composed (the actual character will arrive later).

Glutin also sends the composed character twice. I can look deeper into it, but meanwhile we can just check for pending_key_event_char.get() != Some(ch).

Comments from Reviewable

emilio · 2016-07-04T23:44:59Z

Ah, and I don't think I need to say this, but awesome work here, thanks for doing this Josh! :)

bors-servo · 2016-07-06T00:40:26Z

💔 Test failed - linux-dev

emilio · 2016-07-06T00:45:40Z

Hm... need to update the cef port :/


        Make textinput handle actual key values. Don't restrict character val…

…ues to a single byte.

jdm · 2016-07-06T04:10:39Z

@bors-servo: r=emilio

bors-servo · 2016-07-06T04:10:40Z

📌 Commit 6496d73 has been approved by emilio

bors-servo · 2016-07-06T04:29:05Z

⌛ Testing commit 6496d73 with merge a51c60c...


        Auto merge of #11950 - jdm:keylayout2, r=emilio

Support non-QWERTY keyboards Using the ReceivedCharacter event from glutin, we can obtain the actual key characters that the user is pressing and releasing. This gets passed to the script thread along with the physical key data, since KeyboardEvent needs both pieces of information, where they get merged into a single logical key that gets processed by clients like TextInput without any special changes. Tested by switching my macbook keyboard to dvorak and looking at the output of keypress/keyup/keydown event listeners, as well as playing with tests/html/textarea.html. Non-content keybindings like reload work as expected, too - the remapped keybinding triggers the reload action. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #4144 - [X] These changes do not require tests because I can't think of a way to test remapped keyboard input Fixes #11991.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11950)

bors-servo · 2016-07-06T05:28:01Z

💔 Test failed - linux-rel

highfive · 2016-07-06T05:28:04Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform3d-translatez-001.htm
  └   → /css-transforms-1_dev/html/transform3d-translatez-001.htm 902d90a8d198625335e738587fdfe81dcc90392d
/css-transforms-1_dev/html/reference/transform3d-translatez-ref.htm b14318ccfd8a59b7b249d41521e178d617678823
Testing 902d90a8d198625335e738587fdfe81dcc90392d == b14318ccfd8a59b7b249d41521e178d617678823
/css-transforms-1_dev/html/transform3d-translatez-001.htm 902d90a8d198625335e738587fdfe81dcc90392d
/css-transforms-1_dev/html/reference/transform3d-translatez-notref.htm 902d90a8d198625335e738587fdfe81dcc90392d
Testing 902d90a8d198625335e738587fdfe81dcc90392d != 902d90a8d198625335e738587fdfe81dcc90392d

KiChjang · 2016-07-06T05:29:01Z

@bors-servo retry

#11631

bors-servo · 2016-07-06T06:20:47Z

⚡ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel...

bors-servo · 2016-07-06T07:12:57Z

💔 Test failed - linux-rel

nox · 2016-07-06T09:42:46Z

@bors-servo retry #11693

bors-servo · 2016-07-06T09:51:51Z

⌛ Testing commit 6496d73 with merge 68fb9eb...


        Auto merge of #11950 - jdm:keylayout2, r=emilio

Support non-QWERTY keyboards Using the ReceivedCharacter event from glutin, we can obtain the actual key characters that the user is pressing and releasing. This gets passed to the script thread along with the physical key data, since KeyboardEvent needs both pieces of information, where they get merged into a single logical key that gets processed by clients like TextInput without any special changes. Tested by switching my macbook keyboard to dvorak and looking at the output of keypress/keyup/keydown event listeners, as well as playing with tests/html/textarea.html. Non-content keybindings like reload work as expected, too - the remapped keybinding triggers the reload action. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #4144 - [X] These changes do not require tests because I can't think of a way to test remapped keyboard input Fixes #11991.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11950)

bors-servo · 2016-07-06T10:47:17Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned metajack Jun 30, 2016

highfive added the S-awaiting-review label Jun 30, 2016

nox mentioned this pull request Jun 30, 2016

Support non-QWERTY keyboards, take 2 #11955

Closed

jdm force-pushed the jdm:keylayout2 branch from 373e3d5 to d5a0204 Jun 30, 2016

mbrubeck reviewed Jun 30, 2016
View changes

Ms2ger mentioned this pull request Jul 1, 2016

Keyboard keys AZERTY are badly mapped #11991

Closed

fabricedesre mentioned this pull request Jul 1, 2016

My colemak keyboard layout is not recognized #12043

Closed

metajack mentioned this pull request Jul 1, 2016

Keymap ignored #12057

Closed

nox reviewed Jul 2, 2016
View changes

jdm mentioned this pull request Jul 2, 2016

Shift-7 enters "&" instead of "/" on German keyboard #12189

Closed

highfive added the S-needs-rebase label Jul 3, 2016

emilio added S-needs-code-changes and removed S-awaiting-review labels Jul 4, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 6, 2016

Make textinput handle actual key values. Don't restrict character val…

Loading status checks…

6496d73

…ues to a single byte.

jdm force-pushed the jdm:keylayout2 branch from 44e42b1 to 6496d73 Jul 6, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Jul 6, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 6, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 6, 2016

highfive added the S-tests-failed label Jul 6, 2016

nox added S-awaiting-merge and removed S-tests-failed labels Jul 6, 2016

bors-servo merged commit 6496d73 into servo:master Jul 6, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 6, 2016

servo / servo

Support non-QWERTY keyboards #11950

Support non-QWERTY keyboards #11950

jdm commented Jun 30, 2016 •

edited by Ms2ger

highfive commented Jun 30, 2016

highfive commented Jun 30, 2016

nox commented Jun 30, 2016

paulrouget commented Jun 30, 2016

jdm commented Jun 30, 2016

This comment has been minimized.

mbrubeck commented Jun 30, 2016

nox commented Jul 1, 2016

jdm commented Jul 1, 2016

nox commented Jul 1, 2016

This comment has been minimized.

This comment has been minimized.

pcwalton commented Jul 2, 2016

jdm commented Jul 2, 2016

bors-servo commented Jul 3, 2016

nox commented Jul 4, 2016

jdm commented Jul 4, 2016

emilio commented Jul 4, 2016

emilio commented Jul 4, 2016

bors-servo commented Jul 6, 2016

emilio commented Jul 6, 2016

jdm commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

highfive commented Jul 6, 2016

KiChjang commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

nox commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

servo / servo

Join GitHub today

Support non-QWERTY keyboards #11950

Support non-QWERTY keyboards #11950

Conversation

jdm commented Jun 30, 2016 • edited by Ms2ger

highfive commented Jun 30, 2016

highfive commented Jun 30, 2016

nox commented Jun 30, 2016

paulrouget commented Jun 30, 2016

jdm commented Jun 30, 2016

This comment has been minimized.

mbrubeck Jun 30, 2016

mbrubeck commented Jun 30, 2016

nox commented Jul 1, 2016

jdm commented Jul 1, 2016

nox commented Jul 1, 2016

This comment has been minimized.

nox Jul 2, 2016

This comment has been minimized.

jdm Jul 2, 2016

pcwalton commented Jul 2, 2016

jdm commented Jul 2, 2016

bors-servo commented Jul 3, 2016

nox commented Jul 4, 2016

jdm commented Jul 4, 2016

emilio commented Jul 4, 2016

emilio commented Jul 4, 2016

bors-servo commented Jul 6, 2016

emilio commented Jul 6, 2016

jdm commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

highfive commented Jul 6, 2016

KiChjang commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

nox commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

jdm commented Jun 30, 2016 •

edited by Ms2ger