linux: Do not fallback to the hardware keycodes for modifers.

linux: Do not fallback to the hardware keycodes for modifiers.

Previously Chrome would fall-back to deriving VKEY values directly from the hardware keycode if it was unable to derive a value from the X KeySym. While that is necessary for e.g. providing valid VKEYs for printable character keys under non-Latin keyboard layouts (such as Hebrew or Arabic), it had the side-effect of causing KeySyms with no VKEY equivalent to be mapped to incorrect meanings.

In issue 402320, for example, the ISO_Level3_Latch KeySym has no VKEY equivalent, so should result in VKEY_UNKNOWN, but was instead having an incorrect VKEY derived from the X11 keycode instead.

Note that there is no clear reason why we don't allow the modifier keys to fall back to the hardware keycode while we allow the printable character keys to fall back.  It's just close to what users expect.  There could be a problem if a keyboard layout had a composition character on Control + Ч (which falls back to VKEY_X).  Users cannot type that composition character because a Cut accelerator triggers instead.

This approach is a heuristic designed to approximate the VKEY values that a Windows system would generate, to ensure compatibility.

BUG=402320
TEST=Done manually (assigning ISO_Level3_Latch to key code 108).

Committed: https://crrev.com/83c2b41f1a06ca226a05edf0602d5c6efea32a8b
Cr-Commit-Position: refs/heads/master@{#298478}

[Yuki, 2014-09-29 09:49:31]

yukishiino@chromium.org changed reviewers:
+ shuchen@chromium.org, wez@chromium.org

[Yuki, 2014-09-29 09:49:31]

Could you guys review this CL?
(wez@ as an owner and shuchen@ just fyi.)

This CL will fix Issue 402320 , and I'd like to merge this patch into M39 if
possible.

I'm not 100% sure, but I think theoretically that this patch should be applied
to CrOS, too.  There is no reason to treat the hardware keycodes.

Since I'd like to apply this patch to M39, I limited this patch not to affect
CrOS.

[Wez, 2014-09-29 16:21:53]

If we're making this change for Linux then yes, it seems we should make it
for ChromeOS as well.

It would be useful to know why we had this fallback in the first place -
perhaps git blame can help you find whoever implemented the defaults hack?
On 29 Sep 2014 10:49, <yukishiino@chromium.org> wrote:

> Reviewers: Wez, Shu Chen,
>
> Message:
> Could you guys review this CL?
> (wez@ as an owner and shuchen@ just fyi.)
>
> This CL will fix Issue 402320 , and I'd like to merge this patch into M39
> if
> possible.
>
> I'm not 100% sure, but I think theoretically that this patch should be
> applied
> to CrOS, too.  There is no reason to treat the hardware keycodes.
>
> Since I'd like to apply this patch to M39, I limited this patch not to
> affect
> CrOS.
>
> Description:
> linux: Do not fallback to the hardware keycodes.
>
> This CL makes Chrome never fall back to the hardware keycodes.
>
> On X11 environment, it's meaningless or even harmful to fall back to
> the hardware keycodes.  All the keys must be assigned to X key code
> (e.g. XK_xxx or XK_VoidSymbol), and we have to treat the keys as they
> are assigned to.  If we don't have a corresponding VKEY_xxx, then
> we should treat the key as VKEY_UNKNOWN, because it's just an unknown
> key.
>
> If we fallback to a hardware keycode for such an unknown key, something
> unexpected happens.  For example, if keycode 108 is assigned to
> ISO_Level3_Latch, which is an unknown key for us for now, and if it falls
> back to VKEY_RMENU (a default key mapping defined by hardware keycodes),
> we end up to open a menu while latching international modifier key is
> expected.
>
> Again, on X11 environment, only XK_xxx must be reliable info, and we must
> never care of hardware keycodes, which must be taken care by X11 key
> mappings.
>
> BUG=402320
> TEST=Done manually (assigning ISO_Level3_Latch to key code 108).
>
> Please review this at https://codereview.chromium.org/611993002/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+10, -4 lines):
>   M ui/events/keycodes/keyboard_code_conversion_x.h
>   M ui/events/keycodes/keyboard_code_conversion_x.cc
>
>
> Index: ui/events/keycodes/keyboard_code_conversion_x.cc
> diff --git a/ui/events/keycodes/keyboard_code_conversion_x.cc
> b/ui/events/keycodes/keyboard_code_conversion_x.cc
> index 32f364db9b5a81403c81d212e97b1423e223cb7c..
> a49d081c02d10fd2996aef569c4ccb6f5c1770b6 100644
> --- a/ui/events/keycodes/keyboard_code_conversion_x.cc
> +++ b/ui/events/keycodes/keyboard_code_conversion_x.cc
> @@ -27,6 +27,11 @@ namespace ui {
>
>  namespace {
>
> +#if defined(OS_CHROMEOS)
> +// Converts an X keycode into ui::KeyboardCode.
> +KeyboardCode DefaultKeyboardCodeFromHardwareKeycode(unsigned int
> hardware_code);
> +#endif
> +
>  // MAP0 - MAP3:
>  // These are the generated VKEY code maps for all possible Latin keyboard
>  // layouts in Windows. And the maps are only for special letter keys
> excluding
> @@ -475,6 +480,7 @@ KeyboardCode KeyboardCodeFromXKeyEvent(const XEvent*
> xev) {
>    // 7. If not found, fallback to find in KeyboardCodeFromXKeysym(), which
>    //    mainly for non-letter keys.
>    // 8. If not found, fallback to find with the hardware code in US
> layout.
> +  //    (CrOS only)
>
>    KeySym keysym = NoSymbol;
>    XEvent xkeyevent = {0};
> @@ -545,8 +551,10 @@ KeyboardCode KeyboardCodeFromXKeyEvent(const XEvent*
> xev) {
>    }
>
>    keycode = KeyboardCodeFromXKeysym(keysym);
> +#if defined(OS_CHROMEOS)
>    if (keycode == VKEY_UNKNOWN)
>      keycode = DefaultKeyboardCodeFromHardwareKeycode(xkey->keycode);
> +#endif
>
>    return keycode;
>  }
> @@ -862,6 +870,7 @@ uint16 GetCharacterFromXEvent(const XEvent* xev) {
>    return GetUnicodeCharacterFromXKeySym(keysym);
>  }
>
> +#if defined(OS_CHROMEOS)
>  KeyboardCode DefaultKeyboardCodeFromHardwareKeycode(
>      unsigned int hardware_code) {
>    // This function assumes that X11 is using evdev-based keycodes.
> @@ -1028,6 +1037,7 @@ KeyboardCode DefaultKeyboardCodeFromHardwareKeycode(
>    }
>    return kHardwareKeycodeMap[hardware_code];
>  }
> +#endif
>
>  // TODO(jcampan): this method might be incomplete.
>  int XKeysymForWindowsKeyCode(KeyboardCode keycode, bool shift) {
> Index: ui/events/keycodes/keyboard_code_conversion_x.h
> diff --git a/ui/events/keycodes/keyboard_code_conversion_x.h
> b/ui/events/keycodes/keyboard_code_conversion_x.h
> index 162669124db09331d99dc7b68dae7606b1462b37..
> 3f6d5f06ded3bd5061f284ca1f23f5daf6372da6 100644
> --- a/ui/events/keycodes/keyboard_code_conversion_x.h
> +++ b/ui/events/keycodes/keyboard_code_conversion_x.h
> @@ -35,10 +35,6 @@ EVENTS_BASE_EXPORT unsigned int
> XKeyCodeForWindowsKeyCode(KeyboardCode key_code,
>                                                            int flags,
>                                                            XDisplay*
> display);
>
> -// Converts an X keycode into ui::KeyboardCode.
> -EVENTS_BASE_EXPORT KeyboardCode
> -    DefaultKeyboardCodeFromHardwareKeycode(unsigned int hardware_code);
> -
>  // Initializes a core XKeyEvent from an XI2 key event.
>  EVENTS_BASE_EXPORT void InitXKeyEventFromXIDeviceEvent(const XEvent& src,
>                                                         XEvent* dst);
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Shu Chen, 2014-09-30 01:28:06]

It's me. My cl https://codereview.chromium.org/357613002/ changed the original
behaviors.

Originally the code looks like:
KeyboardCode keycode = KeyboardCodeFromXKeysym(keysym);
  if (keycode == VKEY_UNKNOWN) {
    keysym = DefaultXKeysymFromHardwareKeycode(xev->xkey.keycode);
    keycode = KeyboardCodeFromXKeysym(keysym);
  }
It does the maps: hardware_code -> keysym -> VKEY_code.
I thought it was wasting CPU and could be better to: hardware_code -> VKEY_code.

Wez, do you think we can completely remove that fallback for Chrome OS?

Thanks,
Shu

On 2014/09/29 16:21:53, Wez wrote:
> If we're making this change for Linux then yes, it seems we should make it
> for ChromeOS as well.
> 
> It would be useful to know why we had this fallback in the first place -
> perhaps git blame can help you find whoever implemented the defaults hack?

[Yuki, 2014-09-30 06:40:43]

> On 2014/09/29 16:21:53, Wez wrote:
> > If we're making this change for Linux then yes, it seems we should make it
> > for ChromeOS as well.
> > 
> > It would be useful to know why we had this fallback in the first place -
> > perhaps git blame can help you find whoever implemented the defaults hack?

https://codereview.chromium.org/6674052
This is the first CL which introduced the fallback to the hardware keycodes. 
The purpose was to support accelerator keys in the same manner as US keyboard
layout.

Now I've got better understanding why we needed this fallback, so I've updated
the CL accordingly.  We'll fallback to the US layout only when Control is held. 
Otherwise, we must not fallback to the US layout.

Also see my comment in keyboard_code_conversion_x.cc for an use case.

[Yuki, 2014-09-30 06:49:41]

Patchset #4 (id:60001) has been deleted

[Shu Chen, 2014-09-30 07:23:51]

lgtm

[Yuki, 2014-10-01 07:09:48]

wez@, could you take another look?

[Yuki, 2014-10-02 14:44:41]

I had some discussions with wez@ and found that we need to emit JS key events
with a meaningful keycode as much as possible.  So I totally changed the
approach.  Could you guys take another look at this CL?

Patch Set 5 contains small refactoring without any fix.  It moved the content of
KeyboardCodeFromXKeysym into KeyboardCodeFromXKeysymOrHardwareKeycode as
preparation.

Patch Set 6 adds an actual fix.  You can see the diff between Patch Set 5 and 6,
and see what keycodes are added to return VKEY_UNKNOWN.

KeyboardCodeFromXKeysym is exported to public, so I didn't change its behavior.

Could you review this CL again?

[Shu Chen, 2014-10-02 16:01:07]

I have no problem with the solution. lgtm.

[Wez, 2014-10-03 12:38:44]

The CL description doesn't make clear why the solution is not simply to handle
ISO_Level3_Latch. :) It's also not clear under what circumstances that keysym
would appear in the layout?

General observation, not blocking this CL:
This code has ended up pretty fiendishly constructed, thanks to us trying to
derive a balance between the (arbitrary) local keymap and the Windows VKEY
output; we may do better to hard-code some VKEY tables for sets of language
layouts (the vast majority look like US English at the VKEY level, so that's not
as bad as it sounds).

https://codereview.chromium.org/611993002/diff/180001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_code_conversion_x.cc (right):

https://codereview.chromium.org/611993002/diff/180001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion_x.cc:15: #if !defined(XK_dead_greek)
Under what circumstances will this be undefined?

https://codereview.chromium.org/611993002/diff/180001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion_x.cc:843: case XK_Hyper_L:
Regardless of where this list of always-UNKNOWN codes live, it needs a comment
to explain why we can't fall-back to mapping via hardware codes for these.

https://codereview.chromium.org/611993002/diff/180001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion_x.cc:1048: // 8. If not found,
fallback to find with the hardware code in US layout.
Looks like steps #7 and #8 are now folded into FromXKeySymOrHardwareKeycode(),
which makes this comment a little confusing.

It would be clearer to keep the "special keys that must return VKEY_UNKNOWN"in a
separate function that we call to determine whether to call on to
KeyboardCodeFromHardwareKeycode, I think.

[Yuki, 2014-10-03 14:04:23]

Patchset #10 (id:200001) has been deleted

[Yuki, 2014-10-03 14:33:47]

Thanks for the review.  I revised the code.  Could you review again?

> The CL description doesn't make clear why the solution is not simply to handle
ISO_Level3_Latch.

I updated the CL description, but if it's not enough clear, could you help
improve it?

> It's also not clear under what circumstances that keysym
would appear in the layout?

I don't know exactly which keyboard layouts assign ISO_Level3_Latch, etc. to
keycode 108 (or any other problematic keycode).  It seems that many of (or some
of?) european keyboard layouts have these modifier keys to input accented latin
characters.  I vaguely remember Portuguese, German, Belgian, etc. users
complained the issue (but not sure).

Plus, some users assign these modifier keys for themselves as part of keyboard
layout customization.  They also complained.

https://codereview.chromium.org/611993002/diff/180001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_code_conversion_x.cc (right):

https://codereview.chromium.org/611993002/diff/180001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion_x.cc:15: #if !defined(XK_dead_greek)
On 2014/10/03 12:38:44, Wez wrote:
> Under what circumstances will this be undefined?

Added a comment.

XK_dead_greek was recently added and old versions of X11 don't define this
keysym.  I don't know that exactly which version first defined this keysym.  My
local machine (Ubuntsu 14) has this definition, but trybots (Ubuntsu 12?) don't
have the definition.

https://codereview.chromium.org/611993002/diff/180001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion_x.cc:843: case XK_Hyper_L:
On 2014/10/03 12:38:44, Wez wrote:
> Regardless of where this list of always-UNKNOWN codes live, it needs a comment
> to explain why we can't fall-back to mapping via hardware codes for these.

Added a comment.

https://codereview.chromium.org/611993002/diff/180001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion_x.cc:1048: // 8. If not found,
fallback to find with the hardware code in US layout.
On 2014/10/03 12:38:44, Wez wrote:
> Looks like steps #7 and #8 are now folded into FromXKeySymOrHardwareKeycode(),
> which makes this comment a little confusing.
> 
> It would be clearer to keep the "special keys that must return VKEY_UNKNOWN"in
a
> separate function that we call to determine whether to call on to
> KeyboardCodeFromHardwareKeycode, I think.

Thanks for the suggestion.  I made a separate function named
IsHardwareKeycodeFallbackAllowed.

[Wez, 2014-10-03 17:50:29]

Re the CL description:

--
linux: Do not fallback to the hardware keycodes for modifiers.

This CL Makes Chrome not fall back to the hardware keycodes for
modifier keys and dead keys.
--

This seems a reasonable description of the change the CL actually makes.

--
The cause of Issue 402320 is that, when keycode 108 is assigned to
ISO_Level3_Latch, which is unknown keysym for Chrome, we fall back to
VKEY_RMENU from keycode 108, and end up to focus the hamburger menu
because VKEY_RMENU is an accelerator key to focus or open a menu.
--

nit: I think it's the "hotdog menu", not the "hamburger menu".

Suggest rewording:
"Previously Chrome would fall-back to deriving VKEY values directly from the
hardware keycode if it was unable to derive a value from the X KeySym. While
that is necessary for e.g. providing valid VKEYs for printable character keys
under non-Latin keyboard layouts (such as Hebrew or Arabic), it had the
side-effect of causing KeySyms with no VKEY equivalent to be mapped to incorrect
meanings.

In issue 402320, for example, the ISO_Level3_Latch KeySym has no VKEY
equivalent, so should result in VKEY_UNKNOWN, but was instead having an
incorrect VKEY derived from the X11 keycode instead."

--
For modifier keys and dead keys, we shouldn't fall back to the hardware-
keycode-based US layout.  We should treat those keys as VKEY_UNKNOWN.
--

This describes _what_ the behaviour is, but not _why_. Why should modifier keys
and dead keys not fall-back?

What does Windows generate for dead-keys, for example?  IIRC it generates
VK_PACKET for IME input, but I don't recall the dead-key values.

--
This CL adds a check if we should or shouldn't fall back to the
hardware-keycode-based US layout.  With this fix, Control + Ч (which
falls back to VKEY_X) on Cyrillic keyboard layout is still treated as
Cut accelerator key while XK_ISO_Level3_Latch is never treated as
VKEY_RMENU regardless of the hardware keycode.
--

OK, sounds good.

--
In Javascript world, users see keycode=0 for these modifier and dead
keys.  This is the same behavior as Firefox.
--

The aim is to match the behaviour of Chrome / Windows for the equivalent
language & layout configuration, as far as possible, so that Chrome is as
consistent as possible across platforms. We're not aiming to match Firefox /
Linux behaviour, in general.

[Wez, 2014-10-03 17:57:01]

https://codereview.chromium.org/611993002/diff/220001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_code_conversion_x.cc (right):

https://codereview.chromium.org/611993002/diff/220001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion_x.cc:16: // Old versions of
<X11/keysymdef.h> don't define XK_dead_greek.  If it's not
Do we ever build on platforms that don't have this, though? Isn't Ubuntu Precise
the oldest platform we support building on?

https://codereview.chromium.org/611993002/diff/220001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion_x.cc:477: // dead keys shouldn't
fall back to the hardware-key-based US layout.
nit: Suggest just "See crbug.com/<bug #>" here.

https://codereview.chromium.org/611993002/diff/220001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion_x.cc:480: case XK_Shift_R:
Why are these (and Control, etc) in here? We have explicit X KeySym -> VKEY
mappings for them, below, surely, so we'll never ever try to fall-back to h/w
for them?

Thinking about the Cyrillic case vs ISO_Level3_Shift, is it the case that we
want to fall-back only for _printable_ characters? For non-printable characters
we have to have an explicit mapping anyway.

Is there an X call we can use to check whether the KeySym is printable (whether
or not it would actually generate a character in the current context)?

[Yuki, 2014-10-06 07:50:18]

Thanks for a lot of comments.  Now we need only a few lines of code.  :)

I revised the CL description, too.

AFAICT, there is no clear reason why we need a special casing for modifier keys.
 This is a heuristic solution.  It's just because it's close to what users
expect.  Users expect that sometime keys fall back and sometimes not, and there
seems no clear definition or explanation between them.

https://codereview.chromium.org/611993002/diff/220001/ui/events/keycodes/keyb...
File ui/events/keycodes/keyboard_code_conversion_x.cc (right):

https://codereview.chromium.org/611993002/diff/220001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion_x.cc:16: // Old versions of
<X11/keysymdef.h> don't define XK_dead_greek.  If it's not
On 2014/10/03 17:57:00, Wez wrote:
> Do we ever build on platforms that don't have this, though? Isn't Ubuntu
Precise
> the oldest platform we support building on?

I found that trybots(Precise?) don't define this macro.
My local machine (Trusty) has the definition.

https://codereview.chromium.org/611993002/diff/220001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion_x.cc:477: // dead keys shouldn't
fall back to the hardware-key-based US layout.
On 2014/10/03 17:57:01, Wez wrote:
> nit: Suggest just "See crbug.com/<bug #>" here.

Done.

https://codereview.chromium.org/611993002/diff/220001/ui/events/keycodes/keyb...
ui/events/keycodes/keyboard_code_conversion_x.cc:480: case XK_Shift_R:
On 2014/10/03 17:57:01, Wez wrote:
> Why are these (and Control, etc) in here? We have explicit X KeySym -> VKEY
> mappings for them, below, surely, so we'll never ever try to fall-back to h/w
> for them?

You're right.  I wanted to define this function independent from other functions
as much as possible.

> Thinking about the Cyrillic case vs ISO_Level3_Shift, is it the case that we
> want to fall-back only for _printable_ characters? For non-printable
characters
> we have to have an explicit mapping anyway.
> 
> Is there an X call we can use to check whether the KeySym is printable
(whether
> or not it would actually generate a character in the current context)?

Yes, I think we can use IsModifierKey.

[Yuki, 2014-10-06 07:54:29]

For dead keys, I checked what keycodes are emitted on Windows, and it turned out
that we should fall back to hardware keycodes for dead keys.  It was my
misunderstanding.

IsModifierKey doesn't treat dead keys as modifier keys.

[Wez, 2014-10-07 14:39:43]

LGTM once the CL description is trimmed down!

[Yuki, 2014-10-07 14:46:34]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-10-07 14:47:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/611993002/240001

[commit-bot: I haz the power, 2014-10-07 14:56:39]

Committed patchset #11 (id:240001) as e8bab6cb2fc7694f1634b77129c7b35cfeac305b

[commit-bot: I haz the power, 2014-10-07 14:57:19]

Patchset 11 (id:??) landed as
https://crrev.com/83c2b41f1a06ca226a05edf0602d5c6efea32a8b
Cr-Commit-Position: refs/heads/master@{#298478}