servo: Merge #20327 - fix(keyevent): do not emit default ignorable codepoint (from kwonoj:fix-ignore); r=paulrouget
authorOJ Kwon <kwon.ohjoong@gmail.com>
Thu, 22 Mar 2018 02:00:09 -0400
changeset 409534 4e150d44fea43fada64c447fa5ebabaf84eb748a
parent 409533 a960a84b229758cd075912bda84baa1c2e8247c7
child 409535 959eb9adc89add6d13faa9c7431e2eee31c0213a
push id101247
push usernerli@mozilla.com
push dateThu, 22 Mar 2018 23:00:51 +0000
treeherdermozilla-inbound@02e384bdf97d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspaulrouget
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #20327 - fix(keyevent): do not emit default ignorable codepoint (from kwonoj:fix-ignore); r=paulrouget <!-- Please describe your changes on the following line: --> This PR intends to update `KeyEvent` emit behavior around #18130. Issue https://github.com/servo/servo/issues/17146#issue-233361568 briefly explains what's happening in servo currently - there are KeyboardInput event emitted for separated key (modifier, and `V` in case of paste) and there are also `ReceivedCharacter` event corresponds to `\u{0016}`. `0x0016` is unicode representation of `Synchronous Idle` (https://en.wikipedia.org/wiki/Synchronous_Idle), belong under category of `Default Ignorable` charater in unicode range doesn't have visual representation (http://www.unicode.org/L2/L2002/02368-default-ignorable.pdf). Currently servo forwards all of emitted event from `winit` including this, eventually leads into double execution of control event. In this change try to omit default ignorable charater , if given char received is within range of ignorable do not dispatch `KeyEvent`. Once those are omitted, current event handling logic already takes care of key event with correct modifier state so duplicated event handling won't occur. For implementation perspective, `std::char` in Rust doesn't seem to support `isIdentifierIgnorable` like other platform does (i.e: https://msdn.microsoft.com/en-us/library/aa285330(v=vs.60).aspx / https://docs.oracle.com/javase/7/docs/api/java/lang/Character.html) - so does quick, naïve range comparison check based on unicode range specified in spec, similar to halfbuzz and other does. (https://github.com/harfbuzz/harfbuzz/blob/master/src/hb-unicode-private.hh#L134) Lastly, this is indeed behavior of `winit` to emit all characters by default. *Why not try to make upstream changes instead?* While I've been reading through issues in `winit`, issue like https://github.com/tomaka/winit/issues/350 trying to emit ignorable character by its intention (delete key `ReceivedCharacter` is also under category of default ignorable) and let each consumer application handles it as needed. I assume it'll cause breaking changes in winit's design if it intends to omit those characters, instead tried to make application-level changes. Couple of consideration for review - Is it desired changes to not emit `KeyEvent` for default ignorable chars? Do we rather want mapping / or restoring back to original char as @paulrouget mentioned in https://github.com/servo/servo/issues/17146#issue-233361568? - Any better, recommended approach to detect unicode char range? - Maybe try to make upstream changes to `winit` still, like having configuratble way to opt-in(out) those char event? --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #18130 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ - This PR has been locally tested on Windows, Linux machines. <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 28c92db26837531e75327cff7727ed8bc1c70b48
servo/ports/servo/glutin_app/window.rs
--- a/servo/ports/servo/glutin_app/window.rs
+++ b/servo/ports/servo/glutin_app/window.rs
@@ -341,18 +341,42 @@ impl Window {
         return GlRequest::Specific(Api::OpenGl, (3, 2));
     }
 
     #[cfg(any(target_arch = "arm", target_arch = "aarch64"))]
     fn gl_version() -> GlRequest {
         GlRequest::Specific(Api::OpenGlEs, (3, 0))
     }
 
+    /// Detect if given char is default ignorable in unicode
+    /// http://www.unicode.org/L2/L2002/02368-default-ignorable.pdf
+    fn is_identifier_ignorable(&self, 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
+        }
+    }
+
     fn handle_received_character(&self, ch: char) {
         let modifiers = Window::glutin_mods_to_script_mods(self.key_modifiers.get());
+        if self.is_identifier_ignorable(&ch) {
+            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)
             if !ch.is_control() {
                 match Window::char_to_script_key(ch) {
                     Some(key) => {