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 https://github.com/tomaka/winit/pull/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 -->

components/servo/lib.rs

@@ -367,10 +367,8 @@ impl<Window> Servo<Window> where Window: WindowMethods + 'static {
 
                 (EmbedderMsg::KeyEvent(ch, key, state, modified),
                  ShutdownState::NotShuttingDown) => {
-                    if state == KeyState::Pressed {
+                    let event = (top_level_browsing_context, EmbedderMsg::KeyEvent(ch, key, state, modified));
-                        let event = (top_level_browsing_context, EmbedderMsg::KeyEvent(ch, key, state, modified));
+                    self.embedder_events.push(event);
-                        self.embedder_events.push(event);
-                    }
                 },
 
                 (msg, ShutdownState::NotShuttingDown) => {

ports/servo/browser.rs

@@ -85,89 +85,92 @@ impl Browser {
 
     /// Handle key events before sending them to Servo.
     fn handle_key_from_window(&mut self, ch: Option<char>, key: Key, state: KeyState, mods: KeyModifiers) {
-        match (mods, ch, key) {
+        let pressed = state == KeyState::Pressed;
-            (CMD_OR_CONTROL, Some('r'), _) => {
+        // We don't match the state in the parent `match` because we don't want to do anything
-                if let Some(id) = self.browser_id {
+        // on KeyState::Released when it's a combo that we handle on Pressed. For example,
-                    self.event_queue.push(WindowEvent::Reload(id));
+        // if we catch Alt-Left on pressed, we don't want the Release event to be sent to Servo.
-                }
+        match (mods, ch, key, self.browser_id) {
+            (CMD_OR_CONTROL, _, Key::R, Some(id)) => if pressed {
+                self.event_queue.push(WindowEvent::Reload(id));
             }
-            (CMD_OR_CONTROL, Some('l'), _) => {
+            (CMD_OR_CONTROL, _, Key::L, Some(id)) => if pressed {
-                if let Some(id) = self.browser_id {
+                let url: String = if let Some(ref current_url) = self.current_url {
-                    let url: String = if let Some(ref current_url) = self.current_url {
+                    current_url.to_string()
-                        current_url.to_string()
+                } else {
-                    } else {
+                    String::from("")
-                        String::from("")
+                };
-                    };
+                let title = "URL or search query";
-                    let title = "URL or search query";
+                if let Some(input) = get_url_input(title, &url) {
-                    if let Some(input) = get_url_input(title, &url) {
+                    if let Some(url) = sanitize_url(&input) {
-                        if let Some(url) = sanitize_url(&input) {
+                        self.event_queue.push(WindowEvent::LoadUrl(id, url));
-                            self.event_queue.push(WindowEvent::LoadUrl(id, url));
-                        }
                     }
                 }
             }
-            (CMD_OR_CONTROL, Some('q'), _) => {
+            (CMD_OR_CONTROL, _, Key::Q, _) => if pressed {
                 self.event_queue.push(WindowEvent::Quit);
             }
-            (_, Some('3'), _) if mods ^ KeyModifiers::CONTROL == KeyModifiers::SHIFT => {
+            (_, Some('3'), _, _) if mods ^ KeyModifiers::CONTROL == KeyModifiers::SHIFT => if pressed {
                 self.event_queue.push(WindowEvent::CaptureWebRender);
             }
-            (KeyModifiers::CONTROL, None, Key::F10) => {
+            (KeyModifiers::CONTROL, None, Key::F10, _) => if pressed {
                 let event = WindowEvent::ToggleWebRenderDebug(WebRenderDebugOption::RenderTargetDebug);
                 self.event_queue.push(event);
             }
-            (KeyModifiers::CONTROL, None, Key::F11) => {
+            (KeyModifiers::CONTROL, None, Key::F11, _) => if pressed {
                 let event = WindowEvent::ToggleWebRenderDebug(WebRenderDebugOption::TextureCacheDebug);
                 self.event_queue.push(event);
             }
-            (KeyModifiers::CONTROL, None, Key::F12) => {
+            (KeyModifiers::CONTROL, None, Key::F12, _) => if pressed {
                 let event = WindowEvent::ToggleWebRenderDebug(WebRenderDebugOption::Profiler);
                 self.event_queue.push(event);
             }
-            (CMD_OR_ALT, None, Key::Right) | (KeyModifiers::NONE, None, Key::NavigateForward) => {
+            (CMD_OR_ALT, None, Key::Right, Some(id)) |
-                if let Some(id) = self.browser_id {
+            (KeyModifiers::NONE, None, Key::NavigateForward, Some(id)) => if pressed {
-                    let event = WindowEvent::Navigation(id, TraversalDirection::Forward(1));
+                let event = WindowEvent::Navigation(id, TraversalDirection::Forward(1));
-                    self.event_queue.push(event);
+                self.event_queue.push(event);
-                }
             }
-            (CMD_OR_ALT, None, Key::Left) | (KeyModifiers::NONE, None, Key::NavigateBackward) => {
+            (CMD_OR_ALT, None, Key::Left, Some(id)) |
-                if let Some(id) = self.browser_id {
+            (KeyModifiers::NONE, None, Key::NavigateBackward, Some(id)) => if pressed {
-                    let event = WindowEvent::Navigation(id, TraversalDirection::Back(1));
+                let event = WindowEvent::Navigation(id, TraversalDirection::Back(1));
-                    self.event_queue.push(event);
+                self.event_queue.push(event);
-                }
             }
-            (KeyModifiers::NONE, None, Key::Escape) => {
+            (KeyModifiers::NONE, None, Key::Escape, _) => if pressed {
                 self.event_queue.push(WindowEvent::Quit);
             }
             _ => {
-                let event = self.platform_handle_key(key, mods);
+                self.platform_handle_key(ch, key, mods, state);
-                self.event_queue.push(event.unwrap_or(WindowEvent::KeyEvent(ch, key, state, mods)));
             }
         }
-
     }
 
     #[cfg(not(target_os = "win"))]
-    fn platform_handle_key(&self, key: Key, mods: KeyModifiers) -> Option<WindowEvent> {
+    fn platform_handle_key(&mut self, ch: Option<char>, key: Key, mods: KeyModifiers, state: KeyState) {
+        let pressed = state == KeyState::Pressed;
         match (mods, key, self.browser_id) {
-            (CMD_OR_CONTROL, Key::LeftBracket, Some(id)) => {
+            (CMD_OR_CONTROL, Key::LeftBracket, Some(id)) => if pressed {
-                Some(WindowEvent::Navigation(id, TraversalDirection::Back(1)))
+                let event = WindowEvent::Navigation(id, TraversalDirection::Back(1));
+                self.event_queue.push(event);
             }
-            (CMD_OR_CONTROL, Key::RightBracket, Some(id)) => {
+            (CMD_OR_CONTROL, Key::RightBracket, Some(id)) => if pressed {
-                Some(WindowEvent::Navigation(id, TraversalDirection::Forward(1)))
+                let event = WindowEvent::Navigation(id, TraversalDirection::Back(1));
+                self.event_queue.push(event);
+            }
+            _ => {
+                self.event_queue.push(WindowEvent::KeyEvent(ch, key, state, mods));
             }
-            _ => None
         }
     }
 
     #[cfg(target_os = "win")]
-    fn platform_handle_key(&self, key: Key, mods: KeyModifiers) -> Option<WindowEvent> {
+    fn platform_handle_key(&mut self, _ch: Option<char>, _key: Key, _mods: KeyModifiers, _state: KeyState) {
-        None
     }
 
     /// Handle key events after they have been handled by Servo.
     fn handle_key_from_servo(&mut self, _: Option<BrowserId>, ch: Option<char>,
-                             key: Key, _: KeyState, mods: KeyModifiers) {
+                             key: Key, state: KeyState, mods: KeyModifiers) {
+        if state == KeyState::Pressed {
+            return;
+        }
         match (mods, ch, key) {
             (_, Some('+'), _) => {
                 if mods & !KeyModifiers::SHIFT == CMD_OR_CONTROL {

ports/servo/glutin_app/keyutils.rs

@@ -329,23 +329,3 @@ pub fn is_printable(key_code: VirtualKeyCode) -> bool {
     }
 }
 
-/// Detect if given char is default ignorable in unicode
-/// http://www.unicode.org/L2/L2002/02368-default-ignorable.pdf
-pub fn is_identifier_ignorable(ch: &char) -> bool {
-    match *ch {
-        '\u{0000}'...'\u{0008}' | '\u{000E}'...'\u{001F}' |
-            '\u{007F}'...'\u{0084}' | '\u{0086}'...'\u{009F}' |
-            '\u{06DD}' | '\u{070F}' |
-            '\u{180B}'...'\u{180D}' | '\u{180E}' |
-            '\u{200C}'...'\u{200F}' |
-            '\u{202A}'...'\u{202E}' | '\u{2060}'...'\u{2063}' |
-            '\u{2064}'...'\u{2069}' | '\u{206A}'...'\u{206F}' |
-            '\u{FE00}'...'\u{FE0F}' | '\u{FEFF}' |
-            '\u{FFF0}'...'\u{FFF8}' | '\u{FFF9}'...'\u{FFFB}' |
-            '\u{1D173}'...'\u{1D17A}' | '\u{E0000}' |
-            '\u{E0001}' |
-            '\u{E0002}'...'\u{E001F}' | '\u{E0020}'...'\u{E007F}' |
-            '\u{E0080}'...'\u{E0FFF}' => true,
-        _ => false
-    }
-}

ports/servo/glutin_app/window.rs

@@ -412,29 +412,23 @@ impl Window {
     }
 
     fn handle_received_character(&self, ch: char) {
-        let modifiers = keyutils::winit_mods_to_script_mods(self.key_modifiers.get());
+        let last_key = if let Some(key) = self.last_pressed_key.get() {
-        if keyutils::is_identifier_ignorable(&ch) {
+            key
-            return
-        }
-        if let Some(last_pressed_key) = self.last_pressed_key.get() {
-            let event = WindowEvent::KeyEvent(Some(ch), last_pressed_key, KeyState::Pressed, modifiers);
-            self.event_queue.borrow_mut().push(event);
         } else {
-            // Only send the character if we can print it (by ignoring characters like backspace)
+            return;
-            if !ch.is_control() {
+        };
-                match keyutils::char_to_script_key(ch) {
+
-                    Some(key) => {
-                        let event = WindowEvent::KeyEvent(Some(ch),
-                                                          key,
-                                                          KeyState::Pressed,
-                                                          modifiers);
-                        self.event_queue.borrow_mut().push(event);
-                    }
-                    None => {}
-                }
-            }
-        }
         self.last_pressed_key.set(None);
+
+        let (key, ch) = if let Some(key) = keyutils::char_to_script_key(ch) {
+            (key, Some(ch))
+        } else {
+            (last_key, None)
+        };
+
+        let modifiers = keyutils::winit_mods_to_script_mods(self.key_modifiers.get());
+        let event = WindowEvent::KeyEvent(ch, key, KeyState::Pressed, modifiers);
+        self.event_queue.borrow_mut().push(event);
     }
 
     fn toggle_keyboard_modifiers(&self, virtual_key_code: VirtualKeyCode) {
@@ -459,13 +453,14 @@ impl Window {
                 ElementState::Pressed => KeyState::Pressed,
                 ElementState::Released => KeyState::Released,
             };
-            if element_state == ElementState::Pressed {
+            if element_state == ElementState::Pressed && keyutils::is_printable(virtual_key_code) {
-                if keyutils::is_printable(virtual_key_code) {
+                // If pressed and printable, we expect a ReceivedCharacter event.
-                    self.last_pressed_key.set(Some(key));
+                self.last_pressed_key.set(Some(key));
-                }
+            } else {
+                self.last_pressed_key.set(None);
+                let modifiers = keyutils::winit_mods_to_script_mods(self.key_modifiers.get());
+                self.event_queue.borrow_mut().push(WindowEvent::KeyEvent(None, key, state, modifiers));
             }
-            let modifiers = keyutils::winit_mods_to_script_mods(self.key_modifiers.get());
-            self.event_queue.borrow_mut().push(WindowEvent::KeyEvent(None, key, state, modifiers));
         }
     }