[Android] Synthetic composition event should have DomKey 'Unidentified'

[Android] Synthetic composition event should have DomKey 'Unidentified'

Produce DomKey::UNIDENTIFIED if |keycode| is COMPOSITION_KEY_CODE (229)
and have no valid |dom_code| or |unicode_character|. (Came from IME
|setComposingText()|.)

One concern is 229 overlaps with AKEYCODE_LAST_CHANNEL, but this should
be fine because it should have a proper |dom_code| if it comes from
hardware keyboard.

BUG=595329
Committed: https://crrev.com/0e8c9a231127cb3538d6ebd5c4a614e6b3226109
Cr-Commit-Position: refs/heads/master@{#381952}

[chongz, 2016-03-16 15:15:37]

chongz@chromium.org changed reviewers:
+ aelias@chromium.org

[chongz, 2016-03-16 15:15:37]

Hi aelias, can you take a look at this CL please? Thanks!

https://codereview.chromium.org/1811553002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/web_input_event_builders_android.cc
(right):

https://codereview.chromium.org/1811553002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/web_input_event_builders_android.cc:56:
return ui::DomKey::UNIDENTIFIED;
Cannot find a keyboard that produce this key, but 
1. If it comes from a hardware keyboard it should have a proper |dom_code|.
2. Otherwise we already have poor support for soft keyboards, for example
pressing 'a' on the Google US Keyboard will produce |keycode| 229 and DomKey
'Unidentified'.

[aelias_OOO_until_Jul13, 2016-03-16 18:49:11]

Description was changed from

==========
[Android] Synthetic cmoposition event should have DomKey 'Unidentified'

Produce DomKey::UNIDENTIFIED if |keycode| is COMPOSITION_KEY_CODE (229)
and have no valid |dom_code| or |unicode_character|. (Came from IME
|setComposingText()|.)

One concern is 229 overlaps with AKEYCODE_LAST_CHANNEL, but this should
be fine because it should have a proper |dom_code| if it comes from
hardware keyboard.

BUG=595329
==========

to

==========
[Android] Synthetic composition event should have DomKey 'Unidentified'

Produce DomKey::UNIDENTIFIED if |keycode| is COMPOSITION_KEY_CODE (229)
and have no valid |dom_code| or |unicode_character|. (Came from IME
|setComposingText()|.)

One concern is 229 overlaps with AKEYCODE_LAST_CHANNEL, but this should
be fine because it should have a proper |dom_code| if it comes from
hardware keyboard.

BUG=595329
==========

[aelias_OOO_until_Jul13, 2016-03-16 18:51:46]

https://codereview.chromium.org/1811553002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/web_input_event_builders_android.cc
(right):

https://codereview.chromium.org/1811553002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/web_input_event_builders_android.cc:52: //
but 229 is also mapped to AKEYCODE_LAST_CHANNEL. Return UNIDENTIFIED if the
Since 229 can also be a valid keycode, it seems to have no real informational
value, so can we not consider it?  Would the effect be the same if we wrote "if
(!unicode_character && dom_code == ui::DomCode::NONE) return
ui::DomKey::UNIDENTIFIED;"?

[chongz, 2016-03-16 19:50:15]

Thanks for the review! Please see the comments below.

https://codereview.chromium.org/1811553002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/web_input_event_builders_android.cc
(right):

https://codereview.chromium.org/1811553002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/web_input_event_builders_android.cc:52: //
but 229 is also mapped to AKEYCODE_LAST_CHANNEL. Return UNIDENTIFIED if the
On 2016/03/16 18:51:46, aelias wrote:
> Since 229 can also be a valid keycode, it seems to have no real informational
> value, so can we not consider it?  Would the effect be the same if we wrote
"if
> (!unicode_character && dom_code == ui::DomCode::NONE) return
> ui::DomKey::UNIDENTIFIED;"?

I think it would be different. For example Hacker's keyboard on Android has
Ctrl, Shift, Esc, Home, Arrow keys, etc, but the keyboard is only mimicking
|keycode| but not |scancode|, which means |dom_code| will be None. But I guess
it would make sense to produce DomKey even if we don't have DomCode?

Related note in spec:
```
For compatibility with existing content, virtual keyboards, such as software
keyboards on screen-based input devices, are expected to produce the normal
range of keyboard events, even though they do not possess physical keys.
```
https://w3c.github.io/uievents/#interface-keyboardevent

[aelias_OOO_until_Jul13, 2016-03-16 20:36:51]

https://codereview.chromium.org/1811553002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/web_input_event_builders_android.cc
(right):

https://codereview.chromium.org/1811553002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/web_input_event_builders_android.cc:52: //
but 229 is also mapped to AKEYCODE_LAST_CHANNEL. Return UNIDENTIFIED if the
On 2016/03/16 at 19:50:15, chongz wrote:
> On 2016/03/16 18:51:46, aelias wrote:
> > Since 229 can also be a valid keycode, it seems to have no real
informational
> > value, so can we not consider it?  Would the effect be the same if we wrote
"if
> > (!unicode_character && dom_code == ui::DomCode::NONE) return
> > ui::DomKey::UNIDENTIFIED;"?
> 
> I think it would be different. For example Hacker's keyboard on Android has
Ctrl, Shift, Esc, Home, Arrow keys, etc, but the keyboard is only mimicking
|keycode| but not |scancode|, which means |dom_code| will be None. But I guess
it would make sense to produce DomKey even if we don't have DomCode?

I'd just like us to resolve this namespace conflict correctly with the minimum
of logic.  It looks like an actual AKEYCODE_LAST_CHANNEL would also get assigned
a ui::DomCode::NONE and no unicode character, so it seems that in the current
code such a keypress would not work properly.  Could you write something minimal
and correct for both cases and add a unit test checking that a
AKEYCODE_LAST_CHANNEL physical keypress winds up generating the right events?


> 
> Related note in spec:
> ```
> For compatibility with existing content, virtual keyboards, such as software
keyboards on screen-based input devices, are expected to produce the normal
range of keyboard events, even though they do not possess physical keys.
> ```
> https://w3c.github.io/uievents/#interface-keyboardevent

This note doesn't seem strongly normative, there is also a passage at the
beginning of 5.6 saying "Depending on the character generation device, keyboard
events might not be generated."  On Android, that will be all IME events.  The
appropriate behavior for Android is to generate a keyboard event when it comes
from a physical keyboard, but not otherwise.

[chongz, 2016-03-17 15:20:21]

Patchset #2 (id:20001) has been deleted

[chongz, 2016-03-17 15:50:39]

https://codereview.chromium.org/1811553002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/input/web_input_event_builders_android.cc
(right):

https://codereview.chromium.org/1811553002/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/web_input_event_builders_android.cc:50:
return ui::DomKey::UNIDENTIFIED;
Sorry for the confusing logic... Will this look better?
If the event came from |setComposingText()| or it's a synthetic |Char| event
|android_key_event| will be null, otherwise we should have |android_key_event|
if it came from |sendKeyEvent()|.

For 229 if it's AKEYCODE_LAST_CHANNEL it should come from |sendKeyEvent()| and
has |android_key_event|.

However I'm not able to find a hardware with the key (seems to be some kind of
TV remote?) to test, but I've added a pair of unit tests, hope it would be good.

Thanks!

[aelias_OOO_until_Jul13, 2016-03-18 01:07:39]

lgtm, thanks.

[chongz, 2016-03-18 13:37:03]

The CQ bit was checked by chongz@chromium.org

[commit-bot: I haz the power, 2016-03-18 13:37:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1811553002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1811553002/40001

[commit-bot: I haz the power, 2016-03-18 13:42:14]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-03-18 13:43:21]

Description was changed from

==========
[Android] Synthetic composition event should have DomKey 'Unidentified'

Produce DomKey::UNIDENTIFIED if |keycode| is COMPOSITION_KEY_CODE (229)
and have no valid |dom_code| or |unicode_character|. (Came from IME
|setComposingText()|.)

One concern is 229 overlaps with AKEYCODE_LAST_CHANNEL, but this should
be fine because it should have a proper |dom_code| if it comes from
hardware keyboard.

BUG=595329
==========

to

==========
[Android] Synthetic composition event should have DomKey 'Unidentified'

Produce DomKey::UNIDENTIFIED if |keycode| is COMPOSITION_KEY_CODE (229)
and have no valid |dom_code| or |unicode_character|. (Came from IME
|setComposingText()|.)

One concern is 229 overlaps with AKEYCODE_LAST_CHANNEL, but this should
be fine because it should have a proper |dom_code| if it comes from
hardware keyboard.

BUG=595329

Committed: https://crrev.com/0e8c9a231127cb3538d6ebd5c4a614e6b3226109
Cr-Commit-Position: refs/heads/master@{#381952}
==========

[commit-bot: I haz the power, 2016-03-18 13:43:22]

Patchset 2 (id:??) landed as
https://crrev.com/0e8c9a231127cb3538d6ebd5c4a614e6b3226109
Cr-Commit-Position: refs/heads/master@{#381952}