Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2025733002:
[DomKey] Use VK->DomKey mapping for non-printable keys (Closed)
Description[DomKey] Use VK->DomKey mapping for non-printable keys
The original DomCode+flags->DomKey mapping causes issue when the application
generates mismatched keyCode + scanCode, or missing scanCode entirely.
(e.g. keyboard-rewrite softwares that only modifies keyCode)
A better solution is to use VK+flags->DomKey mapping (for both printable
and non-printable keys).
This CL is the first step for the non-printable keys, where a fixed table
for general non-printable keys was introduced. And there will be follow up CLs
to handle other language-specific cases and printable keys.
BUG=612749
Committed: https://crrev.com/fc639eb35da440358def45719ec8573117069e87
Cr-Commit-Position: refs/heads/master@{#397741}
Patch Set 1 : Add VK->DomKey mapping #
Total comments: 19
Patch Set 2 : Wez's review #
Total comments: 1
Patch Set 3 : Wez's review 2 #
Messages
Total messages: 18 (8 generated)
Patchset #1 (id:1) has been deleted
chongz@chromium.org changed reviewers: + dtapuska@chromium.org
dtapuska@ can you take a look at this DomKey CL please? Thanks!
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
On 2016/05/30 21:29:56, chongz wrote: > dtapuska@ can you take a look at this DomKey CL please? Thanks! lgtm
chongz@chromium.org changed reviewers: + wez@chromium.org
wez@ can you take a look at this non-printable DomKey CL please? Thanks!
https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:146: } kNonPrintableKeyMap[] = { nit: Are you sure that none of these values has DomKey meanings dependent on the modifier states? https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:163: // TODO(chongz): Map VKEY_HANJA/VKEY_KANJI based on layout. nit: Would it make sense, here and for the previous comment, to have the comment and no mapping, since the mapping we're introducing here isn't really valid? I think the comments would be clearer making explicit that VKEY_KANA==VKEY_HANGUL, hence the need for special mapping. Incidentally, is there a character output from those keys that would help us do the right mapping without having to hard-wire language/layout specific knowledge? https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:195: // VKEY_NUMPAD0..9 - Handled by |NumPadKeyCodeToDomKey()|. nit: No need for the "Handled by", I don't think; it's implicit that if it's commented then this table doesn't handle it, so something later must. https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:248: // https://crbug.com/612743 Any reason not to make that change directly, here? Or is the problem that no such DomKey exists right now? https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:264: // TODO(chongz): Handle VKEY_NONAME, VKEY_PA1 nit: Is there a bug # for that? https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... File ui/events/keycodes/platform_key_map_win_unittest.cc (right): https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win_unittest.cc:255: // |key_code|. It doesn't seem to be all that useful to test the production version of the table against something that is just a direct copy of that table (and which must be kept up-to-date with it!). I'd suggest instead iterating over the scancodes and calculating (1) the corresponding VKEY under the US layout and (2) the DomKey that it maps to according to DomKeyFromNativeImpl - then you can re-run the DomKeyFromNativeImpl calls but this time just pass in VKEY codes with bogus scancodes, and verify that the function returns the same results. It wouldn't be a strict test that the entire mapping is valid, but it'd be more meaningful, since you'd be verifying the key property that the scancode can be omitted or mismatched, and the VKEY will be used instead. https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win_unittest.cc:381: for (size_t i = 1; i < arraysize(kNonPrintableKeyTestCase); ++i) { You're testing that your test-case is well-ordered, which doesn't seem to actually be a useful property to your test - you really want to test that the original table is well-ordered, surely?
wez@ thanks for the review! PTAL again, thanks! https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:146: } kNonPrintableKeyMap[] = { On 2016/06/01 20:49:04, Wez wrote: > nit: Are you sure that none of these values has DomKey meanings dependent on the > modifier states? I'm not entirely sure, but according to my understanding FF does not have |.key| attribute internally, instead they map virtual key (without modifiers) to DomKey string directly (if not printable)... See http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.c... https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:163: // TODO(chongz): Map VKEY_HANJA/VKEY_KANJI based on layout. On 2016/06/01 20:49:04, Wez wrote: > nit: Would it make sense, here and for the previous comment, to have the comment > and no mapping, since the mapping we're introducing here isn't really valid? > > I think the comments would be clearer making explicit that > VKEY_KANA==VKEY_HANGUL, hence the need for special mapping. Will do. > > Incidentally, is there a character output from those keys that would help us do > the right mapping without having to hard-wire language/layout specific > knowledge? I would assume they have different |DomCode| according to https://w3c.github.io/uievents-code/#keyboard-103 and https://w3c.github.io/uievents-code/#keyboard-106. But then we have to keep |DomCode| around only for this edge case, otherwise (like FF) we could just throw |DomCode| away and only use |VK|: http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.c... https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:195: // VKEY_NUMPAD0..9 - Handled by |NumPadKeyCodeToDomKey()|. On 2016/06/01 20:49:04, Wez wrote: > nit: No need for the "Handled by", I don't think; it's implicit that if it's > commented then this table doesn't handle it, so something later must. Will change it to """ // VKEY_NUMPAD0..9 """ https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:248: // https://crbug.com/612743 On 2016/06/01 20:49:04, Wez wrote: > Any reason not to make that change directly, here? Or is the problem that no > such DomKey exists right now? Because spec says it should be "LaunchMyComputer", See https://w3c.github.io/uievents-key/#key-LaunchMyComputer I've filed spec issue: https://github.com/w3c/uievents-key/issues/27 https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:264: // TODO(chongz): Handle VKEY_NONAME, VKEY_PA1 On 2016/06/01 20:49:04, Wez wrote: > nit: Is there a bug # for that? Will create one. (But actually no idea what these keys are..) https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... File ui/events/keycodes/platform_key_map_win_unittest.cc (right): https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win_unittest.cc:255: // |key_code|. On 2016/06/01 20:49:04, Wez wrote: > It doesn't seem to be all that useful to test the production version of the > table against something that is just a direct copy of that table (and which must > be kept up-to-date with it!). > > I'd suggest instead iterating over the scancodes and calculating (1) the > corresponding VKEY under the US layout and (2) the DomKey that it maps to > according to DomKeyFromNativeImpl - then you can re-run the DomKeyFromNativeImpl > calls but this time just pass in VKEY codes with bogus scancodes, and verify > that the function returns the same results. > > It wouldn't be a strict test that the entire mapping is valid, but it'd be more > meaningful, since you'd be verifying the key property that the scancode can be > omitted or mismatched, and the VKEY will be used instead. If I understand correctly you are suggesting using |DomCodeToUsLayoutDomKey()|(dom_code=>key) to test against |DomKeyFromNativeImpl()|(VK=>key)? Currently we rely on fallback |DomCodeToUsLayoutDomKey()| for non-printable keys, and this CL is trying to add a VK->DomKey map to get rid of the fallback. To make the process more clear can I iterating over the |dom_code| in |dom_us_layout_data.h::kNonPrintableCodeMap[]|, and map the |dom_code| to |scan_code| then US layout |VK|? I think ideally |DomKeyFromNativeImpl()| should only use |VK+flags|, even if |VK| is missing we should map |scan_code| to |VK| and still use the VK->DomKey table. Note: |DomCodeToUsLayoutDomKey()| has issue on US layout, e.g. DomCode::PAUSE+Ctrl should map to DomKey::CANCEL, but actually it does not consider modifiers. But for testing purpose it should be fine since it covers DomCode::ABORT, which will hopefully be mapped to VKEY_CANCEL and DomKey::CANCEL. https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win_unittest.cc:381: for (size_t i = 1; i < arraysize(kNonPrintableKeyTestCase); ++i) { On 2016/06/01 20:49:04, Wez wrote: > You're testing that your test-case is well-ordered, which doesn't seem to > actually be a useful property to your test - you really want to test that the > original table is well-ordered, surely? Yes, because it must be ordered for binary search, and I didn't find a way to do it using |static_assert<>|...
wez@ I've updated CL as per your comments, PTAL, thanks! (comments and replies in last patch)
lgtm https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:146: } kNonPrintableKeyMap[] = { On 2016/06/01 23:09:42, chongz wrote: > On 2016/06/01 20:49:04, Wez wrote: > > nit: Are you sure that none of these values has DomKey meanings dependent on > the > > modifier states? > > I'm not entirely sure, but according to my understanding FF does not have |.key| > attribute internally, instead they map virtual key (without modifiers) to DomKey > string directly (if not printable)... > > See > http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.c... Acknowledged. https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:248: // https://crbug.com/612743 On 2016/06/01 23:09:42, chongz wrote: > On 2016/06/01 20:49:04, Wez wrote: > > Any reason not to make that change directly, here? Or is the problem that no > > such DomKey exists right now? > > Because spec says it should be "LaunchMyComputer", > See https://w3c.github.io/uievents-key/#key-LaunchMyComputer > > I've filed spec issue: > https://github.com/w3c/uievents-key/issues/27 nit: Seems that it would be best to avoid leaving a comment here (since what is implemented matches the spec) and instead just track the spec issue in your crbug and update here if necessary? https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:264: // TODO(chongz): Handle VKEY_NONAME, VKEY_PA1 On 2016/06/01 23:09:42, chongz wrote: > On 2016/06/01 20:49:04, Wez wrote: > > nit: Is there a bug # for that? > > Will create one. (But actually no idea what these keys are..) Hopefully they can be skipped! https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... File ui/events/keycodes/platform_key_map_win_unittest.cc (right): https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win_unittest.cc:255: // |key_code|. On 2016/06/01 23:09:42, chongz wrote: > On 2016/06/01 20:49:04, Wez wrote: > > It doesn't seem to be all that useful to test the production version of the > > table against something that is just a direct copy of that table (and which > must > > be kept up-to-date with it!). > > > > I'd suggest instead iterating over the scancodes and calculating (1) the > > corresponding VKEY under the US layout and (2) the DomKey that it maps to > > according to DomKeyFromNativeImpl - then you can re-run the > DomKeyFromNativeImpl > > calls but this time just pass in VKEY codes with bogus scancodes, and verify > > that the function returns the same results. > > > > It wouldn't be a strict test that the entire mapping is valid, but it'd be > more > > meaningful, since you'd be verifying the key property that the scancode can be > > omitted or mismatched, and the VKEY will be used instead. > > If I understand correctly you are suggesting using > |DomCodeToUsLayoutDomKey()|(dom_code=>key) to test against > |DomKeyFromNativeImpl()|(VK=>key)? Something along those lines, yes. > Currently we rely on fallback |DomCodeToUsLayoutDomKey()| for non-printable > keys, and this CL is trying to add a VK->DomKey map to get rid of the fallback. Right and we expect DomCodeToUsLayoutDomKey() to match what we would expect the platform to do, so it seems reasonable to use it to do limited (just to US English layouts) tests to verify our platform-specific handling, given that we have scancode<->DomCode and can use MapVirtualKey for scancode<->VKEY under the US layout. > To make the process more clear can I iterating over the |dom_code| in > |dom_us_layout_data.h::kNonPrintableCodeMap[]|, and map the |dom_code| to > |scan_code| then US layout |VK|? Yes, I think that makes sense. > I think ideally |DomKeyFromNativeImpl()| should only use |VK+flags|, even if > |VK| is missing we should map |scan_code| to |VK| and still use the VK->DomKey > table. Agreed - we should be able to do that as soon as the printable-character map is switched over to be keyed off of VK+flags :) > Note: |DomCodeToUsLayoutDomKey()| has issue on US layout, > e.g. DomCode::PAUSE+Ctrl should map to DomKey::CANCEL, but actually it does not > consider modifiers. > But for testing purpose it should be fine since it covers DomCode::ABORT, which > will hopefully be mapped to VKEY_CANCEL and DomKey::CANCEL. Ah yes - perhaps we can patch that up in a follow-up CL, or at least add a note that it is broken? https://codereview.chromium.org/2025733002/diff/60001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win_unittest.cc:381: for (size_t i = 1; i < arraysize(kNonPrintableKeyTestCase); ++i) { On 2016/06/01 23:09:42, chongz wrote: > On 2016/06/01 20:49:04, Wez wrote: > > You're testing that your test-case is well-ordered, which doesn't seem to > > actually be a useful property to your test - you really want to test that the > > original table is well-ordered, surely? > > Yes, because it must be ordered for binary search, and I didn't find a way to do > it using |static_assert<>|... Right; my point was just that testing that a copy of the table is well-ordered, rather than the table itself, may not be very helpful! https://codereview.chromium.org/2025733002/diff/80001/ui/events/keycodes/plat... File ui/events/keycodes/platform_key_map_win_unittest.cc (right): https://codereview.chromium.org/2025733002/diff/80001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win_unittest.cc:268: if (!key_code) nit: I'd explicitly test against VKEY_UNKNOWN here, since |key_code| is an enum.
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2025733002/#ps100001 (title: "Wez's review 2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2025733002/100001
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [DomKey] Use VK->DomKey mapping for non-printable keys The original DomCode+flags->DomKey mapping causes issue when the application generates mismatched keyCode + scanCode, or missing scanCode entirely. (e.g. keyboard-rewrite softwares that only modifies keyCode) A better solution is to use VK+flags->DomKey mapping (for both printable and non-printable keys). This CL is the first step for the non-printable keys, where a fixed table for general non-printable keys was introduced. And there will be follow up CLs to handle other language-specific cases and printable keys. BUG=612749 ========== to ========== [DomKey] Use VK->DomKey mapping for non-printable keys The original DomCode+flags->DomKey mapping causes issue when the application generates mismatched keyCode + scanCode, or missing scanCode entirely. (e.g. keyboard-rewrite softwares that only modifies keyCode) A better solution is to use VK+flags->DomKey mapping (for both printable and non-printable keys). This CL is the first step for the non-printable keys, where a fixed table for general non-printable keys was introduced. And there will be follow up CLs to handle other language-specific cases and printable keys. BUG=612749 Committed: https://crrev.com/fc639eb35da440358def45719ec8573117069e87 Cr-Commit-Position: refs/heads/master@{#397741} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fc639eb35da440358def45719ec8573117069e87 Cr-Commit-Position: refs/heads/master@{#397741} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
