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

Text input is hard-wired to QWERTY #4144

Closed
SimonSapin opened this issue Nov 28, 2014 · 31 comments
Closed

Text input is hard-wired to QWERTY #4144

SimonSapin opened this issue Nov 28, 2014 · 31 comments
Assignees
Labels

Comments

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Nov 28, 2014

It seems that Servo receives keyboard event based on physical keys, and has its own interpretation of how to map they to text which assumes a QWERTY keyboard.

The fix is not to add support for more keyboard layouts. Servo should not be in the business of maintaining a repository of all keyboard layouts in the world, this is the job of the operating system. Servo should have the operating system give it Unicode text in a way that supports whatever input method (a.k.a. IME) the operating system may or may not have.

CC @jdm

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Nov 28, 2014

With GLFW, we’re currently using KeyEvent, but CharEvent also exists: http://doc.servo.org/glfw/enum.WindowEvent.html

glutin similarly has both KeyboardInput and ReceivedCharacter events.

@jdm
Copy link
Member

@jdm jdm commented Nov 28, 2014

Yes, moving to CharEvent is generally desirable, but IIRC there were some downsides (like the lack of separate press and release events, I believe).

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Nov 28, 2014

Is press/release relevant to text input? Repeating a character on long press is also up to the OS, I believe. And this is only forming the text of text input, I don’t mean that all KeyEvent usage needs to be removed. (E.g. arrow keys and backspace probably still should be key events rather than characters.)

@jdm
Copy link
Member

@jdm jdm commented Nov 29, 2014

Right, I spent a while looking into this back after I first wrote the text input code. The current implementation uses the keypress DOM event to trigger text input actions (as dictated by the spec). However, this event comes between the keydown and keyup events, all of which must share the same key code values. It made sense to base all of this off of GLFW's KeyEvents, which provide press/release/repeat information. CharEvent has none of those data available, and it seems like it could be complicated to note a KeyEvent, decide whether or not to delay a DOM event until a corresponding CharEvent, and then figure out the corresponding prior CharEvent that should be used for the basis of a subsequent KeyEvent indicating key release.

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Nov 29, 2014

Ok, I just spent some time looking at https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#keys and yeah, it’s complicated.

I think that the glfw::Key enum in KeyEvent should map to KeyboardEvent.code in the DOM event, but KeyboardEvent.key is maybe kinda closer to what’s in a CharEvent.

So maybe we need the windowing toolkit to give both pieces of information together rather than as separate events?

@jdm
Copy link
Member

@jdm jdm commented Nov 29, 2014

That would be ideal, yes.

@glennw
Copy link
Member

@glennw glennw commented Nov 29, 2014

Once glutin is default on all platforms, and we remove GLFW, I'll revisit this and made modifications to glutin as required to support what we need.

@metajack
Copy link
Contributor

@metajack metajack commented Dec 1, 2015

@glennw Now that glutin is default, perhaps the time has come to at least document here what we should be doing.

@adrianheine
Copy link
Contributor

@adrianheine adrianheine commented Jan 2, 2016

This also applies to for example zooming. With a German keyboard layout, I cannot zoom out, since my = is actually SHIFT | CONTROL Num0.

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Feb 2, 2016

Can someone explain what needs to be done here?

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Feb 2, 2016

@paulrouget I think we want to handle Event::ReceivedCharacter(char) events from glutin as well as Event::KeyboardInput(ElementState, ScanCode, Option<VirtualKeyCode>), but there’s probably a number of places in Servo to change.

@glennw You said you wanted to do this? :)

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Feb 5, 2016

@SimonSapin can we save the character received in Event::ReceivedCharacter, and in Event::KeyboardInput, use the saved character to build the Key instead of key_code (via glutin_key_to_script_key)?

We could probably reuse key_from_string:

fn key_from_string(key_string: &str, location: u32) -> Option<Key> {

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Feb 5, 2016

@paulrouget I think this would only work for ASCII at best. Most Unicode characters don’t have a corresponding key code.

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Feb 5, 2016

Right.

ReceivedCharacter doesn't work for many characters apparently (at least on mac). For example the é key or ``+e->é` doesn't trigger a `ReceivedCharacter` event. I'm wondering if that works on Linux and Windows.

Then, the KeyEvent should have a char_code on KeyState::Released, based on the ReceivedCharacter event.

Then the input can be fed with the charcode.

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Feb 5, 2016

I was just speculating about ReceivedCharacter. I’ve filed rust-windowing/glutin#719

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Feb 5, 2016

@paul, it sounds like you’re hitting glutin bugs. Could you file steps to reproduce on https://github.com/tomaka/glutin/issues ?

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Feb 8, 2016

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Feb 8, 2016

So we need to bring the character down to textinput.rs.

It should be enough to initialize the keyboard event with this character + virtual keycode.

Keyboard events are initialized with the help of the key_properties function:

pub fn key_properties(key: Key, mods: KeyModifiers)

Here, relevant properties are set.

From what I understand:

  • key_string: it should use the character. If there's no character, this should be derived from the virtual keycode. This will be used in textinput.rs to print the apropriate character
  • code: it should use the virtual keycode
  • location: it should use the virtual keycode
  • char_code: it should use the character
  • key_code: if character is an ascii character, it should use the character, otherwise, the keycode

I managed to bring the character value to this function (see https://gist.github.com/paulrouget/668c3dcdb7dbfed7909a). But then I'm having troubles with strings.

from_string is a static str. We need a string (because it's not always a character).

What is the appropriate type? String?

Can someone tell me if all of that makes sense?

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Feb 8, 2016

So we need to bring the character down to textinput.rs.

It should be enough to initialize the keyboard event with this character + virtual keycode.

How would this be done? Event::ReceivedCharacter is separate from Event::KeyboardInput in glutin, and as far as I know they don’t map 1 to 1: a single character sometimes requires more than one key press to type, and a key press sequence could overall result in more than one char at once e.g. in complex input methods like Japanese.

Shouldn’t we rather bring both kinds of events separately to textinput.rs?

@jdm
Copy link
Member

@jdm jdm commented Feb 8, 2016

@paul I don't understand your questions about from_string and String, unfortunately. At least, I don't understand how they're related to the gist.

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Feb 9, 2016

Erf, I meant key_string, not from_string. My comment is not clear. I'll come back with more specific questions.

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Feb 9, 2016

Shouldn’t we rather bring both kinds of events separately to textinput.rs?

We could.

I am assuming that there's always a KeyboardInput event for a ReceivedCharacter event (the inverse is not true). So attaching the character to the keyboard event might be enough.

How would this be done? Event::ReceivedCharacter is separate from Event::KeyboardInput in glutin, and as far as I know they don’t map 1 to 1: a single character sometimes requires more than one key press to type, and a key press sequence could overall result in more than one char at once e.g. in complex input methods like Japanese.

I'm not sure glutin supports that. ReceivedCharacter is only one char.

If we want to support complex input methods (IME), we need to support composition events.

My approach is just a quick-and-dirty way to support azerty layout (we have developers who use this layout, and the hardcoded qwerty-layout is a bit annoying for them :) ).

IME and composition events support is a lot of work I imagine. Is that a thing we are willing to work on soon? If not, is it ok to implement my suggested approach of forwarding the character to textinput.rs via the keyboard event?

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Mar 3, 2016

Apparently, this is not an issue on Linux.

@adrianheine
Copy link
Contributor

@adrianheine adrianheine commented Mar 3, 2016

It was for me on January, 2nd.

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Mar 3, 2016

It was for me on January, 2nd.

Oh, I thought @mbrubeck could type with dvorak on Linux.

@mbrubeck
Copy link
Contributor

@mbrubeck mbrubeck commented Mar 3, 2016

Yes, everything seems fine on my Linux laptop using X11 with a Dvorak keymap.

@adamncasey
Copy link
Contributor

@adamncasey adamncasey commented Mar 19, 2016

On Windows, quite a few ASCII key events (e.g. :~@;'# ) aren't assigned virtual key codes in glutin (See rust-windowing/glutin#744 ). Servo currently ignores all key events which don't have a virtual key code:

if virtual_key_code.is_some() {

Should I file a separate bug for this problem, or will the work here address this?

@adamncasey
Copy link
Contributor

@adamncasey adamncasey commented Mar 20, 2016

Also, from glancing at the glutin code, X11 virtual key codes are much more specific than Windows / OS X / Wayland(?) - which might explain why some keymaps are OK and some aren't.

X11: https://github.com/tomaka/glutin/blob/master/src/api/x11/events.rs#L5
Cocao: https://github.com/tomaka/glutin/blob/master/src/api/cocoa/event.rs
Windows: https://github.com/tomaka/glutin/blob/master/src/api/win32/event.rs

@tdaede
Copy link

@tdaede tdaede commented Mar 29, 2016

Yes, X11 and Wayland both use libxcbcommon now, which has "keycodes" which are independent of the labels on the keyboard, and "keysyms" that take the layout into account. The glutin code uses keysyms for its key events.

This is still not sufficient for a fully featured text field though - IME's currently don't work at all, meaning no CJK text input.

@metajack
Copy link
Contributor

@metajack metajack commented Apr 18, 2016

@SimonSapin mentioned he might have a look at this.

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jun 21, 2016

If we ever want to release a developer review of servo+bhtml, we need to do something about this issue.
Getting IME and composition events to work will require a lot of time and is not something we can get done in time.

But there's probably a way to improve the situation, like using ReceivedCharacter with KeyboardInput to at least try to get the right mono-key characters.

P3 -> P1

@jdm jdm self-assigned this Jun 29, 2016
@jdm jdm mentioned this issue Jun 30, 2016
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Jul 6, 2016
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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11950)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 6, 2016
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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11950)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 6, 2016
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.

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

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