Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1310513010:
Support DomCode on Android devices. (Closed)
|
Created:
5 years, 3 months ago by dtapuska Modified:
5 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jdduke+watch_chromium.org, Sergey Ulanov, tdresser+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master_evdev Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport DomCode on Android devices.
Pass the evdev scancode down from java so that it can be properly
presented to blink. The evdev code is necessary because the
android keycode already has the keyboard mapping applied. If the scancode
is 0 we fall back to mapping the DomCode based on the android keycode.
BUG=227231
Committed: https://crrev.com/ef120ea40d983e21a1d14cc56324d6ca73be8d0a
Cr-Commit-Position: refs/heads/master@{#348721}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Remove Android keycode->domcode conversion path #
Total comments: 2
Messages
Total messages: 23 (6 generated)
dtapuska@chromium.org changed reviewers: + aelias@chromium.org, garykac@chromium.org, kpschoedel@chromium.org, wez@chromium.org
lgtm
lgtm 2.0
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/web_input_event_builders_android.cc (right): https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/web_input_event_builders_android.cc:40: dom_code = ui::DomCodeFromAndroidKeyCode(keycode); I don't think this is the right approach. DomCodeFromAndroidKeyCode() assumes QWERTY layout, so the translation is not correct for any other layout. From https://source.android.com/devices/input/keyboard-devices.html On a US English (QWERTY) keyboard, the top-left alphabetic key is labeled Q. On a French (AZERTY) keyboard, the key in the same position is labeled A. Despite the label, on both keyboards the top-left alphabetic key is referred to using the HID usage 0x07 0x0014 which is mapped to the Linux key code KEY_Q. domCode must not depend on the keyboard layout, i.e. top left key must have the same domCode in both English and French layouts. So unless we can verify that QWERTY layout is selected we cannot be sure that the domCode we get here is correct.
https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/web_input_event_builders_android.cc (right): https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/web_input_event_builders_android.cc:40: dom_code = ui::DomCodeFromAndroidKeyCode(keycode); I don't think this is the right approach. DomCodeFromAndroidKeyCode() assumes QWERTY layout, so the translation is not correct for any other layout. From https://source.android.com/devices/input/keyboard-devices.html On a US English (QWERTY) keyboard, the top-left alphabetic key is labeled Q. On a French (AZERTY) keyboard, the key in the same position is labeled A. Despite the label, on both keyboards the top-left alphabetic key is referred to using the HID usage 0x07 0x0014 which is mapped to the Linux key code KEY_Q. domCode must not depend on the keyboard layout, i.e. top left key must have the same domCode in both English and French layouts. So unless we can verify that QWERTY layout is selected we cannot be sure that the domCode we get here is correct.
https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/web_input_event_builders_android.cc (right): https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/web_input_event_builders_android.cc:40: dom_code = ui::DomCodeFromAndroidKeyCode(keycode); On 2015/09/09 19:55:44, Sergey Ulanov wrote: > I don't think this is the right approach. DomCodeFromAndroidKeyCode() assumes > QWERTY layout, so the translation is not correct for any other layout. > > From https://source.android.com/devices/input/keyboard-devices.html > > On a US English (QWERTY) keyboard, the top-left alphabetic key is > labeled Q. On a French (AZERTY) keyboard, the key in the same > position is labeled A. Despite the label, on both keyboards the > top-left alphabetic key is referred to using the HID usage 0x07 0x0014 > which is mapped to the Linux key code KEY_Q. > > domCode must not depend on the keyboard layout, i.e. top left key must have the > same domCode in both English and French layouts. So unless we can verify that > QWERTY layout is selected we cannot be sure that the domCode we get here is > correct. So what is your preferred fallback? Not to set the domcode at all? The OSK generates keycodes for backspace, tab, space and enter.. but doesn't fill in the scancode.
On 2015/09/09 20:00:13, dtapuska wrote: > https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/input/web_input_event_builders_android.cc > (right): > > https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/input/web_input_event_builders_android.cc:40: > dom_code = ui::DomCodeFromAndroidKeyCode(keycode); > On 2015/09/09 19:55:44, Sergey Ulanov wrote: > > I don't think this is the right approach. DomCodeFromAndroidKeyCode() assumes > > QWERTY layout, so the translation is not correct for any other layout. > > > > From https://source.android.com/devices/input/keyboard-devices.html > > > > On a US English (QWERTY) keyboard, the top-left alphabetic key is > > labeled Q. On a French (AZERTY) keyboard, the key in the same > > position is labeled A. Despite the label, on both keyboards the > > top-left alphabetic key is referred to using the HID usage 0x07 0x0014 > > which is mapped to the Linux key code KEY_Q. > > > > domCode must not depend on the keyboard layout, i.e. top left key must have > the > > same domCode in both English and French layouts. So unless we can verify that > > QWERTY layout is selected we cannot be sure that the domCode we get here is > > correct. > > So what is your preferred fallback? Not to set the domcode at all? > > The OSK generates keycodes for backspace, tab, space and enter.. but doesn't > fill in the scancode. See: http://www.w3.org/TR/DOM-Level-3-Events/#code-virtual-keyboards 6.2.4 code and Virtual Keyboards The usefulness of the code attribute is less obvious for virtual keyboards (and also for remote controls and chording keyboards). In general, if a virtual (or remote control) keyboard is mimicking the layout and functionality of a standard keyboard, then it must also set the code attribute as appropriate. For keyboards which are not mimicking the layout of a standard keyboard, then the code attribute may be set to the closest match on a standard keyboard or it may be left undefined. For virtual keyboards with keys that produce different values based on some modifier state, the code value should be the key value generated when the button is pressed while the device is in its factory-reset state.
https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/web_input_event_builders_android.cc (right): https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/web_input_event_builders_android.cc:40: dom_code = ui::DomCodeFromAndroidKeyCode(keycode); On 2015/09/09 20:00:13, dtapuska wrote: > So what is your preferred fallback? Not to set the domcode at all? > > The OSK generates keycodes for backspace, tab, space and enter.. but doesn't > fill in the scancode. I'm not sure, but I think not setting it at all would be better than setting it incorrectly. Gary would be the best person to answer this question. The spec says the following (see http://www.w3.org/TR/DOM-Level-3-Events-code/#keyboard-chording-virtual ): Virtual keyboards are software-based sets of keys, in a variety of different arrangements, commonly found on touch-screen devices. They are often modal, with the ability to switch between different dynamic sets of keys, such as alphabetic, numeric, or symbolic keys. Because of the lack of physical constraints, these keyboards may present the widest range of characters, including emoticons and other symbols, and may have keys not represented by Unicode [Unicode] or by the key values [DOM-Level-3-Events-key]. Wherever possible, however, virtual keyboards should produce the normal range of keyboard events and values, for ease of authoring and compatibility with existing content. Perhaps we can still map keyCode to domCode, but only for keys that don't change position in different layouts (i.e. backspace, tab, space, enter)
On 2015/09/09 20:20:30, Sergey Ulanov wrote: > https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/input/web_input_event_builders_android.cc > (right): > > https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/input/web_input_event_builders_android.cc:40: > dom_code = ui::DomCodeFromAndroidKeyCode(keycode); > On 2015/09/09 20:00:13, dtapuska wrote: > > So what is your preferred fallback? Not to set the domcode at all? > > > > The OSK generates keycodes for backspace, tab, space and enter.. but doesn't > > fill in the scancode. > > I'm not sure, but I think not setting it at all would be better than setting it > incorrectly. Gary would be the best person to answer this question. > > The spec says the following (see > http://www.w3.org/TR/DOM-Level-3-Events-code/#keyboard-chording-virtual ): > Virtual keyboards are software-based sets of keys, in a variety of different > arrangements, commonly found on touch-screen devices. They are often modal, > with > the ability to switch between different dynamic sets of keys, such as > alphabetic, > numeric, or symbolic keys. Because of the lack of physical constraints, these > keyboards may present the widest range of characters, including emoticons and > other symbols, and may have keys not represented by Unicode [Unicode] or by > the > key values [DOM-Level-3-Events-key]. Wherever possible, however, virtual > keyboards > should produce the normal range of keyboard events and values, for ease of > authoring and compatibility with existing content. > > Perhaps we can still map keyCode to domCode, but only for keys that don't change > position in different layouts (i.e. backspace, tab, space, enter) If you use the stock android keyboard with the ImeAdapter; it only generates 229 for keys; but it does generate the keycodes for enter, space, tab. If you use the swiftkey keyboard (and I presume its the same for samsung keyboard) then it does send keycodes that match the inputted character. And yes a dvorak keyboard will generate a KEY_O when they 'o' is pressed and not a KEY_S as you would expect with a physical keyboard. So the question is whether we want it correct for a qwerty OSK at all.
On 2015/09/10 13:21:33, dtapuska wrote: > On 2015/09/09 20:20:30, Sergey Ulanov wrote: > > > https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... > > File content/browser/renderer_host/input/web_input_event_builders_android.cc > > (right): > > > > > https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... > > content/browser/renderer_host/input/web_input_event_builders_android.cc:40: > > dom_code = ui::DomCodeFromAndroidKeyCode(keycode); > > On 2015/09/09 20:00:13, dtapuska wrote: > > > So what is your preferred fallback? Not to set the domcode at all? > > > > > > The OSK generates keycodes for backspace, tab, space and enter.. but doesn't > > > fill in the scancode. > > > > I'm not sure, but I think not setting it at all would be better than setting > it > > incorrectly. Gary would be the best person to answer this question. > > > > The spec says the following (see > > http://www.w3.org/TR/DOM-Level-3-Events-code/#keyboard-chording-virtual ): > > Virtual keyboards are software-based sets of keys, in a variety of different > > > arrangements, commonly found on touch-screen devices. They are often modal, > > with > > the ability to switch between different dynamic sets of keys, such as > > alphabetic, > > numeric, or symbolic keys. Because of the lack of physical constraints, > these > > keyboards may present the widest range of characters, including emoticons > and > > other symbols, and may have keys not represented by Unicode [Unicode] or by > > the > > key values [DOM-Level-3-Events-key]. Wherever possible, however, virtual > > keyboards > > should produce the normal range of keyboard events and values, for ease of > > authoring and compatibility with existing content. > > > > Perhaps we can still map keyCode to domCode, but only for keys that don't > change > > position in different layouts (i.e. backspace, tab, space, enter) > > If you use the stock android keyboard with the ImeAdapter; it only generates 229 > for keys; but it does generate the keycodes for enter, space, tab. > > If you use the swiftkey keyboard (and I presume its the same for samsung > keyboard) > then it does send keycodes that match the inputted character. And yes a dvorak > keyboard > will generate a KEY_O when they 'o' is pressed and not a KEY_S as you would > expect > with a physical keyboard. > > So the question is whether we want it correct for a qwerty OSK at all. I've removed the android keycode-> domcode conversion; this makes it consistent with FireFox on the Android platform.
dtapuska@chromium.org changed reviewers: + piman@chromium.org
On 2015/09/11 16:04:08, dtapuska wrote: > On 2015/09/10 13:21:33, dtapuska wrote: > > On 2015/09/09 20:20:30, Sergey Ulanov wrote: > > > > > > https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... > > > File content/browser/renderer_host/input/web_input_event_builders_android.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/1310513010/diff/1/content/browser/renderer_ho... > > > content/browser/renderer_host/input/web_input_event_builders_android.cc:40: > > > dom_code = ui::DomCodeFromAndroidKeyCode(keycode); > > > On 2015/09/09 20:00:13, dtapuska wrote: > > > > So what is your preferred fallback? Not to set the domcode at all? > > > > > > > > The OSK generates keycodes for backspace, tab, space and enter.. but > doesn't > > > > fill in the scancode. > > > > > > I'm not sure, but I think not setting it at all would be better than setting > > it > > > incorrectly. Gary would be the best person to answer this question. > > > > > > The spec says the following (see > > > http://www.w3.org/TR/DOM-Level-3-Events-code/#keyboard-chording-virtual ): > > > Virtual keyboards are software-based sets of keys, in a variety of > different > > > > > arrangements, commonly found on touch-screen devices. They are often > modal, > > > with > > > the ability to switch between different dynamic sets of keys, such as > > > alphabetic, > > > numeric, or symbolic keys. Because of the lack of physical constraints, > > these > > > keyboards may present the widest range of characters, including emoticons > > and > > > other symbols, and may have keys not represented by Unicode [Unicode] or > by > > > the > > > key values [DOM-Level-3-Events-key]. Wherever possible, however, virtual > > > keyboards > > > should produce the normal range of keyboard events and values, for ease of > > > > authoring and compatibility with existing content. > > > > > > Perhaps we can still map keyCode to domCode, but only for keys that don't > > change > > > position in different layouts (i.e. backspace, tab, space, enter) > > > > If you use the stock android keyboard with the ImeAdapter; it only generates > 229 > > for keys; but it does generate the keycodes for enter, space, tab. > > > > If you use the swiftkey keyboard (and I presume its the same for samsung > > keyboard) > > then it does send keycodes that match the inputted character. And yes a dvorak > > keyboard > > will generate a KEY_O when they 'o' is pressed and not a KEY_S as you would > > expect > > with a physical keyboard. > > > > So the question is whether we want it correct for a qwerty OSK at all. > > I've removed the android keycode-> domcode conversion; this makes it consistent > with FireFox on the Android platform. +piman to review: content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java & content/public/browser/native_web_keyboard_event.h
lgtm
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, kpschoedel@chromium.org Link to the patchset: https://codereview.chromium.org/1310513010/#ps20001 (title: "Remove Android keycode->domcode conversion path")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310513010/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310513010/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ef120ea40d983e21a1d14cc56324d6ca73be8d0a Cr-Commit-Position: refs/heads/master@{#348721}
Message was sent while issue was closed.
Belated drive-by; LGTM w/ nits. https://codereview.chromium.org/1310513010/diff/20001/content/browser/rendere... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/1310513010/diff/20001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:162: is_system_key); nit: unnecessary line-wrap at the end here? https://codereview.chromium.org/1310513010/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/web_input_event_builders_android.cc (right): https://codereview.chromium.org/1310513010/diff/20001/content/browser/rendere... content/browser/renderer_host/input/web_input_event_builders_android.cc:37: if (scancode != 0) nit: if (scancode)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ef120ea40d983e21a1d14cc56324d6ca73be8d0a Cr-Commit-Position: refs/heads/master@{#348721} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
