Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2128573002:
[DomKey] Support Japanese (JIS) layout special keys (Closed)
Description[DomKey] Support Japanese (JIS) layout special keys
This CL adds mappings for for keys specific to Japanese (JIS) layouts:
1. "半角/全角\n漢字"
(DomKey::HANKAKU/ZENKAKU/KANJI_MODE, DomCode::BACKQUOTE, to the left of Digit1)
Used to switch between Hankaku, Zenkaku and KanjiMode, may use with Alt.
2. "Caps Lock\n英数"
(DomKey::ALPHANUMERIC/HIRAGANA, DomCode::CAPS_LOCK, same location as CapsLock)
Used to switch between Alphanumeric and Hiragana.
3. "カタカナ\nひらがな\nローマ字"
(DomKey::KATAKANA/HIRAGANA/ROMAJI, DomCode::KANA_MODE, to the left of AltRight)
Used to switch between Katakana, Hiragana and Romaji, may use with Alt or Shift
(We don't need to handle Convert/NonConvert specially but could simply put them into the generic mapping table. The reason is their VKEYs are unique and clear - VK_CONVERT/VK_NONCONVERT, and the OS won't produce those VKEYs if they are not supported by current layout. e.g. Will produce 0xff on US layout)
Since the VKEYs for these keys express their correct meanings, taking into account modifiers and lock states, we can use a static table to map them to DomKey values.
See the BUG for how JIS keyboard works and how to produce these values...
Note:
Sometimes pressing a key will cause 'keyup' events that does not match this key, this is known and actually means we are leaving the previous state.
e.g. Press Alphanumeric key first, and then if you hit Hiragana key you will actually get a keyup for Alphanumeric key, which means you are leaving Alphanumeric state.
BUG=612694
Committed: https://crrev.com/9d990a45f12393c2e557d4b33116b5ce1022b403
Cr-Commit-Position: refs/heads/master@{#404715}
Patch Set 1 : Add DomKey map for Japanese special keys #
Total comments: 23
Patch Set 2 : wez's review #Patch Set 3 : Wez's review 2 #
Messages
Total messages: 22 (14 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add DomKey map for Japanese special keys BUG=612694 ========== to ========== [DomKey] Support JIS layout special keys This CL added mapping for JIS keyboard special keys: 1. "半角/全角\n漢字" (to the left of Digit1) 2. "Caps Lock\n英数" (same location as "Caps Lock" key) 3. "カタカナ\nひらがな\nローマ字" (to the left of right Alt) We can do the static mapping from |VKEY| to |DomKey|, and OS will handle the modifiers & state to provide correct |VKEY|. After CL we will match Firefox. See https://crbug.com/612694 for how JIS keyboard works and how to produce these values... Note: Sometimes it will generate a 'keyup' for another key, this means we are leaving the that state. BUG=612694 ==========
Description was changed from ========== [DomKey] Support JIS layout special keys This CL added mapping for JIS keyboard special keys: 1. "半角/全角\n漢字" (to the left of Digit1) 2. "Caps Lock\n英数" (same location as "Caps Lock" key) 3. "カタカナ\nひらがな\nローマ字" (to the left of right Alt) We can do the static mapping from |VKEY| to |DomKey|, and OS will handle the modifiers & state to provide correct |VKEY|. After CL we will match Firefox. See https://crbug.com/612694 for how JIS keyboard works and how to produce these values... Note: Sometimes it will generate a 'keyup' for another key, this means we are leaving the that state. BUG=612694 ========== to ========== [DomKey] Support JIS layout special keys This CL added mapping for JIS keyboard special keys: 1. "半角/全角\n漢字" (to the left of Digit1) 2. "Caps Lock\n英数" (same location as "Caps Lock" key) 3. "カタカナ\nひらがな\nローマ字" (to the left of right Alt) ("Convert"/"NonConvert" has their own VKEYs so could be handled as general keys) We can do the static mapping from |VKEY| to |DomKey|, and OS will handle the modifiers & state to provide correct |VKEY|. After CL we will match Firefox. See https://crbug.com/612694 for how JIS keyboard works and how to produce these values... Note: Sometimes it will generate a 'keyup' for another key, this means we are leaving the that state. BUG=612694 ==========
chongz@chromium.org changed reviewers: + wez@chromium.org
wez@ this CL adds DomKey support for JIS special keys, PTAL, thanks! https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat... File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:215: return DomKey::KANA_MODE; |VKEY_KANA| is not used with modern Japanese keyboard (they use |VKEY_ATTN| instead), but we want to match FF and IE to support it. https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fddca814e0#l1.146 https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:231: return DomKey::KANA_MODE; On other layouts |VKEY_ATTN| will be mapped to |DomKey::ATTN|. https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:243: return key; Do language mapping first so JIS could map |VKEY_ATTN| to |DomKey::KANA_MODE|. https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:313: return DomKey::UNIDENTIFIED; |KeyEvent::ApplyLayout()| will do the fall-back mapping |DomCode->DomKey| if the key is |DomKey::NONE|. We want to avoid that so |KanaMode| key could produce "Unidentified" on non-Japanese layout. (DomCode is still "KanaMode")
This mostly looks great - just some minor nits and mainly comments on the CL
description :D
// This CL added mapping for JIS keyboard special keys:
nit: added->adds, mapping->mappings
I'd also suggest saying "for keys specific to Japanese (JIS) layouts." since not
everyone is familiar with the "JIS" term.
// 1. "半角/全角\n漢字" (to the left of Digit1)
// 2. "Caps Lock\n英数" (same location as "Caps Lock" key)
// 3. "カタカナ\nひらがな\nローマ字" (to the left of right Alt)
nit: It's not clear as a non-Japanese-reader which each of these cases
correspond to in the code, and the location-based descriptions aren't all that
helpful out of context! - I'd suggest providing the key-cap names as you have
done here, then providing the DOM |key| name in brackets, the location
information that you've provided, and then a second line e.g. "Used to switch
between Romaji/Hirigana/Katakana" etc, as appropriate.
nit: In your location descriptions, use the DOM key or code names for the keys
consistently, e.g. Digit1, CapsLock and AltRight in the above.
// ("Convert"/"NonConvert" has their own VKEYs so could be handled as general
keys)
nit: Clarify why we still prefer to handle them specially - presumably so that
English US generates UNIDENTIFIED for them?
// We can do the static mapping from |VKEY| to |DomKey|, and OS will handle the
// modifiers & state to provide correct |VKEY|.
nit: Suggest "Since the VKEYs for these keys express their correct meanings,
taking into account modifiers and lock states, we can use a static table to map
them to DomKey values."
// After CL we will match Firefox.
nit: This seems redundant (and technically a non-goal - the aim for all the
browsers should be to match the spec :P).
// See https://crbug.com/612694 for how JIS keyboard works and how to produce
these values...
nit: You can just say "see the BUG" since it's in the BUG= line, rather than
duplicating the number here. :)
// Note:
// Sometimes it will generate a 'keyup' for another key, this means we are
leaving the // that state.
IIUC you're just saying that the DomKey value may change if the key is down at
the moment that the layout changes, e.g. en_US->jp_JP, and that this is
expected. Is that consistent with the DOM spec?
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_codes_posix.h (right):
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_codes_posix.h:201: VKEY_OEM_COPY = 0xF2,
Since these have not-very-helpful names, can we add comments to explain their
actual use, as we have done above for the GTV OEM codes?
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
File ui/events/keycodes/platform_key_map_win.cc (right):
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win.cc:215: return DomKey::KANA_MODE;
On 2016/07/06 22:50:34, chongz wrote:
> |VKEY_KANA| is not used with modern Japanese keyboard (they use |VKEY_ATTN|
> instead), but we want to match FF and IE to support it.
>
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fddca814e0#l1.146
When you say "modern keyboard" do you mean modern Japanese layouts have switched
to VKEY_ATTN?
Suggest merging this with the VKEY_ATTN switch case, so it's more obvious that
the two values generate the same output.
You might add a comment saying that some Japanese layouts used VKEY_KANA for
KANA_MODE, while others use VKEY_ATTN, so we map them both for compatibility.
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win.cc:243: return key;
On 2016/07/06 22:50:34, chongz wrote:
> Do language mapping first so JIS could map |VKEY_ATTN| to |DomKey::KANA_MODE|.
Acknowledged.
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win.cc:313: return DomKey::UNIDENTIFIED;
On 2016/07/06 22:50:34, chongz wrote:
> |KeyEvent::ApplyLayout()| will do the fall-back mapping |DomCode->DomKey| if
the
> key is |DomKey::NONE|.
>
> We want to avoid that so |KanaMode| key could produce "Unidentified" on
> non-Japanese layout. (DomCode is still "KanaMode")
Acknowledged.
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
File ui/events/keycodes/platform_key_map_win_unittest.cc (right):
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:49: return
reinterpret_cast<HKL>(0x04110411);
LoadKeyboardLayout doesn't work here?
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:120: int flags) {
What's the point of this "helper"? It just seems to convert between
function-call and method-call syntax..?
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:359: PlatformKeyMap
us_keymap(us_layout);
nit: Here and elsewhere, I think it's OK to pass the result of GetInputLocale()
directly to the PlatformKeyMap constructor.
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:360: for (const auto&
test_case : kJapaneseTestCases) {
Suggest folding these two loops into one, for clarity - create the two layouts
and then loop over the cases, doing two EXPECT_EQ()s in each iteration, one for
us, one for Japanese.
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:368: PlatformKeyMap
ko_keymap(jp_layout);
jp_keymap?
Description was changed from
==========
[DomKey] Support JIS layout special keys
This CL added mapping for JIS keyboard special keys:
1. "半角/全角\n漢字" (to the left of Digit1)
2. "Caps Lock\n英数" (same location as "Caps Lock" key)
3. "カタカナ\nひらがな\nローマ字" (to the left of right Alt)
("Convert"/"NonConvert" has their own VKEYs so could be handled as general keys)
We can do the static mapping from |VKEY| to |DomKey|, and OS will handle the
modifiers & state to provide correct |VKEY|.
After CL we will match Firefox.
See https://crbug.com/612694 for how JIS keyboard works and how to produce these
values...
Note:
Sometimes it will generate a 'keyup' for another key, this means we are leaving
the that state.
BUG=612694
==========
to
==========
[DomKey] Support Japanese (JIS) layout special keys
This CL adds mappings for for keys specific to Japanese (JIS) layouts.
1. "半角/全角\n漢字" (Hankaku/Zenkaku/KanjiMode, to the left of Digit1)
2. "Caps Lock\n英数" (Alphanumeric/Hiragana, same location as CapsLock)
3. "カタカナ\nひらがな\nローマ字" (Katakana/Hiragana/Romaji, to the left of AltRight)
(We don't need to handle Convert/NonConvert here because their VKEYs expressed
their correct meanings, and the OS won't generate those VKEYs if they are not
supported in current layout.)
We can do the static mapping from |VKEY| to |DomKey|, and OS will handle the
modifiers & state to provide correct |VKEY|.
After CL we will match Firefox.
See https://crbug.com/612694 for how JIS keyboard works and how to produce these
values...
Note:
Sometimes it will generate a 'keyup' for another key, this means we are leaving
the that state.
BUG=612694
==========
Description was changed from ========== [DomKey] Support Japanese (JIS) layout special keys This CL adds mappings for for keys specific to Japanese (JIS) layouts. 1. "半角/全角\n漢字" (Hankaku/Zenkaku/KanjiMode, to the left of Digit1) 2. "Caps Lock\n英数" (Alphanumeric/Hiragana, same location as CapsLock) 3. "カタカナ\nひらがな\nローマ字" (Katakana/Hiragana/Romaji, to the left of AltRight) (We don't need to handle Convert/NonConvert here because their VKEYs expressed their correct meanings, and the OS won't generate those VKEYs if they are not supported in current layout.) We can do the static mapping from |VKEY| to |DomKey|, and OS will handle the modifiers & state to provide correct |VKEY|. After CL we will match Firefox. See https://crbug.com/612694 for how JIS keyboard works and how to produce these values... Note: Sometimes it will generate a 'keyup' for another key, this means we are leaving the that state. BUG=612694 ========== to ========== [DomKey] Support Japanese (JIS) layout special keys This CL adds mappings for for keys specific to Japanese (JIS) layouts. 1. "半角/全角\n漢字" (Hankaku/Zenkaku/KanjiMode, to the left of Digit1) 2. "Caps Lock\n英数" (Alphanumeric/Hiragana, same location as CapsLock) 3. "カタカナ\nひらがな\nローマ字" (Katakana/Hiragana/Romaji, to the left of AltRight) (We don't need to handle Convert/NonConvert here because they have unique VKEYs, and the OS won't generate those VKEYs if they are not supported in current layout.) Since the VKEYs for these keys express their correct meanings, taking into account modifiers and lock states, we can use a static table to map them to DomKey values. See the BUG for how JIS keyboard works and how to produce these values... Note: Sometimes pressing a key will cause 'keyup' events that does not match this key, this is known and actually means we are leaving the previous state. e.g. Press Alphanumeric key first, and then if you hit Hiragana key you will actually get a keyup for Alphanumeric key, which means you are leaving Alphanumeric state. BUG=612694 ==========
Description was changed from ========== [DomKey] Support Japanese (JIS) layout special keys This CL adds mappings for for keys specific to Japanese (JIS) layouts. 1. "半角/全角\n漢字" (Hankaku/Zenkaku/KanjiMode, to the left of Digit1) 2. "Caps Lock\n英数" (Alphanumeric/Hiragana, same location as CapsLock) 3. "カタカナ\nひらがな\nローマ字" (Katakana/Hiragana/Romaji, to the left of AltRight) (We don't need to handle Convert/NonConvert here because they have unique VKEYs, and the OS won't generate those VKEYs if they are not supported in current layout.) Since the VKEYs for these keys express their correct meanings, taking into account modifiers and lock states, we can use a static table to map them to DomKey values. See the BUG for how JIS keyboard works and how to produce these values... Note: Sometimes pressing a key will cause 'keyup' events that does not match this key, this is known and actually means we are leaving the previous state. e.g. Press Alphanumeric key first, and then if you hit Hiragana key you will actually get a keyup for Alphanumeric key, which means you are leaving Alphanumeric state. BUG=612694 ========== to ========== [DomKey] Support Japanese (JIS) layout special keys This CL adds mappings for for keys specific to Japanese (JIS) layouts. 1. "半角/全角\n漢字" (Hankaku/Zenkaku/KanjiMode, to the left of Digit1) 2. "Caps Lock\n英数" (Alphanumeric/Hiragana, same location as CapsLock) 3. "カタカナ\nひらがな\nローマ字" (Katakana/Hiragana/Romaji, to the left of AltRight) (We don't need to handle Convert/NonConvert here but could simply put them into the generic mapping table. The reason is their VKEYs are unique, and the OS won't generate those VKEYs if they are not supported in current layout.) Since the VKEYs for these keys express their correct meanings, taking into account modifiers and lock states, we can use a static table to map them to DomKey values. See the BUG for how JIS keyboard works and how to produce these values... Note: Sometimes pressing a key will cause 'keyup' events that does not match this key, this is known and actually means we are leaving the previous state. e.g. Press Alphanumeric key first, and then if you hit Hiragana key you will actually get a keyup for Alphanumeric key, which means you are leaving Alphanumeric state. BUG=612694 ==========
Description was changed from ========== [DomKey] Support Japanese (JIS) layout special keys This CL adds mappings for for keys specific to Japanese (JIS) layouts. 1. "半角/全角\n漢字" (Hankaku/Zenkaku/KanjiMode, to the left of Digit1) 2. "Caps Lock\n英数" (Alphanumeric/Hiragana, same location as CapsLock) 3. "カタカナ\nひらがな\nローマ字" (Katakana/Hiragana/Romaji, to the left of AltRight) (We don't need to handle Convert/NonConvert here but could simply put them into the generic mapping table. The reason is their VKEYs are unique, and the OS won't generate those VKEYs if they are not supported in current layout.) Since the VKEYs for these keys express their correct meanings, taking into account modifiers and lock states, we can use a static table to map them to DomKey values. See the BUG for how JIS keyboard works and how to produce these values... Note: Sometimes pressing a key will cause 'keyup' events that does not match this key, this is known and actually means we are leaving the previous state. e.g. Press Alphanumeric key first, and then if you hit Hiragana key you will actually get a keyup for Alphanumeric key, which means you are leaving Alphanumeric state. BUG=612694 ========== to ========== [DomKey] Support Japanese (JIS) layout special keys This CL adds mappings for for keys specific to Japanese (JIS) layouts: 1. "半角/全角\n漢字" (DomKey::HANKAKU/ZENKAKU/KANJI_MODE, DomCode::BACKQUOTE, to the left of Digit1) Used to switch between Hankaku, Zenkaku and KanjiMode. 2. "Caps Lock\n英数" (DomKey::ALPHANUMERIC/HIRAGANA, DomCode::CAPS_LOCK, same location as CapsLock) Used to switch between Alphanumeric and Hiragana. 3. "カタカナ\nひらがな\nローマ字" (DomKey::KATAKANA/HIRAGANA/ROMAJI, DomCode::KANA_MODE, to the left of AltRight) Used to switch between Katakana, Hiragana and Romaji. (We don't need to handle Convert/NonConvert specially but could simply put them into the generic mapping table. The reason is their VKEYs are unique, and the OS won't produce those VKEYs if they are not supported by current layout. e.g. Will produce 0xff on US layout) Since the VKEYs for these keys express their correct meanings, taking into account modifiers and lock states, we can use a static table to map them to DomKey values. See the BUG for how JIS keyboard works and how to produce these values... Note: Sometimes pressing a key will cause 'keyup' events that does not match this key, this is known and actually means we are leaving the previous state. e.g. Press Alphanumeric key first, and then if you hit Hiragana key you will actually get a keyup for Alphanumeric key, which means you are leaving Alphanumeric state. BUG=612694 ==========
Description was changed from ========== [DomKey] Support Japanese (JIS) layout special keys This CL adds mappings for for keys specific to Japanese (JIS) layouts: 1. "半角/全角\n漢字" (DomKey::HANKAKU/ZENKAKU/KANJI_MODE, DomCode::BACKQUOTE, to the left of Digit1) Used to switch between Hankaku, Zenkaku and KanjiMode. 2. "Caps Lock\n英数" (DomKey::ALPHANUMERIC/HIRAGANA, DomCode::CAPS_LOCK, same location as CapsLock) Used to switch between Alphanumeric and Hiragana. 3. "カタカナ\nひらがな\nローマ字" (DomKey::KATAKANA/HIRAGANA/ROMAJI, DomCode::KANA_MODE, to the left of AltRight) Used to switch between Katakana, Hiragana and Romaji. (We don't need to handle Convert/NonConvert specially but could simply put them into the generic mapping table. The reason is their VKEYs are unique, and the OS won't produce those VKEYs if they are not supported by current layout. e.g. Will produce 0xff on US layout) Since the VKEYs for these keys express their correct meanings, taking into account modifiers and lock states, we can use a static table to map them to DomKey values. See the BUG for how JIS keyboard works and how to produce these values... Note: Sometimes pressing a key will cause 'keyup' events that does not match this key, this is known and actually means we are leaving the previous state. e.g. Press Alphanumeric key first, and then if you hit Hiragana key you will actually get a keyup for Alphanumeric key, which means you are leaving Alphanumeric state. BUG=612694 ========== to ========== [DomKey] Support Japanese (JIS) layout special keys This CL adds mappings for for keys specific to Japanese (JIS) layouts: 1. "半角/全角\n漢字" (DomKey::HANKAKU/ZENKAKU/KANJI_MODE, DomCode::BACKQUOTE, to the left of Digit1) Used to switch between Hankaku, Zenkaku and KanjiMode, may use with Alt. 2. "Caps Lock\n英数" (DomKey::ALPHANUMERIC/HIRAGANA, DomCode::CAPS_LOCK, same location as CapsLock) Used to switch between Alphanumeric and Hiragana. 3. "カタカナ\nひらがな\nローマ字" (DomKey::KATAKANA/HIRAGANA/ROMAJI, DomCode::KANA_MODE, to the left of AltRight) Used to switch between Katakana, Hiragana and Romaji, may use with Alt or Shift (We don't need to handle Convert/NonConvert specially but could simply put them into the generic mapping table. The reason is their VKEYs are unique, and the OS won't produce those VKEYs if they are not supported by current layout. e.g. Will produce 0xff on US layout) Since the VKEYs for these keys express their correct meanings, taking into account modifiers and lock states, we can use a static table to map them to DomKey values. See the BUG for how JIS keyboard works and how to produce these values... Note: Sometimes pressing a key will cause 'keyup' events that does not match this key, this is known and actually means we are leaving the previous state. e.g. Press Alphanumeric key first, and then if you hit Hiragana key you will actually get a keyup for Alphanumeric key, which means you are leaving Alphanumeric state. BUG=612694 ==========
Description was changed from ========== [DomKey] Support Japanese (JIS) layout special keys This CL adds mappings for for keys specific to Japanese (JIS) layouts: 1. "半角/全角\n漢字" (DomKey::HANKAKU/ZENKAKU/KANJI_MODE, DomCode::BACKQUOTE, to the left of Digit1) Used to switch between Hankaku, Zenkaku and KanjiMode, may use with Alt. 2. "Caps Lock\n英数" (DomKey::ALPHANUMERIC/HIRAGANA, DomCode::CAPS_LOCK, same location as CapsLock) Used to switch between Alphanumeric and Hiragana. 3. "カタカナ\nひらがな\nローマ字" (DomKey::KATAKANA/HIRAGANA/ROMAJI, DomCode::KANA_MODE, to the left of AltRight) Used to switch between Katakana, Hiragana and Romaji, may use with Alt or Shift (We don't need to handle Convert/NonConvert specially but could simply put them into the generic mapping table. The reason is their VKEYs are unique, and the OS won't produce those VKEYs if they are not supported by current layout. e.g. Will produce 0xff on US layout) Since the VKEYs for these keys express their correct meanings, taking into account modifiers and lock states, we can use a static table to map them to DomKey values. See the BUG for how JIS keyboard works and how to produce these values... Note: Sometimes pressing a key will cause 'keyup' events that does not match this key, this is known and actually means we are leaving the previous state. e.g. Press Alphanumeric key first, and then if you hit Hiragana key you will actually get a keyup for Alphanumeric key, which means you are leaving Alphanumeric state. BUG=612694 ========== to ========== [DomKey] Support Japanese (JIS) layout special keys This CL adds mappings for for keys specific to Japanese (JIS) layouts: 1. "半角/全角\n漢字" (DomKey::HANKAKU/ZENKAKU/KANJI_MODE, DomCode::BACKQUOTE, to the left of Digit1) Used to switch between Hankaku, Zenkaku and KanjiMode, may use with Alt. 2. "Caps Lock\n英数" (DomKey::ALPHANUMERIC/HIRAGANA, DomCode::CAPS_LOCK, same location as CapsLock) Used to switch between Alphanumeric and Hiragana. 3. "カタカナ\nひらがな\nローマ字" (DomKey::KATAKANA/HIRAGANA/ROMAJI, DomCode::KANA_MODE, to the left of AltRight) Used to switch between Katakana, Hiragana and Romaji, may use with Alt or Shift (We don't need to handle Convert/NonConvert specially but could simply put them into the generic mapping table. The reason is their VKEYs are unique and clear - VK_CONVERT/VK_NONCONVERT, and the OS won't produce those VKEYs if they are not supported by current layout. e.g. Will produce 0xff on US layout) Since the VKEYs for these keys express their correct meanings, taking into account modifiers and lock states, we can use a static table to map them to DomKey values. See the BUG for how JIS keyboard works and how to produce these values... Note: Sometimes pressing a key will cause 'keyup' events that does not match this key, this is known and actually means we are leaving the previous state. e.g. Press Alphanumeric key first, and then if you hit Hiragana key you will actually get a keyup for Alphanumeric key, which means you are leaving Alphanumeric state. BUG=612694 ==========
Thanks for the detailed review! :D
PTAL again, thanks!
On 2016/07/07 18:25:51, Wez wrote:
> This mostly looks great - just some minor nits and mainly comments on the CL
> description :D
>
> // This CL added mapping for JIS keyboard special keys:
>
> nit: added->adds, mapping->mappings
Done.
> I'd also suggest saying "for keys specific to Japanese (JIS) layouts." since
not
> everyone is familiar with the "JIS" term.
Done.
>
> // 1. "半角/全角\n漢字" (to the left of Digit1)
> // 2. "Caps Lock\n英数" (same location as "Caps Lock" key)
> // 3. "カタカナ\nひらがな\nローマ字" (to the left of right Alt)
>
> nit: It's not clear as a non-Japanese-reader which each of these cases
> correspond to in the code, and the location-based descriptions aren't all that
> helpful out of context! - I'd suggest providing the key-cap names as you have
> done here, then providing the DOM |key| name in brackets, the location
> information that you've provided, and then a second line e.g. "Used to switch
> between Romaji/Hirigana/Katakana" etc, as appropriate.
Done. Added DomKey, DomCode and usage.
>
> nit: In your location descriptions, use the DOM key or code names for the keys
> consistently, e.g. Digit1, CapsLock and AltRight in the above.
Done.
> // ("Convert"/"NonConvert" has their own VKEYs so could be handled as general
> keys)
>
> nit: Clarify why we still prefer to handle them specially - presumably so that
> English US generates UNIDENTIFIED for them?
>
Added more description, actually we prefer not to handle them specially.
(We don't need to handle Convert/NonConvert specially but could simply put them
into the generic mapping table. The reason is their VKEYs are unique and clear -
VK_CONVERT/VK_NONCONVERT, and the OS won't produce those VKEYs if they are not
supported by current layout. e.g. Will produce 0xff on US layout)
> // We can do the static mapping from |VKEY| to |DomKey|, and OS will handle
the
> // modifiers & state to provide correct |VKEY|.
>
> nit: Suggest "Since the VKEYs for these keys express their correct meanings,
> taking into account modifiers and lock states, we can use a static table to
map
> them to DomKey values."
Done.
>
> // After CL we will match Firefox.
>
> nit: This seems redundant (and technically a non-goal - the aim for all the
> browsers should be to match the spec :P).
Removed...
>
> // See https://crbug.com/612694 for how JIS keyboard works and how to produce
> these values...
>
> nit: You can just say "see the BUG" since it's in the BUG= line, rather than
> duplicating the number here. :)
>
Done.
> // Note:
> // Sometimes it will generate a 'keyup' for another key, this means we are
> leaving the // that state.
>
> IIUC you're just saying that the DomKey value may change if the key is down at
> the moment that the layout changes, e.g. en_US->jp_JP, and that this is
> expected. Is that consistent with the DOM spec?
Sorry for the confusion, I was actually trying to explain a Japanese DomKey
testing issue, where you press a key and the produced 'keyup' is not necessarily
for this key... (without changing any keyboard config)
Note:
Sometimes pressing a key will cause 'keyup' events that does not match this key,
this is known and actually means we are leaving the previous state.
e.g. Press and release Alphanumeric key first, and then if you hit Hiragana key
you will actually get a keyup for Alphanumeric key, which means you are leaving
Alphanumeric state.
---
For the layout changing case (en_US->jp_JP) DomKey could be different for
'keydown' and 'keyup' if the layout was changed in between.
I didn't find specific spec about this, but I think it should be fine since we
could even miss 'keyup' if we moved focus out of Chrome and release key.
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_codes_posix.h (right):
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_codes_posix.h:201: VKEY_OEM_COPY = 0xF2,
On 2016/07/07 18:25:51, Wez wrote:
> Since these have not-very-helpful names, can we add comments to explain their
> actual use, as we have done above for the GTV OEM codes?
Done.
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
File ui/events/keycodes/platform_key_map_win.cc (right):
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win.cc:215: return DomKey::KANA_MODE;
On 2016/07/07 18:25:51, Wez wrote:
> On 2016/07/06 22:50:34, chongz wrote:
> > |VKEY_KANA| is not used with modern Japanese keyboard (they use |VKEY_ATTN|
> > instead), but we want to match FF and IE to support it.
> >
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fddca814e0#l1.146
>
> When you say "modern keyboard" do you mean modern Japanese layouts have
switched
> to VKEY_ATTN?
Actually FireFox says so but I cannot find more info about VK_KANA. However at
least my keyboard is producing VK_ATTN on Win 10, and I cannot figure out how to
produce VK_KANA using it.
>
> Suggest merging this with the VKEY_ATTN switch case, so it's more obvious that
> the two values generate the same output.
>
> You might add a comment saying that some Japanese layouts used VKEY_KANA for
> KANA_MODE, while others use VKEY_ATTN, so we map them both for compatibility.
Done.
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
File ui/events/keycodes/platform_key_map_win_unittest.cc (right):
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:49: return
reinterpret_cast<HKL>(0x04110411);
On 2016/07/07 18:25:51, Wez wrote:
> LoadKeyboardLayout doesn't work here?
Yes |LoadKeyboardLayout()| won't pass DrMemory test for Japanese and Korean.
I haven't started looking how to add DrMemory test suppression, but it should be
fine as is since we are not using |ToUnicodeEx()|.
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:120: int flags) {
On 2016/07/07 18:25:51, Wez wrote:
> What's the point of this "helper"? It just seems to convert between
> function-call and method-call syntax..?
Because |PlatformKeyMap::DomKeyFromKeyboardCodeImpl| is private and I cannot
access it in "TEST_F(PlatformKeyMapTest, ECT) {...}".
Added a comment:
// Need this helper function to access private methods of |PlatformKeyMap|.
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:359: PlatformKeyMap
us_keymap(us_layout);
On 2016/07/07 18:25:51, Wez wrote:
> nit: Here and elsewhere, I think it's OK to pass the result of
GetInputLocale()
> directly to the PlatformKeyMap constructor.
Done. Except one place for DomCode=>Key testing.
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:360: for (const auto&
test_case : kJapaneseTestCases) {
On 2016/07/07 18:25:51, Wez wrote:
> Suggest folding these two loops into one, for clarity - create the two layouts
> and then loop over the cases, doing two EXPECT_EQ()s in each iteration, one
for
> us, one for Japanese.
Done.
https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:368: PlatformKeyMap
ko_keymap(jp_layout);
On 2016/07/07 18:25:51, Wez wrote:
> jp_keymap?
Done.
lgtm https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat... File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win.cc:215: return DomKey::KANA_MODE; On 2016/07/07 22:18:09, chongz wrote: > On 2016/07/07 18:25:51, Wez wrote: > > On 2016/07/06 22:50:34, chongz wrote: > > > |VKEY_KANA| is not used with modern Japanese keyboard (they use |VKEY_ATTN| > > > instead), but we want to match FF and IE to support it. > > > > > > https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fddca814e0#l1.146 > > > > When you say "modern keyboard" do you mean modern Japanese layouts have > switched > > to VKEY_ATTN? > Actually FireFox says so but I cannot find more info about VK_KANA. However at > least my keyboard is producing VK_ATTN on Win 10, and I cannot figure out how to > produce VK_KANA using it. That's really unfortunate. It sounds like the Firefox rationale was that it exists and when they injected it into IE it had generated KanaMode, so we should make sure our comment reflects that, e.g: "VKEY_KANA isn't generated by any modern layouts but is a listed value that third-party apps might synthesize, so we handle it anyway." https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat... File ui/events/keycodes/platform_key_map_win_unittest.cc (right): https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win_unittest.cc:49: return reinterpret_cast<HKL>(0x04110411); On 2016/07/07 22:18:09, chongz wrote: > On 2016/07/07 18:25:51, Wez wrote: > > LoadKeyboardLayout doesn't work here? > > Yes |LoadKeyboardLayout()| won't pass DrMemory test for Japanese and Korean. > > I haven't started looking how to add DrMemory test suppression, but it should be > fine as is since we are not using |ToUnicodeEx()|. OK, what I was missing is that we just pull the "primary language" out of this value, without calling any Windows APIs (since we don't look at any printable keys), so it's OK not to have the layouts actually loaded. https://codereview.chromium.org/2128573002/diff/20001/ui/events/keycodes/plat... ui/events/keycodes/platform_key_map_win_unittest.cc:120: int flags) { On 2016/07/07 22:18:09, chongz wrote: > On 2016/07/07 18:25:51, Wez wrote: > > What's the point of this "helper"? It just seems to convert between > > function-call and method-call syntax..? > > Because |PlatformKeyMap::DomKeyFromKeyboardCodeImpl| is private and I cannot > access it in "TEST_F(PlatformKeyMapTest, ECT) {...}". > > Added a comment: > // Need this helper function to access private methods of |PlatformKeyMap|. Acknowledged.
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/2128573002/#ps60001 (title: "Wez's review 2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [DomKey] Support Japanese (JIS) layout special keys This CL adds mappings for for keys specific to Japanese (JIS) layouts: 1. "半角/全角\n漢字" (DomKey::HANKAKU/ZENKAKU/KANJI_MODE, DomCode::BACKQUOTE, to the left of Digit1) Used to switch between Hankaku, Zenkaku and KanjiMode, may use with Alt. 2. "Caps Lock\n英数" (DomKey::ALPHANUMERIC/HIRAGANA, DomCode::CAPS_LOCK, same location as CapsLock) Used to switch between Alphanumeric and Hiragana. 3. "カタカナ\nひらがな\nローマ字" (DomKey::KATAKANA/HIRAGANA/ROMAJI, DomCode::KANA_MODE, to the left of AltRight) Used to switch between Katakana, Hiragana and Romaji, may use with Alt or Shift (We don't need to handle Convert/NonConvert specially but could simply put them into the generic mapping table. The reason is their VKEYs are unique and clear - VK_CONVERT/VK_NONCONVERT, and the OS won't produce those VKEYs if they are not supported by current layout. e.g. Will produce 0xff on US layout) Since the VKEYs for these keys express their correct meanings, taking into account modifiers and lock states, we can use a static table to map them to DomKey values. See the BUG for how JIS keyboard works and how to produce these values... Note: Sometimes pressing a key will cause 'keyup' events that does not match this key, this is known and actually means we are leaving the previous state. e.g. Press Alphanumeric key first, and then if you hit Hiragana key you will actually get a keyup for Alphanumeric key, which means you are leaving Alphanumeric state. BUG=612694 ========== to ========== [DomKey] Support Japanese (JIS) layout special keys This CL adds mappings for for keys specific to Japanese (JIS) layouts: 1. "半角/全角\n漢字" (DomKey::HANKAKU/ZENKAKU/KANJI_MODE, DomCode::BACKQUOTE, to the left of Digit1) Used to switch between Hankaku, Zenkaku and KanjiMode, may use with Alt. 2. "Caps Lock\n英数" (DomKey::ALPHANUMERIC/HIRAGANA, DomCode::CAPS_LOCK, same location as CapsLock) Used to switch between Alphanumeric and Hiragana. 3. "カタカナ\nひらがな\nローマ字" (DomKey::KATAKANA/HIRAGANA/ROMAJI, DomCode::KANA_MODE, to the left of AltRight) Used to switch between Katakana, Hiragana and Romaji, may use with Alt or Shift (We don't need to handle Convert/NonConvert specially but could simply put them into the generic mapping table. The reason is their VKEYs are unique and clear - VK_CONVERT/VK_NONCONVERT, and the OS won't produce those VKEYs if they are not supported by current layout. e.g. Will produce 0xff on US layout) Since the VKEYs for these keys express their correct meanings, taking into account modifiers and lock states, we can use a static table to map them to DomKey values. See the BUG for how JIS keyboard works and how to produce these values... Note: Sometimes pressing a key will cause 'keyup' events that does not match this key, this is known and actually means we are leaving the previous state. e.g. Press Alphanumeric key first, and then if you hit Hiragana key you will actually get a keyup for Alphanumeric key, which means you are leaving Alphanumeric state. BUG=612694 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [DomKey] Support Japanese (JIS) layout special keys This CL adds mappings for for keys specific to Japanese (JIS) layouts: 1. "半角/全角\n漢字" (DomKey::HANKAKU/ZENKAKU/KANJI_MODE, DomCode::BACKQUOTE, to the left of Digit1) Used to switch between Hankaku, Zenkaku and KanjiMode, may use with Alt. 2. "Caps Lock\n英数" (DomKey::ALPHANUMERIC/HIRAGANA, DomCode::CAPS_LOCK, same location as CapsLock) Used to switch between Alphanumeric and Hiragana. 3. "カタカナ\nひらがな\nローマ字" (DomKey::KATAKANA/HIRAGANA/ROMAJI, DomCode::KANA_MODE, to the left of AltRight) Used to switch between Katakana, Hiragana and Romaji, may use with Alt or Shift (We don't need to handle Convert/NonConvert specially but could simply put them into the generic mapping table. The reason is their VKEYs are unique and clear - VK_CONVERT/VK_NONCONVERT, and the OS won't produce those VKEYs if they are not supported by current layout. e.g. Will produce 0xff on US layout) Since the VKEYs for these keys express their correct meanings, taking into account modifiers and lock states, we can use a static table to map them to DomKey values. See the BUG for how JIS keyboard works and how to produce these values... Note: Sometimes pressing a key will cause 'keyup' events that does not match this key, this is known and actually means we are leaving the previous state. e.g. Press Alphanumeric key first, and then if you hit Hiragana key you will actually get a keyup for Alphanumeric key, which means you are leaving Alphanumeric state. BUG=612694 ========== to ========== [DomKey] Support Japanese (JIS) layout special keys This CL adds mappings for for keys specific to Japanese (JIS) layouts: 1. "半角/全角\n漢字" (DomKey::HANKAKU/ZENKAKU/KANJI_MODE, DomCode::BACKQUOTE, to the left of Digit1) Used to switch between Hankaku, Zenkaku and KanjiMode, may use with Alt. 2. "Caps Lock\n英数" (DomKey::ALPHANUMERIC/HIRAGANA, DomCode::CAPS_LOCK, same location as CapsLock) Used to switch between Alphanumeric and Hiragana. 3. "カタカナ\nひらがな\nローマ字" (DomKey::KATAKANA/HIRAGANA/ROMAJI, DomCode::KANA_MODE, to the left of AltRight) Used to switch between Katakana, Hiragana and Romaji, may use with Alt or Shift (We don't need to handle Convert/NonConvert specially but could simply put them into the generic mapping table. The reason is their VKEYs are unique and clear - VK_CONVERT/VK_NONCONVERT, and the OS won't produce those VKEYs if they are not supported by current layout. e.g. Will produce 0xff on US layout) Since the VKEYs for these keys express their correct meanings, taking into account modifiers and lock states, we can use a static table to map them to DomKey values. See the BUG for how JIS keyboard works and how to produce these values... Note: Sometimes pressing a key will cause 'keyup' events that does not match this key, this is known and actually means we are leaving the previous state. e.g. Press Alphanumeric key first, and then if you hit Hiragana key you will actually get a keyup for Alphanumeric key, which means you are leaving Alphanumeric state. BUG=612694 Committed: https://crrev.com/9d990a45f12393c2e557d4b33116b5ce1022b403 Cr-Commit-Position: refs/heads/master@{#404715} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9d990a45f12393c2e557d4b33116b5ce1022b403 Cr-Commit-Position: refs/heads/master@{#404715} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
