aura-linux: Supports ISO Level3 Latch key.

aura-linux: Supports ISO Level3 Latch key.

XK_ISO_Level3_Latch key was treated as a character key.  This CL treats the key as same as AltGr (XK_ISO_Level3_Shift).

Note: Apostrophe key in Latvian keyboard layout (apostrophe variant) is assigned to XK_ISO_Level3_Latch.

BUG=375670
TEST=Done manually.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278646

[Yuki, 2014-06-10 09:11:04]

Could you review this CL?

[sadrul, 2014-06-11 20:45:31]

-me +wez@ as ui/events/keycodes/ owner

[Wez, 2014-06-11 22:01:03]

If XK_ISO_Level3_Latch is used for an apostrophe character on Latvian layouts,
why do we want to treat it as equivalent to XK_ISO_Level3_Shift?

[Yuki, 2014-06-12 02:47:15]

On 2014/06/11 22:01:03, Wez wrote:
> If XK_ISO_Level3_Latch is used for an apostrophe character on Latvian layouts,
> why do we want to treat it as equivalent to XK_ISO_Level3_Shift?

The equivalency doesn't matter much.

WindowTargeter::FindTargetForKeyEvent discards VKEY_UNKNOWN key events.  So, we
need to
1. assign non-zero virtual key code to XK_ISO_Level3_Shift, or
2. allow VKEY_UNKNOWN key events to pass through all event targets, including
renderers.

And some people think option 2 is a little risky because it's been long time
since we stopped sending VKEY_UNKNOWN key events.  So I chose option 1.

I thought VKEY_ALTGR is most appropriate.

[Wez, 2014-06-12 19:07:34]

On 2014/06/12 02:47:15, Yuki wrote:
> On 2014/06/11 22:01:03, Wez wrote:
> > If XK_ISO_Level3_Latch is used for an apostrophe character on Latvian
layouts,
> > why do we want to treat it as equivalent to XK_ISO_Level3_Shift?
> 
> The equivalency doesn't matter much.
> 
> WindowTargeter::FindTargetForKeyEvent discards VKEY_UNKNOWN key events.  So,
we
> need to
> 1. assign non-zero virtual key code to XK_ISO_Level3_Shift, or
> 2. allow VKEY_UNKNOWN key events to pass through all event targets, including
> renderers.
> 
> And some people think option 2 is a little risky because it's been long time
> since we stopped sending VKEY_UNKNOWN key events.  So I chose option 1.
> 
> I thought VKEY_ALTGR is most appropriate.

Why would we want to generate AltGr, if the key that generates this code is
actually a form of apostrophe? How will an app distinguish that apostrophe press
from a real AltGr press in that case?

[Yuki, 2014-06-13 03:42:47]

On 2014/06/12 19:07:34, Wez wrote:
> On 2014/06/12 02:47:15, Yuki wrote:
> > On 2014/06/11 22:01:03, Wez wrote:
> > > If XK_ISO_Level3_Latch is used for an apostrophe character on Latvian
> layouts,
> > > why do we want to treat it as equivalent to XK_ISO_Level3_Shift?
> > 
> > The equivalency doesn't matter much.
> > 
> > WindowTargeter::FindTargetForKeyEvent discards VKEY_UNKNOWN key events.  So,
> we
> > need to
> > 1. assign non-zero virtual key code to XK_ISO_Level3_Shift, or
> > 2. allow VKEY_UNKNOWN key events to pass through all event targets,
including
> > renderers.
> > 
> > And some people think option 2 is a little risky because it's been long time
> > since we stopped sending VKEY_UNKNOWN key events.  So I chose option 1.
> > 
> > I thought VKEY_ALTGR is most appropriate.
> 
> Why would we want to generate AltGr, if the key that generates this code is
> actually a form of apostrophe? How will an app distinguish that apostrophe
press
> from a real AltGr press in that case?

This patch generates VKEY_ALTGR only for XK_ISO_Level3_Latch, not for
apostrophe.

XK_apostrophe is mapped to VKEY_OEM_7 (=222), and we send keydown event with
keycode=VKEY_OEM_7 and keypress event with keycode=39 (apostrophe) regardless of
this patch.

Talking about a specific case to Latvian keyboard layout (apostrophe variant),
when you hit once the key which corresponds to apostrophe in US keyboard, X11
generates XK_ISO_Level3_Latch as a dead key, and we map it to VKEY_ALTGR.
When you hit twice the key which corresponds to apostrophe in US keyboard, X11
generates XK_apostrophe, and we map it to VKEY_OEM_7.
Of course it works well in a case that a keyboard layout has XK_ISO_Level3_Latch
and XK_apostrophe keys separately.

It's quite similar to other dead keys, and it's not surprising to send
VKEY_ALTGR for dead keys, I think.

[Wez, 2014-06-13 04:45:42]

On 2014/06/13 03:42:47, Yuki wrote:
> On 2014/06/12 19:07:34, Wez wrote:
> > On 2014/06/12 02:47:15, Yuki wrote:
> > > On 2014/06/11 22:01:03, Wez wrote:
> > > > If XK_ISO_Level3_Latch is used for an apostrophe character on Latvian
> > layouts,
> > > > why do we want to treat it as equivalent to XK_ISO_Level3_Shift?
> > > 
> > > The equivalency doesn't matter much.
> > > 
> > > WindowTargeter::FindTargetForKeyEvent discards VKEY_UNKNOWN key events. 
So,
> > we
> > > need to
> > > 1. assign non-zero virtual key code to XK_ISO_Level3_Shift, or
> > > 2. allow VKEY_UNKNOWN key events to pass through all event targets,
> including
> > > renderers.
> > > 
> > > And some people think option 2 is a little risky because it's been long
time
> > > since we stopped sending VKEY_UNKNOWN key events.  So I chose option 1.
> > > 
> > > I thought VKEY_ALTGR is most appropriate.
> > 
> > Why would we want to generate AltGr, if the key that generates this code is
> > actually a form of apostrophe? How will an app distinguish that apostrophe
> press
> > from a real AltGr press in that case?
> 
> This patch generates VKEY_ALTGR only for XK_ISO_Level3_Latch, not for
> apostrophe.
> 
> XK_apostrophe is mapped to VKEY_OEM_7 (=222), and we send keydown event with
> keycode=VKEY_OEM_7 and keypress event with keycode=39 (apostrophe) regardless
of
> this patch.
> 
> Talking about a specific case to Latvian keyboard layout (apostrophe variant),
> when you hit once the key which corresponds to apostrophe in US keyboard, X11
> generates XK_ISO_Level3_Latch as a dead key, and we map it to VKEY_ALTGR.
> When you hit twice the key which corresponds to apostrophe in US keyboard, X11
> generates XK_apostrophe, and we map it to VKEY_OEM_7.
> Of course it works well in a case that a keyboard layout has
XK_ISO_Level3_Latch
> and XK_apostrophe keys separately.
> 
> It's quite similar to other dead keys, and it's not surprising to send
> VKEY_ALTGR for dead keys, I think.

VKEY_ALTGR is never sent as a dead key; it's a value used to distinguish the
AltGr modifier use of right-Alt from normal Alt.

What does Windows produce when you press the Latvian "apostrophe variant" key?

[Yuki, 2014-06-13 07:09:48]

On 2014/06/13 04:45:42, Wez wrote:
> On 2014/06/13 03:42:47, Yuki wrote:
> > On 2014/06/12 19:07:34, Wez wrote:
> > > On 2014/06/12 02:47:15, Yuki wrote:
> > > > On 2014/06/11 22:01:03, Wez wrote:
> > > > > If XK_ISO_Level3_Latch is used for an apostrophe character on Latvian
> > > layouts,
> > > > > why do we want to treat it as equivalent to XK_ISO_Level3_Shift?
> > > > 
> > > > The equivalency doesn't matter much.
> > > > 
> > > > WindowTargeter::FindTargetForKeyEvent discards VKEY_UNKNOWN key events. 
> So,
> > > we
> > > > need to
> > > > 1. assign non-zero virtual key code to XK_ISO_Level3_Shift, or
> > > > 2. allow VKEY_UNKNOWN key events to pass through all event targets,
> > including
> > > > renderers.
> > > > 
> > > > And some people think option 2 is a little risky because it's been long
> time
> > > > since we stopped sending VKEY_UNKNOWN key events.  So I chose option 1.
> > > > 
> > > > I thought VKEY_ALTGR is most appropriate.
> > > 
> > > Why would we want to generate AltGr, if the key that generates this code
is
> > > actually a form of apostrophe? How will an app distinguish that apostrophe
> > press
> > > from a real AltGr press in that case?
> > 
> > This patch generates VKEY_ALTGR only for XK_ISO_Level3_Latch, not for
> > apostrophe.
> > 
> > XK_apostrophe is mapped to VKEY_OEM_7 (=222), and we send keydown event with
> > keycode=VKEY_OEM_7 and keypress event with keycode=39 (apostrophe)
regardless
> of
> > this patch.
> > 
> > Talking about a specific case to Latvian keyboard layout (apostrophe
variant),
> > when you hit once the key which corresponds to apostrophe in US keyboard,
X11
> > generates XK_ISO_Level3_Latch as a dead key, and we map it to VKEY_ALTGR.
> > When you hit twice the key which corresponds to apostrophe in US keyboard,
X11
> > generates XK_apostrophe, and we map it to VKEY_OEM_7.
> > Of course it works well in a case that a keyboard layout has
> XK_ISO_Level3_Latch
> > and XK_apostrophe keys separately.
> > 
> > It's quite similar to other dead keys, and it's not surprising to send
> > VKEY_ALTGR for dead keys, I think.
> 
> VKEY_ALTGR is never sent as a dead key; it's a value used to distinguish the
> AltGr modifier use of right-Alt from normal Alt.
> 
> What does Windows produce when you press the Latvian "apostrophe variant" key?

It seems there is no exactly same keyboard layout on Windows, so I used
"Latvian" keyboard layout, which seems the closest one.

IE on Win produces keycode=222
Firefox on Win produces keycode=0

IMHO, virtual keycode should represent a role unlike hardware keycode.  So
IE/Win seems not good because 222 (VKEY_OEM_7) is just a hardware keycode
regardless of its role.  Firefox seems reasonably correct.

What virtual keycode do you think we should assign to XK_ISO_Level3_Latch?


By the way, I was confused in the previous response #4.  Let me describe the
situation again.

In KeyEvent::GetCharacter(),
1. GetCharacterFromXEvent() returns zero because XK_ISO_Level3_Latch is not a
character. (correct)
2. We fallback to keycode, and will call GetCharacterFromKeyCode()

In KeyboardCodeFromXKeyEvent(),
3. XK_ISO_Level3_Latch defaults to VKEY_UNKNOWN (KeyboardCodeFromXKeysym)
4. Since it's VKEY_UNKNOWN, we fallback to hardware keycode
(DefaultXKeysymFromHardwareKeycode)
   In this case, hardware keycode is 48 and mapped to XK_apostrophe, and then
mapped to VKEY_OEM_7.

Back to KeyEvent::GetCharacter(),
5. Call GetCharacterFromKeyCode with VKEY_OEM_7 and get apostrophe character.
(wrong!)
6. An apostrophe character is inserted unexpectedly.

XK_ISO_Level3_Latch is not a character, so we shouldn't insert an apostrophe.

My idea is that, at step 3 and/or 4, we should return non-character virtual
keycode.


Back to the question to you, what virtual keycode do you think we should use?
a) It must be non-character virtual keycode.  (Or we need to stop falling back
to GetCharacterFromKeyCode.)
b) VKEY_UNKNOWN is blocked in WindowTargeter::FindTargetForKeyEvent, so if we'd
like to use VKEY_UNKNOWN, we need to change WindowTargeter.

[Wez, 2014-06-13 18:59:51]

On 13 June 2014 00:09, <yukishiino@chromium.org> wrote:

> On 2014/06/13 04:45:42, Wez wrote:
>
>> On 2014/06/13 03:42:47, Yuki wrote:
>> > On 2014/06/12 19:07:34, Wez wrote:
>> > > On 2014/06/12 02:47:15, Yuki wrote:
>> > > > On 2014/06/11 22:01:03, Wez wrote:
>> > > > > If XK_ISO_Level3_Latch is used for an apostrophe character on
>> Latvian
>> > > layouts,
>> > > > > why do we want to treat it as equivalent to XK_ISO_Level3_Shift?
>> > > >
>> > > > The equivalency doesn't matter much.
>> > > >
>> > > > WindowTargeter::FindTargetForKeyEvent discards VKEY_UNKNOWN key
>> events.
>> So,
>> > > we
>> > > > need to
>> > > > 1. assign non-zero virtual key code to XK_ISO_Level3_Shift, or
>> > > > 2. allow VKEY_UNKNOWN key events to pass through all event targets,
>> > including
>> > > > renderers.
>> > > >
>> > > > And some people think option 2 is a little risky because it's been
>> long
>> time
>> > > > since we stopped sending VKEY_UNKNOWN key events.  So I chose
>> option 1.
>> > > >
>> > > > I thought VKEY_ALTGR is most appropriate.
>> > >
>> > > Why would we want to generate AltGr, if the key that generates this
>> code
>>
> is
>
>> > > actually a form of apostrophe? How will an app distinguish that
>> apostrophe
>> > press
>> > > from a real AltGr press in that case?
>> >
>> > This patch generates VKEY_ALTGR only for XK_ISO_Level3_Latch, not for
>> > apostrophe.
>> >
>> > XK_apostrophe is mapped to VKEY_OEM_7 (=222), and we send keydown event
>> with
>> > keycode=VKEY_OEM_7 and keypress event with keycode=39 (apostrophe)
>>
> regardless
>
>> of
>> > this patch.
>> >
>> > Talking about a specific case to Latvian keyboard layout (apostrophe
>>
> variant),
>
>> > when you hit once the key which corresponds to apostrophe in US
>> keyboard,
>>
> X11
>
>> > generates XK_ISO_Level3_Latch as a dead key, and we map it to
>> VKEY_ALTGR.
>> > When you hit twice the key which corresponds to apostrophe in US
>> keyboard,
>>
> X11
>
>> > generates XK_apostrophe, and we map it to VKEY_OEM_7.
>> > Of course it works well in a case that a keyboard layout has
>> XK_ISO_Level3_Latch
>> > and XK_apostrophe keys separately.
>> >
>> > It's quite similar to other dead keys, and it's not surprising to send
>> > VKEY_ALTGR for dead keys, I think.
>>
>
>  VKEY_ALTGR is never sent as a dead key; it's a value used to distinguish
>> the
>> AltGr modifier use of right-Alt from normal Alt.
>>
>
>  What does Windows produce when you press the Latvian "apostrophe variant"
>> key?
>>
>
> It seems there is no exactly same keyboard layout on Windows, so I used
> "Latvian" keyboard layout, which seems the closest one.
>
> IE on Win produces keycode=222
> Firefox on Win produces keycode=0
>

What does Chrome generate for the key under Windows? Where possible, the
aim is to be consistent across the platforms Chrome runs on. My guess is
that Chrome will also generate VK_OEM_7, in which case it's the only value
that is in any sense "correct" to generate in this situation.

Some more questions...
1. Does XK_ISO_Level3_Latch appear on other standard layouts, or is Latvian
the only one we're aware of?
2. What effect does it have if you press it, then press some other key? Is
it like pressing the other key with ISO_Level3_Shift pressed?
3. What happens if you press and hold ISO_Level3_Shift and then press the
Latvian apostrophe variant key?

From what you've described, it sounds like this key may be a special case
that is going to be tricky to crowbar into Chrome/Blink's input handling.

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

[Yuki, 2014-06-14 08:58:58]

+sadrul, I'd like to hear your opinions, too, in addition to wez.

Thanks for questions and discussions.  They made me consider this problem more
carefully.  Now I recognize that the root cause should be the fallback mechanism
in KeyEvent::GetCharacter() method.

If GetCharacterFromXEvent returns zero, we should respect it and returns zero as
a resulting character.

I changed the approach and updated the code.
Could you take yet another look?

[Wez, 2014-06-16 21:10:39]

On 2014/06/14 08:58:58, Yuki wrote:
> +sadrul, I'd like to hear your opinions, too, in addition to wez.
> 
> Thanks for questions and discussions.  They made me consider this problem more
> carefully.  Now I recognize that the root cause should be the fallback
mechanism
> in KeyEvent::GetCharacter() method.
> 
> If GetCharacterFromXEvent returns zero, we should respect it and returns zero
as
> a resulting character.
> 
> I changed the approach and updated the code.
> Could you take yet another look?

LGTM, provided that the GetCharacterFromKeyCode behaviour is well-defined.

You're no longer mapping XK_ISO_Level3_Latch to anything - I'm guessing that
will mean it continues to map to VKEY_UNKNOWN rather than VKEY_OEM_7?

[Yuki, 2014-06-17 05:12:00]

On 2014/06/16 21:10:39, Wez wrote:
> On 2014/06/14 08:58:58, Yuki wrote:
> > +sadrul, I'd like to hear your opinions, too, in addition to wez.
> > 
> > Thanks for questions and discussions.  They made me consider this problem
more
> > carefully.  Now I recognize that the root cause should be the fallback
> mechanism
> > in KeyEvent::GetCharacter() method.
> > 
> > If GetCharacterFromXEvent returns zero, we should respect it and returns
zero
> as
> > a resulting character.
> > 
> > I changed the approach and updated the code.
> > Could you take yet another look?
> 
> LGTM, provided that the GetCharacterFromKeyCode behaviour is well-defined.
> 
> You're no longer mapping XK_ISO_Level3_Latch to anything - I'm guessing that
> will mean it continues to map to VKEY_UNKNOWN rather than VKEY_OEM_7?

It depends on which key is bound to XK_ISO_Level3_Latch.  If "apostrophe" key
(is US layout) was assigned to XK_ISO_Level3_Latch, the key is mapped to
VKEY_UNKNOWN first, and we fall back to the hardware keycode, and then we get
VKEY_OEM_7.  Depending on the hardware keycode, we may fall back to VKEY_UNKNOWN
or any other keycodes.

[Yuki, 2014-06-17 05:14:09]

sadrul@, could you review this CL as an owner?

[sadrul, 2014-06-17 17:17:06]

Can there be a test?

[Yuki, 2014-06-18 04:27:13]

On 2014/06/17 17:17:06, sadrul wrote:
> Can there be a test?

It's hard for this case.  In order to test this change, we'd like XLookupString
to return XK_ISO_Level3_Latch for example, and it requires to change X11
environment.  Since X11 environment is shared among all processes, I think it's
dangerous to change X11 env just for this test case.  If another process is
running in parallel, and that process depends on some part of X11, that process
would fail.  In addition, it's possible that the test process crashes before
recovering X11 env to the original state.

[Yuki, 2014-06-19 14:27:05]

On 2014/06/18 04:27:13, Yuki wrote:
> On 2014/06/17 17:17:06, sadrul wrote:
> > Can there be a test?
> 
> It's hard for this case.  In order to test this change, we'd like
XLookupString
> to return XK_ISO_Level3_Latch for example, and it requires to change X11
> environment.  Since X11 environment is shared among all processes, I think
it's
> dangerous to change X11 env just for this test case.  If another process is
> running in parallel, and that process depends on some part of X11, that
process
> would fail.  In addition, it's possible that the test process crashes before
> recovering X11 env to the original state.

Friendly ping?

[sadrul, 2014-06-19 14:29:39]

LGTM

[Yuki, 2014-06-19 14:35:25]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-06-19 14:36:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yukishiino@chromium.org/321153002/40001

[commit-bot: I haz the power, 2014-06-19 21:26:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-19 21:26:36]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[Yuki, 2014-06-20 05:59:47]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-06-20 06:00:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yukishiino@chromium.org/321153002/40001

[commit-bot: I haz the power, 2014-06-20 08:04:27]

Change committed as 278646