Refactor winit key handling #21250

paulrouget · 2018-07-26T12:47:33Z

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.

This change is


        Refactor winit key handling

paulrouget · 2018-07-26T13:01:02Z

I basically recorded all the winit events when doing this: #21161 (comment)

Linux and Windows behave the same way. Mac has extra ReceivedCharacter events.

I figured that the logic is to only send release events on KeyboardInput and send press events on ReceivedCharacter only if the character is printable (see the code comments).

paulrouget · 2018-07-26T13:03:02Z

/cc @dorfsmay maybe you can also test this PR

dorfsmay · 2018-07-26T15:32:39Z

The sticky control key is still there.

I checked that I'm on the right branch (checked the line in the files), and also:

$ git branch -v
* fixkeys 3c7bb87 Refactor winit key handling
master  4c5a7ea Auto merge of #21246 - servo:webgl, r=emilio

But, it's not worse, so if this PR fixes other things it's worth merging, and we can investigate the Linux ctrl key further later.

paulrouget · 2018-07-27T08:26:53Z

@dorfsmay with this PR, here, I can not reproduce the sticky control key issue.

This is what I do:

Linux, Xorg server
./mach run -d 'data:text/html,<input>'
click on the text input
Ctrl-L, this brings the popup. Click cancel.
l, the letter l is printed in the input

dorfsmay · 2018-07-27T16:11:43Z

Correct that sequence works fine, but this one shows the stickiness of the control key:

./mach run --dev
move mouse to servo and give focus
press the R key - nothing happens - good, as expected
press ctrl-R - nothing happens - bad, should reload
press ctrl-R - reloads content
press R - reloads content!! - bad, shows that the ctrl state is stuck
press ctrl-R (or anything) - reloads
press R - nothing happens, shows that the ctrl state is gone now

If this PR fixes other issues, I think it's worth merging, and we can keep working on the sticky control separately.

jdm

These changes seem like a reasonable improvement.

kwonoj · 2018-07-27T18:24:09Z

Changewise looks fine, I may try few on windows once this is landed.

paulrouget · 2018-07-30T10:33:10Z

@dorfsmay sadly I cannot reproduce your steps. Works fine here on Ubuntu. ctrl-R reloads on first hit. Can you check with xev the events you got on Ctrl-R and gather as many info about this bug in a different issue? We will also need to understand which version of Linux you use, your keyboard layout, and if possible, some winit logs.

paulrouget · 2018-07-30T10:33:31Z

@bors-servo r=jdm

bors-servo · 2018-07-30T10:33:31Z

📌 Commit 3c7bb87 has been approved by jdm

bors-servo · 2018-07-30T10:33:38Z

⌛ Testing commit 3c7bb87 with merge 9c7891c...


        Auto merge of #21250 - paulrouget:fixkeys, r=jdm

@kwonoj

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.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21250)

bors-servo · 2018-07-30T10:51:18Z

💔 Test failed - mac-rel-css1

jdm · 2018-07-30T13:33:18Z

@bors-servo retry

atouchet · 2018-07-30T15:20:24Z

I will make sure to test this on Windows when it's merged.

bors-servo · 2018-07-30T15:24:36Z

⚡ Previous build results for android, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only mac-rel-css1...

bors-servo · 2018-07-30T15:43:18Z

💔 Test failed - mac-rel-css1

CYBAI · 2018-07-30T16:01:43Z

@bors-servo retry

#JustInfraThings

bors-servo · 2018-07-30T17:02:43Z

⌛ Testing commit 3c7bb87 with merge 1bd34e7...


        Auto merge of #21250 - paulrouget:fixkeys, r=jdm

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.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21250)

bors-servo · 2018-07-30T18:05:11Z

💔 Test failed - android

jdm · 2018-07-30T18:08:27Z

@bors-servo retry

cleared out the builder

bors-servo · 2018-07-30T18:29:57Z

⚡ Previous build results for android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only android...


        Auto merge of #21250 - paulrouget:fixkeys, r=jdm

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.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21250)

bors-servo · 2018-07-30T19:34:27Z

⌛ Testing commit 3c7bb87 with merge 9790f87...

bors-servo · 2018-07-30T19:35:43Z

💔 Test failed - android

jdm · 2018-07-30T19:36:43Z

@bors-servo retry

bors-servo · 2018-07-30T19:40:02Z

⚡ Previous build results for android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only android...

bors-servo · 2018-07-30T20:07:34Z

☀️ Test successful - android, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 1bd34e7 to master...

Refactor winit key handling

Loading status checks…

3c7bb87

highfive assigned emilio Jul 26, 2018

highfive added the S-awaiting-review label Jul 26, 2018

paulrouget mentioned this pull request Jul 26, 2018

Keybindings don't work on Linux and maybe windows #21161

Closed

jdm approved these changes Jul 27, 2018

View changes

jdm assigned jdm and unassigned emilio Jul 27, 2018

jdm added S-blocked-on-external and removed S-awaiting-review labels Jul 27, 2018

highfive added the S-awaiting-merge label Jul 30, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 30, 2018

highfive added the S-tests-failed label Jul 30, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 30, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 30, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 30, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 30, 2018

This was referenced Jul 30, 2018

Ignore navigation input if key state is Released #20628

Closed

Revisit how the Android port works. #20912

Merged

Android: Introduce ServoSurface #21234

Merged

Publish a AAR with servoview #21199

Merged

paulrouget mentioned this pull request Jul 31, 2018

Go back/forward keybinding should not traverse history on key release #20478

Closed

atouchet mentioned this pull request Jul 31, 2018

Issues with URL dialog box #21294

Closed

servo / servo

Refactor winit key handling #21250

Refactor winit key handling #21250

paulrouget commented Jul 26, 2018 •

edited by atouchet

paulrouget commented Jul 26, 2018

paulrouget commented Jul 26, 2018 •

edited

dorfsmay commented Jul 26, 2018

paulrouget commented Jul 27, 2018

dorfsmay commented Jul 27, 2018

jdm left a comment

kwonoj commented Jul 27, 2018

paulrouget commented Jul 30, 2018

paulrouget commented Jul 30, 2018

bors-servo commented Jul 30, 2018

bors-servo commented Jul 30, 2018

bors-servo commented Jul 30, 2018

jdm commented Jul 30, 2018

atouchet commented Jul 30, 2018

bors-servo commented Jul 30, 2018

bors-servo commented Jul 30, 2018

CYBAI commented Jul 30, 2018

bors-servo commented Jul 30, 2018

bors-servo commented Jul 30, 2018

jdm commented Jul 30, 2018

bors-servo commented Jul 30, 2018

bors-servo commented Jul 30, 2018

bors-servo commented Jul 30, 2018

jdm commented Jul 30, 2018

bors-servo commented Jul 30, 2018

bors-servo commented Jul 30, 2018

servo / servo

Join GitHub today

Refactor winit key handling #21250

Refactor winit key handling #21250

Conversation

paulrouget commented Jul 26, 2018 • edited by atouchet

paulrouget commented Jul 26, 2018

paulrouget commented Jul 26, 2018 • edited

dorfsmay commented Jul 26, 2018

paulrouget commented Jul 27, 2018

dorfsmay commented Jul 27, 2018

jdm left a comment

kwonoj commented Jul 27, 2018

paulrouget commented Jul 30, 2018

paulrouget commented Jul 30, 2018

bors-servo commented Jul 30, 2018

bors-servo commented Jul 30, 2018

bors-servo commented Jul 30, 2018

jdm commented Jul 30, 2018

atouchet commented Jul 30, 2018

bors-servo commented Jul 30, 2018

bors-servo commented Jul 30, 2018

CYBAI commented Jul 30, 2018

bors-servo commented Jul 30, 2018

bors-servo commented Jul 30, 2018

jdm commented Jul 30, 2018

bors-servo commented Jul 30, 2018

bors-servo commented Jul 30, 2018

bors-servo commented Jul 30, 2018

jdm commented Jul 30, 2018

bors-servo commented Jul 30, 2018

bors-servo commented Jul 30, 2018

paulrouget commented Jul 26, 2018 •

edited by atouchet

paulrouget commented Jul 26, 2018 •

edited