Suppress KeyUp events after PreHandleKeyboardEvent consumes RawKeyDown

content/browser/renderer_host/render_widget_host_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/render_widget_host_impl.h"
 
 #include <math.h>
 
 #include <set>
 #include <tuple>
@@ skipping 1125 matching lines @@
       suppress_next_char_events_ = true;
     return;
   }
 
   // Double check the type to make sure caller hasn't sent us nonsense that
   // will mess up our key queue.
   if (!WebInputEvent::isKeyboardEventType(key_event.type))
     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;
   }
 
   bool is_shortcut = false;
 
   // Only pre-handle the key event if it's not handled by the input method.
   if (delegate_ && !key_event.skip_in_browser) {
     // We need to set |suppress_next_char_events_| to true if
     // PreHandleKeyboardEvent() returns true, but |this| may already be
     // destroyed at that time. So set |suppress_next_char_events_| true here,
@@ skipping 1052 matching lines @@
   return delegate_ ? delegate_->GetRootBrowserAccessibilityManager() : NULL;
 }
 
 BrowserAccessibilityManager*
     RenderWidgetHostImpl::GetOrCreateRootBrowserAccessibilityManager() {
   return delegate_ ?
       delegate_->GetOrCreateRootBrowserAccessibilityManager() : NULL;
 }
 
 }  // namespace content