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

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: kou, Assigned: kou)

Tracking

({dev-doc-complete, inputmethod})

Trunk
x86_64
Linux
dev-doc-complete, inputmethod
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
Created attachment 451553 [details] [diff] [review]
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+.
Attachment #451553 - Flags: review?
(Assignee)

Updated

8 years ago
Summary: some special keys (e.g. IME related keys) can't be handled on Lilnux → some special keys (e.g. IME related keys) can't be handled on Linux
looks ok for me, but I'll review it tomorrow.

# And I guess that Mac has same bug.
> +  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.
Assignee: nobody → kou
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Version: unspecified → Trunk
(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.
Keywords: inputmethod
(Assignee)

Comment 5

8 years ago
(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?
(Assignee)

Comment 6

8 years ago
Created attachment 452523 [details] [diff] [review]
applied a comment #3.
> 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.
(Assignee)

Comment 8

8 years ago
Created attachment 452532 [details] [diff] [review]
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.
Attachment #451553 - Attachment is obsolete: true
Attachment #452523 - Attachment is obsolete: true
Attachment #452532 - Flags: review?(masayuki)
Attachment #451553 - Flags: review?
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.
(Assignee)

Comment 10

8 years ago
Created attachment 452734 [details] [diff] [review]
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.
Attachment #452532 - Attachment is obsolete: true
Attachment #452734 - Flags: review?(masayuki)
Attachment #452532 - Flags: review?(masayuki)
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.
Attachment #452734 - Flags: superreview?(Olli.Pettay)
Attachment #452734 - Flags: review?(masayuki)
Attachment #452734 - Flags: review+

Updated

8 years ago
Attachment #452734 - Flags: superreview?(Olli.Pettay) → superreview+
http://hg.mozilla.org/mozilla-central/rev/2e74d29bd943

Thank you, Sutou-san.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
> /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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

8 years ago
Created attachment 455070 [details] [diff] [review]
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.
Attachment #452734 - Attachment is obsolete: true
Attachment #455070 - Flags: review?(masayuki)
I pushed the new patch to tryserver, I'll review it after I confirm the result.
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.
(Assignee)

Comment 17

8 years ago
(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?
> Should I update a patch for the case2?

Yes, please do it.
(Assignee)

Comment 19

8 years ago
Created attachment 455302 [details] [diff] [review]
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.
Attachment #455070 - Attachment is obsolete: true
Attachment #455070 - Flags: review?(masayuki)
(Assignee)

Updated

8 years ago
Attachment #455302 - Flags: review?(masayuki)
Attachment #455302 - Flags: review?(masayuki) → review+
http://hg.mozilla.org/mozilla-central/rev/007ceb11e614
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
> 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

8 years ago
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 ?
This is not text inputting bug. This is the DOM keyboard event compatibility issue between platforms.
(In reply to comment #2)
> # And I guess that Mac has same bug.

I filed bug 603697.
You need to log in before you can comment on or make changes to this bug.