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

content/browser/renderer_host/input/web_input_event_builders_android.cc

 // Copyright 2013 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/input/web_input_event_builders_android.h"
 
 #include <android/input.h>
 
 #include "base/logging.h"
 #include "content/browser/renderer_host/input/web_input_event_util.h"
@@ skipping 11 matching lines @@
 using blink::WebMouseEvent;
 using blink::WebMouseWheelEvent;
 using blink::WebPointerProperties;
 using blink::WebTouchEvent;
 using blink::WebTouchPoint;
 
 namespace content {
 
 namespace {
 
+const int kCompositionKeyCode = 229;
+
 int WebInputEventToAndroidModifier(int web_modifier) {
   int android_modifier = 0;
   // Currently only Shift, CapsLock are used, add other modifiers if required.
   if (web_modifier & WebInputEvent::ShiftKey)
     android_modifier |= AMETA_SHIFT_ON;
   if (web_modifier & WebInputEvent::CapsLockOn)
     android_modifier |= AMETA_CAPS_LOCK_ON;
   return android_modifier;
 }
 
 ui::DomKey GetDomKeyFromEvent(
     JNIEnv* env,
     const base::android::JavaRef<jobject>& android_key_event,
     int keycode,
+    ui::DomCode dom_code,
     int modifiers,
     int unicode_character) {
+  // Chrome will send |keycode| COMPOSITION_KEY_CODE 229 for composition events,
+  // but 229 is also mapped to AKEYCODE_LAST_CHANNEL. Return UNIDENTIFIED if the

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

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]

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]

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?
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.

+  // key came from soft keyboard.
+  if (keycode == kCompositionKeyCode && !unicode_character &&
+      dom_code == ui::DomCode::NONE)
+    return ui::DomKey::UNIDENTIFIED;

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

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'.

+
   if (!unicode_character && env) {
     // According to spec |kAllowedModifiers| should be Shift and AltGr, however
     // Android doesn't have AltGr key and ImeAdapter::getModifiers won't pass it
     // either.
     // According to discussion we want to honor CapsLock and possibly NumLock as
     // well. https://github.com/w3c/uievents/issues/70
     const int kAllowedModifiers =
         WebInputEvent::ShiftKey | WebInputEvent::CapsLockOn;
     int fallback_modifiers =
         WebInputEventToAndroidModifier(modifiers & kAllowedModifiers);
@@ skipping 27 matching lines @@
   if (scancode)
     dom_code = ui::KeycodeConverter::NativeKeycodeToDomCode(scancode);
   result.type = type;
   result.modifiers = modifiers;
   result.timeStampSeconds = time_sec;
   result.windowsKeyCode = ui::LocatedToNonLocatedKeyboardCode(
       ui::KeyboardCodeFromAndroidKeyCode(keycode));
   result.modifiers |= DomCodeToWebInputEventModifiers(dom_code);
   result.nativeKeyCode = keycode;
   result.domCode = static_cast<int>(dom_code);
-  result.domKey = GetDomKeyFromEvent(env, android_key_event, keycode, modifiers,
-                                     unicode_character);
+  result.domKey = GetDomKeyFromEvent(env, android_key_event, keycode, dom_code,
+                                     modifiers, unicode_character);
   result.unmodifiedText[0] = unicode_character;
   if (result.windowsKeyCode == ui::VKEY_RETURN) {
     // This is the same behavior as GTK:
     // We need to treat the enter key as a key press of character \r. This
     // is apparently just how webkit handles it and what it expects.
     result.unmodifiedText[0] = '\r';
   }
   result.text[0] = result.unmodifiedText[0];
   result.isSystemKey = is_system_key;
   result.setKeyIdentifierFromWindowsKeyCode();
@@ skipping 66 matching lines @@
   result.type = type;
   result.x = x;
   result.y = y;
   result.timeStampSeconds = time_sec;
   result.sourceDevice = blink::WebGestureDeviceTouchscreen;
 
   return result;
 }
 
 }  // namespace content