Handle KeyboardEvent without virtual key code #15800

charlesvdv · 2017-03-02T20:10:07Z

This PR allows to properly handling KeyboardEvent without a virtual key code.

These PR is not completed yet but I prefer start a discussion to see if my approach is correct to you. (For exemple I have to remove the Result over here).

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15240 (github issue number if applicable).
Theses changes should fix #12616

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-03-02T20:10:12Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/keyboardevent.rs
@KiChjang: components/script/dom/keyboardevent.rs

highfive · 2017-03-02T20:10:13Z

Warning

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

KiChjang · 2017-03-02T20:13:39Z

ports/glutin/window.rs

+                    Some(VirtualKeyCode::LAlt) => self.toggle_modifier(LEFT_ALT),
+                    Some(VirtualKeyCode::RAlt) => self.toggle_modifier(RIGHT_ALT),
+                    Some(VirtualKeyCode::LWin) => self.toggle_modifier(LEFT_SUPER),
+                    Some(VirtualKeyCode::RWin) => self.toggle_modifier(RIGHT_SUPER),


Why is this an improvement?

This is the improvements because now we are handling correctly the case where virtual key code is None.

KiChjang · 2017-03-02T20:18:18Z

ports/glutin/window.rs

@@ -345,16 +345,16 @@ impl Window {
                    self.pending_key_event_char.set(Some(ch));
                }
            }
-            Event::KeyboardInput(element_state, scan_code, Some(virtual_key_code)) => {


Removing the match arm you linked should just be fine, why did you need to remove this Some()?

Oh I see, looks like you're rolling in functionality for handling Some and None into the same match arm. Would it not be ok to just make a separate match arm instead specifically for the None case?

Because before we were just handling the case with Some(virtual_key) but now we handle the case None (which doesn't change much the code so I put both on the same case).

I'm actually more of the opinion that we should split this up into different match arms. The common code between the two match arms should just be refactored into a separate function, since this arm is already pretty big as it is. What do you think?

I'm totally ok with that!

KiChjang · 2017-03-02T20:26:31Z

ports/glutin/window.rs

-            Event::KeyboardInput(_, _, None) => {
-                debug!("Keyboard input without virtual key.");
+                let key = match virtual_key_code {
+                    Some(v) => Window::glutin_key_to_script_key(v).unwrap(),


Why is it OK to assume the unwrap wouldn't panic?

Because I will remove the Result from here but before doing so I prefer to see if you are ok with the changes 😄

Can't seem to follow the link you posted.

Oh sorry. This one should works!

KiChjang · 2017-03-03T22:11:35Z

Changes look good to me, but I see that @jdm has raised some questions in the relevant issue. Handing this over to him for approval.
r? @jdm

jdm · 2017-03-04T04:27:25Z

I'm not available for reviewing code right now, so I'm going to hand this off to @Ms2ger.
r? @Ms2ger


        Store non-printable characters in the pressed_key_map.


        Factor out get_keyboard_input_char().


        Factor out send_key_event().


        Handle KeyboardEvent without virtual key code

Ms2ger

I found it pretty hard to review, so I tried to split up your the refactoring from the actual changes: https://github.com/servo/servo/tree/keyboard-virtual.

Would you mind pushing that branch to your PR? That should help to evaluate it.

Ms2ger · 2017-03-07T10:34:19Z

ports/glutin/window.rs

+    fn get_keyboard_input_char(&self, element_state: ElementState, scan_code: ScanCode) -> Option<char> {
+        match element_state {
+            ElementState::Pressed => {
+                // Retrieve any previosly stored ReceivedCharacter value.


"previously"

Ms2ger · 2017-03-07T10:34:19Z

ports/glutin/window.rs

+                // Store the association between the scan code and the actual
+                // character value, if there is one.
+                let ch = self.pending_key_event_char
+                             .get();


Can go on the previous line.


        fix code review comment

nox · 2017-04-11T08:35:26Z

That changes from your last fixup commit needs to be properly squashed into the commits where they belong.

nox · 2017-04-11T08:37:49Z

You don't need Key::Unidentified. Instead keep returning Result<Key, ()>, and use an Option<Key> with None where you used Key::Unidentified.

bors-servo · 2017-04-12T13:46:47Z

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

nox · 2017-10-09T10:53:47Z

@jonathandturner Think you could take a look at this? It became hard to rebase thanks to #16198.

jonathandturner · 2017-10-09T12:35:41Z

@nox - apologies. Wish I could help, though unfortunately too busy/out-of-the-loop to help fix.

dralley · 2020-04-20T21:19:27Z

I think this PR (as well as possibly the mentioned issue) are obsolete, it seems like Servo's keyboard event handling was made more spec-compliant by 0ccaa7e#diff-8dc9268416cd4f037d50206e76c73cf9

The linked issue also talks about Glutin limitations, I'm not sure if that is still relevant anymore either.

jdm · 2020-04-20T22:13:32Z

It certainly could still be relevant, since glutin just exported a bunch of input-related types and APIs from winit. We still rely on winit, so we probably still have the same input-related problems. However, this PR is not receiving any attention so I'm going to go ahead and close it.

highfive assigned metajack Mar 2, 2017

highfive added the S-awaiting-review label Mar 2, 2017

KiChjang reviewed Mar 2, 2017

View changes

metajack assigned KiChjang and unassigned metajack Mar 2, 2017

KiChjang added S-needs-code-changes and removed S-awaiting-review labels Mar 3, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 3, 2017

highfive assigned jdm and unassigned KiChjang Mar 3, 2017

highfive assigned Ms2ger and unassigned jdm Mar 4, 2017

charlesvdv added 4 commits Mar 7, 2017

Store non-printable characters in the pressed_key_map.

2d7a9fb

Factor out get_keyboard_input_char().

abde4c0

Factor out send_key_event().

89ff8c0

Handle KeyboardEvent without virtual key code

Loading status checks…

bebf655

Ms2ger reviewed Mar 7, 2017

View changes

fix code review comment

Loading status checks…

29ac79e

charlesvdv force-pushed the charlesvdv:fix-keyboard branch from fb6d5d5 to 29ac79e Mar 8, 2017

nox assigned nox and unassigned Ms2ger Mar 28, 2017

nox added S-needs-code-changes and removed S-awaiting-review labels Apr 11, 2017

highfive added the S-needs-rebase label Apr 12, 2017

jdm closed this Apr 20, 2020

servo / servo

Handle KeyboardEvent without virtual key code #15800

Handle KeyboardEvent without virtual key code #15800

charlesvdv commented Mar 2, 2017 •

edited by larsbergstrom

highfive commented Mar 2, 2017

highfive commented Mar 2, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Mar 3, 2017

jdm commented Mar 4, 2017

Ms2ger left a comment

This comment has been minimized.

This comment has been minimized.

nox commented Apr 11, 2017

nox commented Apr 11, 2017

bors-servo commented Apr 12, 2017

nox commented Oct 9, 2017

jonathandturner commented Oct 9, 2017

dralley commented Apr 20, 2020

jdm commented Apr 20, 2020

servo / servo

Join GitHub today

Handle KeyboardEvent without virtual key code #15800

Handle KeyboardEvent without virtual key code #15800

Conversation

charlesvdv commented Mar 2, 2017 • edited by larsbergstrom

highfive commented Mar 2, 2017

highfive commented Mar 2, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Mar 3, 2017

jdm commented Mar 4, 2017

Ms2ger left a comment

This comment has been minimized.

This comment has been minimized.

nox commented Apr 11, 2017

nox commented Apr 11, 2017

bors-servo commented Apr 12, 2017

nox commented Oct 9, 2017

jonathandturner commented Oct 9, 2017

dralley commented Apr 20, 2020

jdm commented Apr 20, 2020

charlesvdv commented Mar 2, 2017 •

edited by larsbergstrom