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

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

Closed
paulrouget opened this issue Jul 11, 2018 · 5 comments
Closed

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

paulrouget opened this issue Jul 11, 2018 · 5 comments
Labels

Comments

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jul 11, 2018

Maybe because of #21087

@paulrouget paulrouget mentioned this issue Jul 11, 2018
0 of 5 tasks complete
@paulrouget paulrouget changed the title Keybindings don't work on Linux (and maybe windows) Keybindings don't work on Linux and maybe windows Jul 25, 2018
@paulrouget paulrouget removed the P-linux label Jul 25, 2018
@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jul 25, 2018

I'm going to try to tackle that one, including #17146. We accumulated a lot of workarounds and winit has changed quite a bit (fixed some stuff, broke some other stuff).

There are few bugs in winit on Mac where key down events for modifiers are not sent correctly.
The behavior for control keys is different on Windows.
And ReceivedCharacter is not always fired the same way depending on the platform.

My goal is to achieve the same behavior across the 3 platforms for:

  • basic typing
  • typing with a different layout (testing with Azerty)
  • getting return, backspace and arrows to work
  • get Cmd-A and Ctrl-A to work in a textbox
  • make clipboard shortcut work
  • make sure Ctrl/Cmd-XXX shortcuts are caught and not repeated
@dorfsmay
Copy link

@dorfsmay dorfsmay commented Jul 25, 2018

I've had a look but made very little progress. I'm not sure how useful this will be, but just in case, here's what I have found on Linux:

Everything happens in ports/servo/glutin_app/window.rs and components/servo/lib.rs

  • the run function in windows.rs does a callback to self.winit_event_to_servo_event on each window events.

  • printing out the event (e) in the events_loop, before the callback shows that all events are the same at this point, Key::KeyboardInput

  • the Window::handle_events interprets normal alphabet and Alt keys as [Key], but interprets the control key as a [MouseMove]!

  • I think the issue is that winit_event_to_servo_event (in ports/servo/glutin_app/window.rs) is matching for winit::WindowEvent::KeyboardInput but the Control key is still a winit::Key::KeyboardInput. I had some code that showed this, but removed it since, and haven't had time to dig there since then.

  • the control keys create a sticky state: If you press CTRL -X (x or any letter it doesn't matter) once, then all keys pressed after that show ctrl: true as a ModifiersState. The only way to change that state back to false is to again press CTRL with a key. Pressing CTRL on its own will not change the state.

  • This point above explains why CTRL-L in servo on Linux does not bring the URL menu. You can get the URL menu following this sequence:

    • start servo
    • press CTRL-L (not necessarily L, any letter at the same time as CTRL)
    • press the key 'L' on its own
    • press CTRL-L (any letter) again to switch away from the control modifier
@dorfsmay
Copy link

@dorfsmay dorfsmay commented Jul 25, 2018

This is a printout from adding a bunch of debugging println when pressing the following keys in this order:

  • a
  • b
  • c
  • ctrl
  • D
  • ctrl-z
  • e (check ModifiersState)
  • ctrl-x
  • f (check ModifiersState now!)
  • alt (shows as a key!)
  • g (`ModifierState is what you'd expect)
  • atl-c (again, `ModifierState is what you'd expect)
  • h (and yet again, `ModifierState is what you'd expect)
  • esc key

I can push my branch that I used to generate this, but it is just a few println in the functions mentioned above

key-press-printout.txt

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jul 26, 2018

@dorfsmay thank you. I think I might need help later with extensive testing.

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jul 26, 2018

PR: #21250

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 -->
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.

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