From 33fb62bb259b05dcfd6f319328b2f90bd387df13 Mon Sep 17 00:00:00 2001
From: Simon Hausmann <hausmann@gmail.com>
Date: Thu, 29 Oct 2020 22:13:21 +0100
Subject: [PATCH] Fix WindowEvent::ReceivedCharacter on web (#1747)

* Fix WindowEvent::ReceivedCharacter on web

The event was never sent to the application because of the unconditional
preventDefault() call on keydown.

Fixes #1741

* Don't scroll when pressing space on a focused canvas

After reaching keypress, we should prevent further propagation.

Relates to #1741
---
 CHANGELOG.md                            |  1 +
 src/platform_impl/web/stdweb/canvas.rs  | 20 ++++++++++++++++----
 src/platform_impl/web/web_sys/canvas.rs | 14 +++++++++++++-
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5f75ec13d6..bf0ed4759d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,7 @@
 - On Wayland, fix window not being resizeable when using `with_min_inner_size` in `WindowBuilder`.
 - On Unix, fix cross-compiling to wasm32 without enabling X11 or Wayland.
 - On Windows, fix use after free crash during window destruction.
+- On Web, fix `WindowEvent::ReceivedCharacter` never being sent on key input.
 
 # 0.23.0 (2020-10-02)
 
diff --git a/src/platform_impl/web/stdweb/canvas.rs b/src/platform_impl/web/stdweb/canvas.rs
index cd952abe35..c68ec911aa 100644
--- a/src/platform_impl/web/stdweb/canvas.rs
+++ b/src/platform_impl/web/stdweb/canvas.rs
@@ -10,9 +10,9 @@ use stdweb::js;
 use stdweb::traits::IPointerEvent;
 use stdweb::unstable::TryInto;
 use stdweb::web::event::{
-    BlurEvent, ConcreteEvent, FocusEvent, FullscreenChangeEvent, IEvent, KeyDownEvent,
-    KeyPressEvent, KeyUpEvent, MouseWheelEvent, PointerDownEvent, PointerMoveEvent,
-    PointerOutEvent, PointerOverEvent, PointerUpEvent,
+    BlurEvent, ConcreteEvent, FocusEvent, FullscreenChangeEvent, IEvent, IKeyboardEvent,
+    KeyDownEvent, KeyPressEvent, KeyUpEvent, ModifierKey, MouseWheelEvent, PointerDownEvent,
+    PointerMoveEvent, PointerOutEvent, PointerOverEvent, PointerUpEvent,
 };
 use stdweb::web::html_element::CanvasElement;
 use stdweb::web::{document, EventListenerHandle, IElement, IEventTarget, IHtmlElement};
@@ -136,7 +136,17 @@ impl Canvas {
         F: 'static + FnMut(ScanCode, Option<VirtualKeyCode>, ModifiersState),
     {
         self.on_keyboard_press = Some(self.add_user_event(move |event: KeyDownEvent| {
-            event.prevent_default();
+            // event.prevent_default() would suppress subsequent on_received_character() calls. That
+            // supression is correct for key sequences like Tab/Shift-Tab, Ctrl+R, PgUp/Down to
+            // scroll, etc. We should not do it for key sequences that result in meaningful character
+            // input though.
+            let event_key = &event.key();
+            let is_key_string = event_key.len() == 1 || !event_key.is_ascii();
+            let is_shortcut_modifiers = (event.ctrl_key() || event.alt_key())
+                && !event.get_modifier_state(ModifierKey::AltGr);
+            if !is_key_string || is_shortcut_modifiers {
+                event.prevent_default();
+            }
             handler(
                 event::scan_code(&event),
                 event::virtual_key_code(&event),
@@ -155,6 +165,8 @@ impl Canvas {
         // viable/compatible alternative as of now. `beforeinput` is still widely
         // unsupported.
         self.on_received_character = Some(self.add_user_event(move |event: KeyPressEvent| {
+            // Supress further handling to stop keys like the space key from scrolling the page.
+            event.prevent_default();
             handler(event::codepoint(&event));
         }));
     }
diff --git a/src/platform_impl/web/web_sys/canvas.rs b/src/platform_impl/web/web_sys/canvas.rs
index 46d9531157..ae2fe0e186 100644
--- a/src/platform_impl/web/web_sys/canvas.rs
+++ b/src/platform_impl/web/web_sys/canvas.rs
@@ -157,7 +157,17 @@ impl Canvas {
         self.on_keyboard_press = Some(self.common.add_user_event(
             "keydown",
             move |event: KeyboardEvent| {
-                event.prevent_default();
+                // event.prevent_default() would suppress subsequent on_received_character() calls. That
+                // supression is correct for key sequences like Tab/Shift-Tab, Ctrl+R, PgUp/Down to
+                // scroll, etc. We should not do it for key sequences that result in meaningful character
+                // input though.
+                let event_key = &event.key();
+                let is_key_string = event_key.len() == 1 || !event_key.is_ascii();
+                let is_shortcut_modifiers =
+                    (event.ctrl_key() || event.alt_key()) && !event.get_modifier_state("AltGr");
+                if !is_key_string || is_shortcut_modifiers {
+                    event.prevent_default();
+                }
                 handler(
                     event::scan_code(&event),
                     event::virtual_key_code(&event),
@@ -179,6 +189,8 @@ impl Canvas {
         self.on_received_character = Some(self.common.add_user_event(
             "keypress",
             move |event: KeyboardEvent| {
+                // Supress further handling to stop keys like the space key from scrolling the page.
+                event.prevent_default();
                 handler(event::codepoint(&event));
             },
         ));