572385 - some special keys (e.g. IME related keys) can't be handled on Linux

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Kouhei Sutou]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; ja; rv:1.9.2.4) Gecko/20100505 Iceweasel/3.6.4 (like Firefox/3.6.4)
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.3a6pre) Gecko/20100615 Minefield/3.7a6pre

Some special keys, such as KANA, KANJI, CONVERT and NONCONVERT keys that are keys for changing Japanese IME mode, can't be handled on Linux (GTK+) port.

They can be handled on Windows port because they aren't needed to convert to NS_VK_* (Mozilla keycode) from VK_* (Windows native keycode). But they are needed to convert to NS_VK_* from GDK_* (GDK keycode) on GTK+ port.

We need to define GDK_* -> NS_VK_* keycode map on GTK+ port for handling those special keys.

Reproducible: Always

Steps to Reproduce:
1. listen keypress event
2. press KANJI key on Japanese 106 layout keyboard
3. show aEvent.keyCode in keypress event listener

Actual Results:  
aEvent.keyCode is 0.


Expected Results:  
aEvent.keyCode is 0x19 in 4. because 0x19 is returned on Windows.

Comment 1

[Kouhei Sutou]

Attachment: add key codes for special keys and map on GTK+

This patch adds some key values. Those key values are taken from Windows VK_* values. Because those key values already can be used on Windows. To use the same key values of them, we don't introduce any backward incompatibility on Windows. This patch just adds new key values support to GTK+.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

looks ok for me, but I'll review it tomorrow.

# And I guess that Mac has same bug.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> +  const unsigned long DOM_VK_KANA           = 0x15;
> +  const unsigned long DOM_VK_HANGUEL        = 0x15;
> +  const unsigned long DOM_VK_HANGUL         = 0x15;

> +#define NS_VK_KANA           nsIDOMKeyEvent::DOM_VK_KANA
> +#define NS_VK_HANGUEL        nsIDOMKeyEvent::DOM_VK_HANGUEL
> +#define NS_VK_HANGUL         nsIDOMKeyEvent::DOM_VK_HANGUL

> +    { NS_VK_KANA,       GDK_Kana_Lock },
> +    { NS_VK_KANA,       GDK_Kana_Shift },
> +    { NS_VK_HANGUL,     GDK_Hangul },

I think that we don't need DOM_VK_HANGUEL because it seems that defined for compatibility with old SDK.

And VK_KANA and VK_HANGUEL have same keycode on Windows but Linux isn't so. I think that the better way is:
const unsinged long DOM_VK_KANA_OR_HANGUL = 0x15;
const unsigned long DOM_VK_KANA = ...;
const unsigned long DOM_VK_KANA_SHIFT = ...;
const unsigned long DOM_VK_HANGUL = ...;

Smaug, do you have objections?

> +    { NS_VK_HANJA,      GDK_Hangul_Hanja },
> +    { NS_VK_KANJI,      GDK_Kanji },

same.

> +    // { NS_VK_JUNJA,      GDK_XXX },
> +    // { NS_VK_FINAL,      GDK_XXX },

See
http://nmhc.mohw.go.kr/en/html/service/service_pop_02.jsp

Junja means "Double-byte Latin letters". I'm not sure the final. We need help by Korean people...

Gen-san, do you know some good people?

> +    { NS_VK_LWIN,       GDK_Meta_L },
> +    { NS_VK_RWIN,       GDK_Meta_R },

Why is this needed? I guess you add this for DOMKeyCodeToGdkKeyCode(). DOMKeyCodeToGdkKeyCode() is called by nsNativeKeyBindings::KeyPress(). The aEvent should be initialized by a native key event which was dispatched by nsWindow. So, NS_VK_*WIN shouldn't come here.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #3)
> And VK_KANA and VK_HANGUEL have same keycode on Windows but Linux isn't so. I
> think that the better way is:
> const unsinged long DOM_VK_KANA_OR_HANGUL = 0x15;
> const unsigned long DOM_VK_KANA = ...;
> const unsigned long DOM_VK_KANA_SHIFT = ...;
> const unsigned long DOM_VK_HANGUL = ...;
> 
> Smaug, do you have objections?

Ah, sorry, DOM_VK_KANA and DOM_VK_HANGUL should be another name in this case for compatibility with Fx3.6 and earlier.

Comment 5

[Kouhei Sutou]

(In reply to comment #3)
> > +  const unsigned long DOM_VK_KANA           = 0x15;
> > +  const unsigned long DOM_VK_HANGUEL        = 0x15;
> > +  const unsigned long DOM_VK_HANGUL         = 0x15;
> 
> > +#define NS_VK_KANA           nsIDOMKeyEvent::DOM_VK_KANA
> > +#define NS_VK_HANGUEL        nsIDOMKeyEvent::DOM_VK_HANGUEL
> > +#define NS_VK_HANGUL         nsIDOMKeyEvent::DOM_VK_HANGUL
> 
> > +    { NS_VK_KANA,       GDK_Kana_Lock },
> > +    { NS_VK_KANA,       GDK_Kana_Shift },
> > +    { NS_VK_HANGUL,     GDK_Hangul },
> 
> I think that we don't need DOM_VK_HANGUEL because it seems that defined for
> compatibility with old SDK.

I'll remove it in the v2 patch.

> > +    // { NS_VK_JUNJA,      GDK_XXX },
> > +    // { NS_VK_FINAL,      GDK_XXX },
> 
> See
> http://nmhc.mohw.go.kr/en/html/service/service_pop_02.jsp
>
> Junja means "Double-byte Latin letters". I'm not sure the final. We need help
> by Korean people...

Thanks for the information. But it seems that GDK doesn't have any corresponding key code. Here are Hangle related key codes in GDK:

> #define GDK_Hangul 0xff31
> #define GDK_Hangul_Start 0xff32
> #define GDK_Hangul_End 0xff33
> #define GDK_Hangul_Hanja 0xff34
> #define GDK_Hangul_Jamo 0xff35
> #define GDK_Hangul_Romaja 0xff36
> #define GDK_Hangul_Codeinput 0xff37
> #define GDK_Hangul_Jeonja 0xff38
> #define GDK_Hangul_Banja 0xff39
> #define GDK_Hangul_PreHanja 0xff3a
> #define GDK_Hangul_PostHanja 0xff3b
> #define GDK_Hangul_SingleCandidate 0xff3c
> #define GDK_Hangul_MultipleCandidate 0xff3d
> #define GDK_Hangul_PreviousCandidate 0xff3e
> #define GDK_Hangul_Special 0xff3f
> #define GDK_Hangul_switch 0xff7e

It seems that meanings of those key codes are the same as
http://www.cs.columbia.edu/~lennox/draft-lennox-avt-app-sharing-00.html#virtual_hangul

In the v2 patch will not change the mappings... Sorry...

> > +    { NS_VK_LWIN,       GDK_Meta_L },
> > +    { NS_VK_RWIN,       GDK_Meta_R },
> 
> Why is this needed? I guess you add this for DOMKeyCodeToGdkKeyCode().
> DOMKeyCodeToGdkKeyCode() is called by nsNativeKeyBindings::KeyPress(). The
> aEvent should be initialized by a native key event which was dispatched by
> nsWindow. So, NS_VK_*WIN shouldn't come here.

nsKeycodes includes GDK_Shift_L, GDK_Shift_R and so on. So I think they are needed in nsKeycodes. They will be used in DOMKeyCodeToGdkKeyCode. If this is right, the change may be included in other patch. Because it breaks "a patch should include a thing" rule.

Should I create another bug for it?

Comment 6

[Kouhei Sutou]

Attachment: applied a comment #3.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> It seems that meanings of those key codes are the same as
> http://www.cs.columbia.edu/~lennox/draft-lennox-avt-app-sharing-00.html#virtual_hangul
> 
> In the v2 patch will not change the mappings... Sorry...

O.K. That is not important for now until some Korean people gives some hints to us.

>> > +    { NS_VK_LWIN,       GDK_Meta_L },
>> > +    { NS_VK_RWIN,       GDK_Meta_R },
>> 
>> Why is this needed? I guess you add this for DOMKeyCodeToGdkKeyCode().
>> DOMKeyCodeToGdkKeyCode() is called by nsNativeKeyBindings::KeyPress(). The
>> aEvent should be initialized by a native key event which was dispatched by
>> nsWindow. So, NS_VK_*WIN shouldn't come here.
> 
> nsKeycodes includes GDK_Shift_L, GDK_Shift_R and so on. So I think they are
> needed in nsKeycodes. They will be used in DOMKeyCodeToGdkKeyCode. If this is
> right, the change may be included in other patch.

I still think that you never send NS_VK_*WIN from gtk2/nsWindow.cpp. Therefore, they're not needed in nsKeycodes. In other words, DOMKeyCodeToGdkKeyCode() is never called with NS_VK_*WIN. Furthermore, GDK_Shift_L and GDK_Shift_R cases are needed for GdkKeyCodeToDOMKeyCode(), but GDK_Meta_L and GDK_Meta_R are already there.

# And I think that windows/nsWindow.cpp should convert NS_VK_*WIN to NS_VK_META.

And please request the review to me at next patch and later, and also please mark the old patches as obsolete, thanks.

Comment 8

[Kouhei Sutou]

Attachment: applied a feedback in the comment #7.

(In reply to comment #7)
> >> > +    { NS_VK_LWIN,       GDK_Meta_L },
> >> > +    { NS_VK_RWIN,       GDK_Meta_R },
> >> 
> >> Why is this needed? I guess you add this for DOMKeyCodeToGdkKeyCode().
> >> DOMKeyCodeToGdkKeyCode() is called by nsNativeKeyBindings::KeyPress(). The
> >> aEvent should be initialized by a native key event which was dispatched by
> >> nsWindow. So, NS_VK_*WIN shouldn't come here.
> > 
> > nsKeycodes includes GDK_Shift_L, GDK_Shift_R and so on. So I think they are
> > needed in nsKeycodes. They will be used in DOMKeyCodeToGdkKeyCode. If this is
> > right, the change may be included in other patch.
> 
> I still think that you never send NS_VK_*WIN from gtk2/nsWindow.cpp. Therefore,
> they're not needed in nsKeycodes. In other words, DOMKeyCodeToGdkKeyCode() is
> never called with NS_VK_*WIN. Furthermore, GDK_Shift_L and GDK_Shift_R cases
> are needed for GdkKeyCodeToDOMKeyCode(), but GDK_Meta_L and GDK_Meta_R are
> already there.

O.K. I understand. They are removed from the v3 patch.

> And please request the review to me at next patch and later, and also please
> mark the old patches as obsolete, thanks.

I'll do them in this submit.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 452532 [details] [diff] [review]
applied a feedback in the comment #7.

>diff --git a/dom/interfaces/events/nsIDOMKeyEvent.idl b/dom/interfaces/events/nsIDOMKeyEvent.idl
>--- a/dom/interfaces/events/nsIDOMKeyEvent.idl
>+++ b/dom/interfaces/events/nsIDOMKeyEvent.idl
>@@ -106,17 +119,20 @@ interface nsIDOMKeyEvent : nsIDOMUIEvent
>   const unsigned long DOM_VK_T              = 0x54;
>   const unsigned long DOM_VK_U              = 0x55;
>   const unsigned long DOM_VK_V              = 0x56;
>   const unsigned long DOM_VK_W              = 0x57;
>   const unsigned long DOM_VK_X              = 0x58;
>   const unsigned long DOM_VK_Y              = 0x59;
>   const unsigned long DOM_VK_Z              = 0x5A;
> 
>+  const unsigned long DOM_VK_LWIN           = 0x5B;
>+  const unsigned long DOM_VK_RWIN           = 0x5C;
>   const unsigned long DOM_VK_CONTEXT_MENU   = 0x5D;
>+  const unsigned long DOM_VK_SLEEP          = 0x5F;

Remove DOM_VK_LWIN and DOM_VK_RWIN.

>diff --git a/widget/public/nsGUIEvent.h b/widget/public/nsGUIEvent.h
>--- a/widget/public/nsGUIEvent.h
>+++ b/widget/public/nsGUIEvent.h
>@@ -1489,26 +1489,39 @@ enum nsDragDropEventStatus {
>+#define NS_VK_LWIN           nsIDOMKeyEvent::DOM_VK_LWIN
>+#define NS_VK_RWIN           nsIDOMKeyEvent::DOM_VK_RWIN

Same.

Otherwise, looks O.K.

Comment 10

[Kouhei Sutou]

Attachment: applied a feedback in the comment #9.

(In reply to comment #9)
> (From update of attachment 452532 [details] [diff] [review])
> >diff --git a/dom/interfaces/events/nsIDOMKeyEvent.idl b/dom/interfaces/events/nsIDOMKeyEvent.idl
> >--- a/dom/interfaces/events/nsIDOMKeyEvent.idl
> >+++ b/dom/interfaces/events/nsIDOMKeyEvent.idl
> >@@ -106,17 +119,20 @@ interface nsIDOMKeyEvent : nsIDOMUIEvent
> >   const unsigned long DOM_VK_T              = 0x54;
> >   const unsigned long DOM_VK_U              = 0x55;
> >   const unsigned long DOM_VK_V              = 0x56;
> >   const unsigned long DOM_VK_W              = 0x57;
> >   const unsigned long DOM_VK_X              = 0x58;
> >   const unsigned long DOM_VK_Y              = 0x59;
> >   const unsigned long DOM_VK_Z              = 0x5A;
> > 
> >+  const unsigned long DOM_VK_LWIN           = 0x5B;
> >+  const unsigned long DOM_VK_RWIN           = 0x5C;
> >   const unsigned long DOM_VK_CONTEXT_MENU   = 0x5D;
> >+  const unsigned long DOM_VK_SLEEP          = 0x5F;
> 
> Remove DOM_VK_LWIN and DOM_VK_RWIN.
> 
> >diff --git a/widget/public/nsGUIEvent.h b/widget/public/nsGUIEvent.h
> >--- a/widget/public/nsGUIEvent.h
> >+++ b/widget/public/nsGUIEvent.h
> >@@ -1489,26 +1489,39 @@ enum nsDragDropEventStatus {
> >+#define NS_VK_LWIN           nsIDOMKeyEvent::DOM_VK_LWIN
> >+#define NS_VK_RWIN           nsIDOMKeyEvent::DOM_VK_RWIN
> 
> Same.

In the new patch, they are removed.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 452734 [details] [diff] [review]
applied a feedback in the comment #9.

Looks good for me. Perhaps, smaug should sr the interface changes.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/2e74d29bd943

Thank you, Sutou-san.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> /builds/slave/mozilla-central-linux-debug/build/widget/src/gtk2/nsGtkKeyUtils.cpp:143: error: ‘GDK_Sleep’ was not declared in this scope
> NEXT ERROR make[6]: *** [nsGtkKeyUtils.o] Error 1
> make[6]: Leaving directory `/builds/slave/mozilla-central-linux-debug/build/obj-firefox/widget/src/gtk2'
> NEXT ERROR make[5]: *** [libs] Error 2
> make[5]: Leaving directory `/builds/slave/mozilla-central-linux-debug/build/obj-firefox/widget/src'
> NEXT ERROR make[4]: *** [libs] Error 2
> make[4]: Leaving directory `/builds/slave/mozilla-central-linux-debug/build/obj-firefox/widget'
> NEXT ERROR make[3]: *** [libs_tier_platform] Error 2
> make[3]: Leaving directory `/builds/slave/mozilla-central-linux-debug/build/obj-firefox'
> make[2]: *** [tier_platform] Error 2
> make[2]: Leaving directory `/builds/slave/mozilla-central-linux-debug/build/obj-firefox'
> make[1]: *** [default] Error 2
> make[1]: Leaving directory `/builds/slave/mozilla-central-linux-debug/build/obj-firefox'
> make: *** [build] Error 2
> program finished with exit code 2
> elapsedTime=386.005569
> === Output ended ===

backedout.

Comment 14

[Kouhei Sutou]

Attachment: check GDK_Sleep availability.

GTK+ 2.10 doesn't have GDK_Sleep.
I've added a #ifdef check for GDK_Sleep in the attached patch.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I pushed the new patch to tryserver, I'll review it after I confirm the result.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If we press sleep key before GTK+2.10 system, what event is dispatched? If the problem is just the definition, i.e., a key event is dispatched with key code 0x1008ff2f, I think we should define GDK_Sleep ourselves.

Comment 17

[Kouhei Sutou]

(In reply to comment #16)
> If we press sleep key before GTK+2.10 system, what event is dispatched? If the
> problem is just the definition, i.e., a key event is dispatched with key code
> 0x1008ff2f, I think we should define GDK_Sleep ourselves.

A sleep key is handled like the current implementation, i.e. it's just ignored. An event key code will be -1.

0x1008ff2f will not be changed because the value is derived from Xorg keysyms. So we can use 0x1008ff2f without breaking forward compatibility.


* Case1: not define fallback GDK_Sleep: A sleep key just ignored on old GTK+ platform. (the current behavior)
* Case2: define fallback GDK_Sleep: A sleep key works on all GTK+ platform without breaking forward compatibility.

It seems that both cases are reasonable. I agree with your suggestion (the case2) because GDK_Sleep had been appeared since GTK+ 2.18 and Ubuntu hardy (LTS) using GTK+ 2.12. It's good that Ubuntu hardy user can also use a sleep key.

Should I update a patch for the case2?

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Should I update a patch for the case2?

Yes, please do it.

Comment 19

[Kouhei Sutou]

Attachment: define fallback GDK_Sleep

(In reply to comment #18)
> > Should I update a patch for the case2?
> 
> Yes, please do it.

I've added a fallback GDK_Sleep definition in the attached patch.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/007ceb11e614

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> I still think that you never send NS_VK_*WIN from gtk2/nsWindow.cpp. Therefore,
> they're not needed in nsKeycodes. In other words, DOMKeyCodeToGdkKeyCode() is
> never called with NS_VK_*WIN. Furthermore, GDK_Shift_L and GDK_Shift_R cases
> are needed for GdkKeyCodeToDOMKeyCode(), but GDK_Meta_L and GDK_Meta_R are
> already there.
> 
> # And I think that windows/nsWindow.cpp should convert NS_VK_*WIN to
> NS_VK_META.

Hmm, both IE and Chrome fire the key events with the raw keycode (VK_*WIN). So, we shouldn't touch to it for compatibility with other browsers.

Comment 22

[andré]

I have what seems to be the same problem, on Linux 32-bit with Gnome, GTK+ 2.18.9,
using the Canadian French keyboard.
Special caracters generated using right-Alt (the level-3 key) do not work with Seamonkey 2.0.5.
However they work all my other (non-Mozilla) applications, and can be inserted using cut/copy.
It also worked with Seamonkey 2.0.0.  It stopped working recently, I'm not sure with what version.

I note that this bug is marked resolved fixed.  Is it correct to assume that the fix will be included with the next update of Seamonkey ?

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This is not text inputting bug. This is the DOM keyboard event compatibility issue between platforms.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

documented in MDC
https://developer.mozilla.org/en/DOM/Event/UIEvent/KeyEvent

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #2)
> # And I guess that Mac has same bug.

I filed bug 603697.