Suppress KeyUp events after PreHandleKeyboardEvent consumes RawKeyDown

content/browser/renderer_host/render_widget_host_impl.cc

Index: content/browser/renderer_host/render_widget_host_impl.cc
diff --git a/content/browser/renderer_host/render_widget_host_impl.cc b/content/browser/renderer_host/render_widget_host_impl.cc
index 2335a60fa9e27504129300f8935b3a8e6eef7961..0f1f6b9d5b1140b71aa5b4840c547ad85cc165ab 100644
--- a/content/browser/renderer_host/render_widget_host_impl.cc
+++ b/content/browser/renderer_host/render_widget_host_impl.cc
@@ -1143,13 +1143,14 @@ void RenderWidgetHostImpl::ForwardKeyboardEvent(
     return;
 
   if (suppress_next_char_events_) {
-    // If preceding RawKeyDown event was handled by the browser, then we need
-    // suppress all Char events generated by it. Please note that, one
-    // RawKeyDown event may generate multiple Char events, so we can't reset
-    // |suppress_next_char_events_| until we get a KeyUp or a RawKeyDown.
-    if (key_event.type == WebKeyboardEvent::Char)
+    // If the preceding RawKeyDown event was handled by the browser, then we
+    // need to suppress all events generated by it until the next RawKeyDown or

[dtapuska, 2016/11/07 16:00:44]

The member variable name is slightly confusing now. At least the comment here:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
needs an update.

Yes it could happen in the render_widget_input_handler as well. The OS is not
necessarily guaranteed to send us a logical stream of events so having this
state related code is somewhat dangerous.

I'd prefer that we used a model in which we still fired the events at javascript
but actually indicated that they weren't cancelable and then we'd just look up
each time if the mapping is a shortcut for down, char and up instead of having
state. But I have no idea how inter-operable that would be.

[Matt Giuca, 2016/11/08 03:04:15]

+1
Do you mean the OS could send a KeyUp before the corresponding RawKeyDown? I've
never heard of that happening... do you have information?

Maybe you're talking about interleaved key events (press A, press B, release A,
release B). I'll mention that in a separate comment.
 
I would rather not fire the keyup at JS at all if we can help it. The keydown is
being intercepted so we should suppress the corresponding keyup also.

+    // KeyDown event.
+    if (key_event.type == WebKeyboardEvent::KeyUp ||
+        key_event.type == WebKeyboardEvent::Char)

[Matt Giuca, 2016/11/08 03:04:15]

So I thought about the case where you interleave two key presses like this:

1. Press A [captured by browser]
2. Press B
3. Release A
4. Release B

What happens is that neither Release A nor Release B are suppressed, because
Press B sets suppress_next_char_events_ to false.

That's a little bit dodgy; really Release A should still be suppressed. But it'd
be hard to keep track of all the state required, so I am happy with this
behaviour. It solves the main case of a single keypress.

There's also another edge case: if you hold down the key long enough to trigger
a key repeat:

1. Press A.
2. Hold A, triggering a second keydown event.
3. Release A.

The release does not get suppressed, because the second keydown event sets
suppress_next_char_events_ to false. I'm also happy with this behaviour, because
you are legitimately delivering the key repeat to the web page and it isn't
being suppressed the second time (in the case of Esc to exit fullscreen).

So, no action required. Just noting a few things.

       return;
-    // We get a KeyUp or a RawKeyDown event.
+    DCHECK(key_event.type == WebKeyboardEvent::RawKeyDown ||
+           key_event.type == WebKeyboardEvent::KeyDown);
     suppress_next_char_events_ = false;
   }