Unstrangle unhandled key event callback for webview

Unstrangle unhandled key event callback for webview

Currently, when typing on an input form using a BT keyboard or
when InputConnection#sendKeyEvent() is used, we end up getting unhandled
ACTION_UP event in WebViewClient#onUnhandledInputEvent(). Here's what
happens:
1) RawKeyDown event is not processed: we create Char event in
  ime_adapter_android.cc and process it instead.
  So RenderWidgetHostImpl::OnKeyboardEventAck() should normally propagate
  to onUnhandledInputEvent() as the event was not processed. But
  we've set the skip_in_browser flag in ime_adapter_android.cc so
  OnKeyboardEventAck() does not propagate this event.
2) Char event is processed.
3) KeyUp event is not processed, and OnKeyboardEventAck() propagates the
  event, and WebViewClient#onUnhandledInputEvent() is called.

The last one is an incorrect behavior, and we get unpaired
onUnhandledInputEvent() callback only for ACTION_UP.

Also, when ACTION_DOWN / ACTION_UP are not handled (when no input form is
focused), the following happens:
1) RawKeyDown event is not processed. But we set the skip_in_browser flag
  so OnKeyboardEventAck() does not propagate the event.
  (This is an incorrect behavior.)
2) Char event is not created.
3) KeyUp event is not processed, and OnKeyboardEventAck() propagates the
  event, and onUnhandledInputEvent() is called.

We still get unpaired onUnhandledInputEvent() callback, only for ACTION_UP.

The first problem here is that return value of handleInputEvent() for
KeyUp event is different from that for RawKeyDown or Char events.

Another problem is that we sometimes create Char event and sometimes we
do not, but Char event cannot deliver (propagate) platform-specific event.

So for a proper fix, we need the following changes:
- When Char event is created, we set os_event such that if Char event is
  not processed, we make sure onUnhandledInputEvent() is called.
- Change EventHandler::keyEvent to return the same value for KeyUp event
  as the value returned for RawKeyDown or Char event.

BUG=538377

[Changwan Ryu, 2015-10-02 17:29:05]

changwan@chromium.org changed reviewers:
+ aelias@chromium.org, jdduke@chromium.org

[jdduke (slow), 2015-10-02 17:51:29]

Any simple way to test this?

[aelias_OOO_until_Jul13, 2015-10-02 20:11:51]

https://codereview.chromium.org/1378583003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/1378583003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/ime_adapter_android.cc:154: event.skip_in_browser
= true;
"true" seems unnecessarily broad.  That will suppress browser handling of all
key events (for example, F1-F10 keys), but the intent of benm's patch was just
to suppress the events which are duplicative with a Char event.  I think we
should only broaden suppression to Up events that also correspond to a Char
event.  So I suggest writing: "event.skip_in_browser = event.text[0];" instead.

[Changwan Ryu, 2015-10-05 12:59:32]

On 2015/10/02 20:11:51, aelias wrote:
>
https://codereview.chromium.org/1378583003/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/ime_adapter_android.cc (right):
> 
>
https://codereview.chromium.org/1378583003/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/ime_adapter_android.cc:154:
event.skip_in_browser
> = true;
> "true" seems unnecessarily broad.  That will suppress browser handling of all
> key events (for example, F1-F10 keys), but the intent of benm's patch was just
> to suppress the events which are duplicative with a Char event.  I think we
> should only broaden suppression to Up events that also correspond to a Char
> event.  So I suggest writing: "event.skip_in_browser = event.text[0];"
instead.

Hmm... I tried it, but we get onUnhandledKeyEvent only for the KeyUp event when
it is a delete key. Investigating more.

[Changwan Ryu, 2015-12-02 00:49:26]

Description was changed from

==========
Fix KeyEvent.ACTION_UP not to call WebViewClient#onUnhandledKeyEvent()

When typing using a BT keyboard or InputConnection#sendKeyEvent() is used,
we end up getting an additional ACTION_UP event because
- we do not normally process ACTION_DOWN / ACTION_UP
- for ACTION_DOWN, we set skip_in_browser to be true so that
  OnKeyboardEventAck() does not propagate the value
- for ACTION_UP, we do not set the value so OnKeyboardEventAck() propagates
  the value to WebViewClient#onUnhanhandledKeyEvent() whose default
  implementation is to dispatch the event to the View root

This was introduced two years ago: https://codereview.chromium.org/17074005

BUG=538377
==========

to

==========
Unstrangle unhandled key event callback for webview

When typing using a BT keyboard or InputConnection#sendKeyEvent() is used,
we end up getting unhandled ACTION_UP event. This happens because
- RawKeyDown event is not processed: we create Char event and process it
  instead. But we set the skip_in_browser flag in ime_adapter_android.cc and
  RenderWidgetHostImpl::OnKeyboardEventAck() does not propagate the event.
- Char event is processed.
- KeyUp event is not processed, and OnKeyboardEventAck() propagates the
  event, and WebViewClient#onUnhandledInputEvent() is called.
  (This is an incorrect behavior.)

Also, when ACTION_DOWN / ACTION_UP are not handled, the following happens:
- RawKeyDown event is not processed. But we set the skip_in_browser flag
  so OnKeyboardEventAck() does not propagate the event.
  (This is an incorrect behavior.)
- Char event is not created.
- KeyUp event is not processed, and OnKeyboardEventAck() propagates the
  event, and WebViewClient#onUnhandledInputEvent() is called.

For a proper fix, there are two things that should be changed:
- When Char event is created, we set os_event such that if Char event is
  not processed, we make sure onUnhandledInputEvent() is called.
- Change EventHandler::keyEvent to return the same value for KeyUp event
  as the value returned for RawKeyDown or Char event.

BUG=538377
==========

[Changwan Ryu, 2015-12-02 00:51:04]

There are some tests that should be added, and some chrome os test breakage that
needs to be investigated, but could you take an initial look? Does this approach
make sense?

[Changwan Ryu, 2015-12-02 01:02:27]

Description was changed from

==========
Unstrangle unhandled key event callback for webview

When typing using a BT keyboard or InputConnection#sendKeyEvent() is used,
we end up getting unhandled ACTION_UP event. This happens because
- RawKeyDown event is not processed: we create Char event and process it
  instead. But we set the skip_in_browser flag in ime_adapter_android.cc and
  RenderWidgetHostImpl::OnKeyboardEventAck() does not propagate the event.
- Char event is processed.
- KeyUp event is not processed, and OnKeyboardEventAck() propagates the
  event, and WebViewClient#onUnhandledInputEvent() is called.
  (This is an incorrect behavior.)

Also, when ACTION_DOWN / ACTION_UP are not handled, the following happens:
- RawKeyDown event is not processed. But we set the skip_in_browser flag
  so OnKeyboardEventAck() does not propagate the event.
  (This is an incorrect behavior.)
- Char event is not created.
- KeyUp event is not processed, and OnKeyboardEventAck() propagates the
  event, and WebViewClient#onUnhandledInputEvent() is called.

For a proper fix, there are two things that should be changed:
- When Char event is created, we set os_event such that if Char event is
  not processed, we make sure onUnhandledInputEvent() is called.
- Change EventHandler::keyEvent to return the same value for KeyUp event
  as the value returned for RawKeyDown or Char event.

BUG=538377
==========

to

==========
Unstrangle unhandled key event callback for webview

Currently, when typing on an input form using a BT keyboard or
when InputConnection#sendKeyEvent() is used, we end up getting unhandled
ACTION_UP event in WebViewClient#onUnhandledInputEvent(). Here's what
happens:
- RawKeyDown event is not processed: we create Char event in
  ime_adapter_android.cc and process it instead.
  So RenderWidgetHostImpl::OnKeyboardEventAck() should normally propagate
  to onUnhandledInputEvent() as the event as it was not processed. But
  we've set the skip_in_browser flag in ime_adapter_android.cc so
  OnKeyboardEventAck() does not propagate this event.
- Char event is processed.
- KeyUp event is not processed, and OnKeyboardEventAck() propagates the
  event, and WebViewClient#onUnhandledInputEvent() is called.

The last one is an incorrect behavior, and we get unpaired
onUnhandledInputEvent() callback only for ACTION_UP.

Also, when ACTION_DOWN / ACTION_UP are not handled (when no input form is
focused), the following happens:
- RawKeyDown event is not processed. But we set the skip_in_browser flag
  so OnKeyboardEventAck() does not propagate the event.
  (This is an incorrect behavior.)
- Char event is not created.
- KeyUp event is not processed, and OnKeyboardEventAck() propagates the
  event, and onUnhandledInputEvent() is called.

We still get unpaired onUnhandledInputEvent() callback, only for ACTION_UP.

The first problem here is that return value of handleInputEvent() for
KeyUp event is different from that for RawKeyDown or Char events.

Another problem is that we sometimes create Char event and sometimes we
do not, but Char event cannot deliver (propagate) platform-specific event.

So for a proper fix, we need the following changes:
- When Char event is created, we set os_event such that if Char event is
  not processed, we make sure onUnhandledInputEvent() is called.
- Change EventHandler::keyEvent to return the same value for KeyUp event
  as the value returned for RawKeyDown or Char event.

BUG=538377
==========

[Changwan Ryu, 2015-12-02 01:04:42]

Description was changed from

==========
Unstrangle unhandled key event callback for webview

Currently, when typing on an input form using a BT keyboard or
when InputConnection#sendKeyEvent() is used, we end up getting unhandled
ACTION_UP event in WebViewClient#onUnhandledInputEvent(). Here's what
happens:
- RawKeyDown event is not processed: we create Char event in
  ime_adapter_android.cc and process it instead.
  So RenderWidgetHostImpl::OnKeyboardEventAck() should normally propagate
  to onUnhandledInputEvent() as the event as it was not processed. But
  we've set the skip_in_browser flag in ime_adapter_android.cc so
  OnKeyboardEventAck() does not propagate this event.
- Char event is processed.
- KeyUp event is not processed, and OnKeyboardEventAck() propagates the
  event, and WebViewClient#onUnhandledInputEvent() is called.

The last one is an incorrect behavior, and we get unpaired
onUnhandledInputEvent() callback only for ACTION_UP.

Also, when ACTION_DOWN / ACTION_UP are not handled (when no input form is
focused), the following happens:
- RawKeyDown event is not processed. But we set the skip_in_browser flag
  so OnKeyboardEventAck() does not propagate the event.
  (This is an incorrect behavior.)
- Char event is not created.
- KeyUp event is not processed, and OnKeyboardEventAck() propagates the
  event, and onUnhandledInputEvent() is called.

We still get unpaired onUnhandledInputEvent() callback, only for ACTION_UP.

The first problem here is that return value of handleInputEvent() for
KeyUp event is different from that for RawKeyDown or Char events.

Another problem is that we sometimes create Char event and sometimes we
do not, but Char event cannot deliver (propagate) platform-specific event.

So for a proper fix, we need the following changes:
- When Char event is created, we set os_event such that if Char event is
  not processed, we make sure onUnhandledInputEvent() is called.
- Change EventHandler::keyEvent to return the same value for KeyUp event
  as the value returned for RawKeyDown or Char event.

BUG=538377
==========

to

==========
Unstrangle unhandled key event callback for webview

Currently, when typing on an input form using a BT keyboard or
when InputConnection#sendKeyEvent() is used, we end up getting unhandled
ACTION_UP event in WebViewClient#onUnhandledInputEvent(). Here's what
happens:
1) RawKeyDown event is not processed: we create Char event in
  ime_adapter_android.cc and process it instead.
  So RenderWidgetHostImpl::OnKeyboardEventAck() should normally propagate
  to onUnhandledInputEvent() as the event was not processed. But
  we've set the skip_in_browser flag in ime_adapter_android.cc so
  OnKeyboardEventAck() does not propagate this event.
2) Char event is processed.
3) KeyUp event is not processed, and OnKeyboardEventAck() propagates the
  event, and WebViewClient#onUnhandledInputEvent() is called.

The last one is an incorrect behavior, and we get unpaired
onUnhandledInputEvent() callback only for ACTION_UP.

Also, when ACTION_DOWN / ACTION_UP are not handled (when no input form is
focused), the following happens:
1) RawKeyDown event is not processed. But we set the skip_in_browser flag
  so OnKeyboardEventAck() does not propagate the event.
  (This is an incorrect behavior.)
2) Char event is not created.
3) KeyUp event is not processed, and OnKeyboardEventAck() propagates the
  event, and onUnhandledInputEvent() is called.

We still get unpaired onUnhandledInputEvent() callback, only for ACTION_UP.

The first problem here is that return value of handleInputEvent() for
KeyUp event is different from that for RawKeyDown or Char events.

Another problem is that we sometimes create Char event and sometimes we
do not, but Char event cannot deliver (propagate) platform-specific event.

So for a proper fix, we need the following changes:
- When Char event is created, we set os_event such that if Char event is
  not processed, we make sure onUnhandledInputEvent() is called.
- Change EventHandler::keyEvent to return the same value for KeyUp event
  as the value returned for RawKeyDown or Char event.

BUG=538377
==========

[aelias_OOO_until_Jul13, 2015-12-02 03:41:32]

aelias@chromium.org changed reviewers:
- jdduke@chromium.org

[dtapuska, 2015-12-02 14:25:11]

dtapuska@chromium.org changed reviewers:
+ dtapuska@chromium.org

[dtapuska, 2015-12-02 14:25:12]

https://codereview.chromium.org/1378583003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1378583003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3155: bool
overrideKeyupEvent = false;
Not every keyup corresponds to a keydown.
Ctrl-Tab
Alt-Tab
Drag and Drop when the key modifiers go up.

Switching the application and changing the focus with the mouse.

Are we certain these events are covered and the right result code is returned? I
don't care for tracking state
based on previous events when possible because these valid streams of events are
typically thought to be invalid.

[aelias_OOO_until_Jul13, 2015-12-03 04:04:59]

I agree we shouldn't try to fix this with tracking state.  This bug is not
causing any real problem so I'd rather not land anything unless we have a fix
that simplifies the system rather than makes it more complex.

Dave, I recall you told me you disliked KeyPress events in general, and this
issue is related to that.  I was thinking browser-side code should not generate
any KeyPress events at all and just send the raw key down to the renderer and
change it to do the KeyPress stuff when needed, what do you think?

[dtapuska, 2015-12-03 19:15:22]

On 2015/12/03 04:04:59, aelias wrote:
> I agree we shouldn't try to fix this with tracking state.  This bug is not
> causing any real problem so I'd rather not land anything unless we have a fix
> that simplifies the system rather than makes it more complex.
> 
> Dave, I recall you told me you disliked KeyPress events in general, and this
> issue is related to that.  I was thinking browser-side code should not
generate
> any KeyPress events at all and just send the raw key down to the renderer and
> change it to do the KeyPress stuff when needed, what do you think?

The question is why is skip_in_browser is set. This is only set while we are
processing IME events right. So that those events like backspace don't cause the
page to go back in history when editing inside an IME.

Since this is all done after IME is present I'm not sure why skip_in_browser is
set for android.