Last Comment Bug 447757 - Up to half the keyCode assignments in keyup/keydown events are missing or wrong.
: Up to half the keyCode assignments in keyup/keydown events are missing or wrong.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: Trunk
: x86 Linux
: -- major with 1 vote (vote)
: mozilla15
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
:
:
Mentors:
data:text/html,<p id="p"></p><script>...
: 583721 (view as bug list)
Depends on: 784246
Blocks: 479942 631165
  Show dependency treegraph
 
Reported: 2008-07-24 02:13 PDT by Allan Jacobs
Modified: 2012-08-20 20:37 PDT (History)
11 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Sample HTML code (4.82 KB, text/html)
2008-07-24 02:16 PDT, Allan Jacobs
no flags Details
HTML example appropriate for year 2010 (9.30 KB, text/html)
2010-09-10 16:36 PDT, Allan Jacobs
no flags Details
Patch v1.0 (3.84 KB, patch)
2010-11-10 22:24 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch v1.0.1 (5.45 KB, patch)
2010-11-10 22:31 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
masayuki: review-
Details | Diff | Splinter Review
Patch v1.0.2 (6.73 KB, patch)
2010-11-10 23:32 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
karlt: review+
Details | Diff | Splinter Review
Patch v1.0.3 (7.03 KB, patch)
2010-11-17 04:25 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch v2.0 (19.49 KB, patch)
2011-02-08 21:23 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch v2.1 (21.49 KB, patch)
2011-02-09 20:41 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch v2.2 (22.06 KB, patch)
2011-02-10 20:56 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
part.1 Compute DOM keycode for non-printable key from table first (10.38 KB, patch)
2012-03-14 20:33 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
part.2 DOM keycode for printable keys in Numpad should be computed by switch (5.03 KB, patch)
2012-03-14 20:35 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
part.3 Use unshifted char of printable keys for computing keycode on GTK (16.27 KB, patch)
2012-03-14 21:52 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
part.4 Clean up modifier keycode mapping on GTK (4.45 KB, patch)
2012-03-14 21:56 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
part.1 Compute DOM keycode for non-printable key from table first (10.37 KB, patch)
2012-05-07 20:26 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
karlt: review-
karlt: feedback+
Details | Diff | Splinter Review
part.2 DOM keycode for printable keys in Numpad should be computed by switch (5.03 KB, patch)
2012-05-07 20:27 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
karlt: review+
Details | Diff | Splinter Review
part.3 Use unshifted char of printable keys for computing keycode on GTK (16.27 KB, patch)
2012-05-07 20:28 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
karlt: review+
Details | Diff | Splinter Review
part.4 Clean up modifier keycode mapping on GTK (4.45 KB, patch)
2012-05-07 20:28 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
karlt: review+
Details | Diff | Splinter Review
part.1 Compute DOM keycode for non-printable key from table first (10.24 KB, patch)
2012-05-15 06:21 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
karlt: review-
Details | Diff | Splinter Review
part.3 Use unshifted char of printable keys for computing keycode on GTK (15.56 KB, patch)
2012-05-15 06:25 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
part.4 Clean up modifier keycode mapping on GTK (4.46 KB, patch)
2012-05-15 06:26 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
bugs: superreview+
Details | Diff | Splinter Review
part.1 Compute DOM keycode for non-printable key from table first (10.50 KB, patch)
2012-05-16 19:33 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
karlt: review+
Details | Diff | Splinter Review
part.2 DOM keycode for printable keys in Numpad should be computed by switch (5.07 KB, patch)
2012-05-16 19:34 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
part.3 Use unshifted char of printable keys for computing keycode on GTK (15.59 KB, patch)
2012-05-16 19:34 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review

Description Allan Jacobs 2008-07-24 02:13:39 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008061015 Firefox/3.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008061015 Firefox/3.0

Keyup/keydown events should possess a keyCode attribute that encodes the key that was pressed.  On Linux, on non-US layouts, many of the assignments to this attribute are incorrect.



Reproducible: Always

Steps to Reproduce:
1. Make Albania/Default the default layout.
2. Start up a browser.
3. Run the attached code.
4. Hit the Separate button.
5. Type the key that would be 'E' on a US keyboard.
6. Hold the AltGr key down and type the 'E' key again.  A Euro symbol will appear in the text field.  This is character U+20ac.  The keyCode is 0 and should be 69.

Actual Results:  
Keycodes in AltGr shift state and Shift+AltGr shift state usually get assigned 0 as a keyCode.  Those keys that are assigned a non-zero value are assigned the wrong non-zero value.

Expected Results:  
Column 1 is the observed keycode.  Column 2 is the Unicode for the character grabbed from a keypress event (for dead keys this is assigned using domain knowledge).  Column 3 is the key that appears in the text field.  Column 4, when present, is a comment describing the problem.

Albanian Default Normal State
220	U+005c	\
49	U+0031	1
50	U+0032	2
51	U+0033	3
52	U+0034	4
53	U+0035	5
54	U+0036	6
55	U+0037	7
56	U+0038	8
57	U+0039	9
48	U+0030	0
109	U+002d	-
61	U+003d	=
81	U+0071	q
87	U+0077	w
69	U+0065	e
82	U+0072	r
84	U+0074	t
90	U+007a	z
85	U+0075	u
73	U+0069	i
79	U+006f	o
80	U+0070	p
0	U+00e7	ç	(bug: keyCode should be ?)	
50	U+0040	@
65	U+0061	a
83	U+0073	s
68	U+0064	d
70	U+0066	f
71	U+0067	g
72	U+0068	h
74	U+006a	j
75	U+006b	k
76	U+006c	l
0	U+00eb	ë	(bug: keyCode should be ?)	
219	U+005b	[
221	U+005d	]
188	U+003c	<	(bug: keyCode 188 is assigned twice)
89	U+0079	y	
88	U+0078	x	
68	U+0064	d	
86	U+0076	v	
66	U+0062	b	
78	U+006e	n	
77	U+006d	m	
188	U+002c	,	(bug: keyCode 188 is assigned twice)
190	U+002e	.	
191	U+002f	/

Albanian Default Shift State
220	U+007c	|	
49	U+0021	!	
222	U+0022	"	(bug: keyCode should be 50)
51	U+0023	#	
52	U+0024	$	
53	U+0025	%	
54	U+005e	^	
55	U+0026	&	
56	U+002a	*	
57	U+0028	(	
48	U+0029	)	
109	U+005f	_	
61	U+002b	+	
81	U+0051	Q	
87	U+0057	W	
69	U+0045	E	
82	U+0052	R	
84	U+0054	T	
90	U+005a	Z	
85	U+0055	U	
73	U+0049	I	
79	U+004f	O	
80	U+0050	P	
0	U+00c7	Ç	
222	U+0027	'	
65	U+0041	A	
83	U+0053	S	
68	U+0044	D	
70	U+0046	F	
71	U+0047	G	
72	U+0048	H	
74	U+004a	J	
75	U+004b	K
76	U+004c	L	
0	U+00cb	Ë	(bug: keyCode should be ?)
219	U+007b	{	
221	U+007d	}	
190	U+003e	>	(bug: keyCode is 188 in Normal state)
89	U+0059	Y	
88	U+0058	X	
67	U+0043	C	
86	U+0056	V	
66	U+0042	B	
78	U+004e	N	
77	U+004d	M	
59	U+003b	;	(bug: keyCode 59 is assigned twice)
59	U+003a	:	(bug: keyCode 59 is assigned twice)
191	U+003f	?		

Albanian Default AltGr State
0	U+00ac	¬	(bug: keyCode should be 220)
192	U+007e	~	(bug: should be U+005f.  Keycode should be 49)
N/A	U+02c7	ˇ	(bug: no event generated. Dead key behavior flawed.)
N/A	U+005e	^	(bug: no event generated. Dead key behavior flawed.)
N/A	U+02d8	˘	(bug: no event generated. Dead key behavior flawed.)
N/A	U+00b0	˚	(bug: no event generated. Dead key behavior flawed.)
N/A	U+02db	˛	(bug: no event generated. Dead key behavior flawed.)
192	U+0060	`	(bug: keyCode should be 55)
N/A	U+02d9	˙	(bug: no event generated. Dead key behavior flawed.)
N/A	U+00b4	´	(bug: no event generated. Dead key behavior flawed.)
N/A	U+02dd	˝	(bug: no event generated. Dead key behavior flawed.)
N/A	U+00a8	¨	(bug: no event generated. Dead key behavior flawed.)
N/A	U+00b8	¸	(bug: no event generated. Dead key behavior flawed.)
220	U+005c	\	(bug: keyCode should be 81)
220	U+007c	|	(bug: keyCode should be 87)
0	U+20ac	€	(bug: keyCode should be 69)
0	U+00b6	¶	(bug: keyCode should be 82)
0	U+0167	ŧ	(bug: keyCode should be 84)
0	U+2190	←	(bug: keyCode should be 90)
0	U+2193	↓	(bug: keyCode should be 85)
0	U+2192	→	(bug: keyCode should be 73)
0	U+00f8	ø	(bug: keyCode should be 79)	
0	U+00fe	þ	(bug: keyCode should be 80)
0	U+00f7	÷	(bug: keyCode not assigned)
0	U+00d7	×	(bug: keyCode not assigned)
0	U+00e6	æ	(bug: keyCode should be 65)
0	U+0111	đ	(bug: keyCode should be 83)
0	U+0110	Đ	(bug: keyCode should be 68)
219	U+005b	[	(bug: keyCode should be 71)
221	U+005d	]	(bug: keyCode should be 72)
0	U+0127	ħ	(bug: keyCode should be 74)
0	U+0142	ł	(bug: keyCode should be 75)
0	U+0142	ł	(bug: keyCode should be 76)
52	U+0024	$	(bug: keyCode should be ?)
0	U+00df	ß	(bug: keyCode should be 219)
0	U+00a4	¤	(bug: keyCode should be ?)
0	U+007c	|	(bug: keyCode should be 190)
0	U+00ab	«	(bug: keyCode should be 89)
0	U+00bb	»	(bug: keyCode should be 88)
0	U+00a2	¢	(bug: keyCode should be 67)
50	U+0040	@	(bug: keyCode should be 86)
219	U+007b	{	(bug: keyCode should be 66)
221	U+007d	}	(bug: keyCode should be 78)
0	U+00a7	§	(bug: keyCode should be ?)
188	U+003c	<	(bug: keyCode should be ?)
190	U+003e	>	(bug: keyCode should be ?)


Albanian Default Shift+AltGr State
0	U+00ac	¬	(bug: keyCode is 0)
0	U+007e	~	(bug: no event generated. Dead key behavior flawed.)
0	U+215b	⅛	(bug: keyCode should be 50)
0	U+00a3	£	(bug: keyCode should be 51)
52	U+0054	$
0	U+215c	⅜	(bug: keyCode should be 53)
0	U+215d	⅝	(bug: keyCode should be 54)
N/A	U+0000	`	(bug: no event generated. Dead key behavior flawed.)
0	U+2122	™	(bug: keyCode should be 56)
0	U+00b1	±	(bug: keyCode should be 57)
0	U+00b0	°	(bug: keyCode should be 48)
0	U+00bf	¿	(bug: keyCode should be 109)
N/A	U+0000	˛	(bug: no event generated. Dead key behavior flawed.)
0	U+03a9	Ω	(bug: keyCode should be 81)
0	U+0141	Ł	(bug: keyCode should be 87)
0	U+20ac	€	(bug: keyCode should be 69)
0	U+00ae	®	(bug: keyCode should be 82)
0	U+0166	Ŧ	(bug: keyCode should be 84)
0	U+00a5	¥	(bug: keyCode should be 90)
0	U+2191	↑	(bug: keyCode should be 85)
0	U+0131	ı	(bug: keyCode should be 73)
0	U+00d8	Ø	(bug: keyCode should be 79)
0	U+00de	Þ	(bug: keyCode should be 80)
0	?	˚	(bug: no event generated. Dead key behavior flawed.)
0	?	¯	(bug: no event generated. Dead key behavior flawed.)
0	U+00c6	Æ	(bug: keyCode should be 65)
0	U+00a7	§	(bug: keyCode should be 83)
0	U+00d0	Ð	(bug: keyCode should be 68)
0	U+00aa	ª	(bug: keyCode should be 71)
0	U+014a	Ŋ	(bug: keyCode should be 72)
0	U+0126	Ħ	(bug: keyCode should be 74)
55	U+0026	&	(bug: keyCode should be 75)
0	U+0141	Ł	(bug: keyCode should be 76)
N/A	?	˝	(bug: no event generated. Dead key behavior flawed.)
N/A	?	˘	(bug: no event generated. Dead key behavior flawed.)
0	U+00a6	¦	(bug: keyCode should be 190)
188	U+003c	<	(bug: keyCode should be 89)
190	U+003e	>	(bug: keyCode should be 88)
0	U+00a9	©	(bug: keyCode should be 67)
192	U+0060	`	(bug: keyCode should be 86)
222	U+0027	'	(bug: keyCode should be 66)
221	U+007d	}	(bug: keyCode should be 78)
0	U+00ba	º	(bug: keyCode should be ?)
0	U+00d7	×	(bug: keyCode should be ?)
0	U+00f7	÷	(bug: keyCode should be ?)
N/A	?	˙	(bug: no event generated.  Dead key behavior flawed.)


Test code will be attached.

Albania/Default was chosen because it's first in keyboard layout when these are put into alphabetic order.  The same problem occurs on almost all Linux layouts.

The keys that do not generate either keyboard events or text events are dead keys.  This problem has been reported in bugs 101279 and 308820.
Comment 1 Allan Jacobs 2008-07-24 02:16:05 PDT
Created attachment 331066 [details]
Sample HTML code
Comment 2 Allan Jacobs 2008-07-26 00:50:01 PDT
Traceable to widget/src/gtk2/nsGtkKeyUtils.cpp.  The methods GdkKeyCodeToDOMKeyCode(int aKeysym) and DOMKeyCodeToGdkKeyCode(int aKeysym) assume a US layout.
(1) The methods are set up to handle function keys, some control characters, the numeric keypad, digits, letters a to z, letters A to Z, and the punctuation characters `~!@#$%^&*()_+-=[];'\,./<>?.  There are a lot more characters to handle than this.
(2) The two methods assume two shift states.  Many layouts require four shift states (Normal, Shift, AltGr, and Shift+AltGr).

The method GetFlagWord32 in widget/src/gtk2/nsWindow.h asserts that any keyCode assignment should be between 1 and 255, inclusive.

The call to DOMKeyCodeToGdkKeyCode in nsNativeKeyBindings.cpp is probably ok.  It is called for keypress events where charCode is not assigned (probably for arrow keys).  The code just below the call (in the KeyPress method) assumes two shift states again -- probably a problem.

The call to GdkKeyCodeToDOMKeyCode in nsWindow.cpp is a problem. GdkKeyCodeToDOMKeyCode returns 0 to it's caller (OnKeyPress).  The 0 is tested by the utility IsKeyDown and returns true to often, suppressing the keyDown event.  This may be the root cause of the dead key problem mentioned in bug 308820.

There is another call to GdkKeyCodeToDOMKeyCode in InitKeyEvent in the file widget/src/gtk2/nsCommonWidget.cpp.  This is the root cause of the bug.  The return from the call to GdkKeyCodeToDOMKeyCode is assigned to the keyCode attribute of an event object.  But, this return value is 0 for many characters.

On other operating systems, the equivalent of GdkKeyCodeToDOMKeyCode would have a signature that included the layout and maybe the shift state.



Comment 3 Allan Jacobs 2008-08-02 02:24:08 PDT
There are two calls to GdkKeyCodeToDOMKeyCode in the source code.  If either of these returns a keycode of 0 then there will be a bad KeyboardEvent generated -- one with keyCode=0.  If the following method is called to assign the keyCode when the GdkKeyCodeToDOMKeyCode fails in this way then most of the keyCode assignment problems are corrected.  The assignments are backwards compatible with previous versions of Mozilla.
int
GdkKeyCodeToDOMKeyCodeExtended(GdkEventKey *event)
{
    int aKeysym, i, j, length = 0, klength = 0;
    gint n_entries = 0;
    guint *keyvals = NULL;
    GdkKeymapKey *keys = NULL;
    GdkKeymap *keymap = gdk_keymap_get_default();
    gdk_keymap_get_entries_for_keycode(keymap, event->hardware_keycode,
        &keys, &keyvals, &n_entries);

    // US keyboard characters, numbers, misc other things
    // in other shift states take precedence over the keyvals
    // in the upper case shift state.
    length = sizeof(nsKeycodes) / sizeof(struct nsKeyConverter);
    klength = sizeof(keyvals);
    if (klength > 4) klength = 4;
    for (j = 0; j < klength; j++) {
        aKeysym = keyvals[j];
        for (i = 0; i < length; i++) {
            if (aKeysym >= GDK_a && aKeysym <= GDK_z)
                return aKeysym - GDK_a + NS_VK_A;
            if (aKeysym >= GDK_A && aKeysym <= GDK_Z)
                return aKeysym - GDK_A + NS_VK_A;
            if (aKeysym >= GDK_0 && aKeysym <= GDK_9)
                return aKeysym - GDK_0 + NS_VK_0;
            if (nsKeycodes[i].keysym == aKeysym) {
                return(nsKeycodes[i].vkCode);
            }
        }
    }
    return keyvals[1];
}

There are still two types of keys that are problematic even with this fix applied.  One set is a subset of the control keys that definitely includes the AltGr key.  The second set are the dead keys.  GTK+ events for keys in these two classes are well outside the 0 to 255 range assumed by Mozilla source code.
Comment 4 Allan Jacobs 2008-08-08 19:06:30 PDT
The algorithm in the previous comment assigns keycodes in a manner that is backwards compatible on US layout keyboards.

A problem arises if an attempt is made to assign Netscape-specific constants to the numeric and punctuation characters.  This simply cannot be done in a layout-independent way.

The existing Netscape keycode assignments bind (as they should in US layout) the same keycode to 2 and @.  Also, to ' and ".  I intended to examine the standard Linux layouts one by one in alphabetic order.  The first is Albania.  There is no need to look further.  In the Albania layout, 2 and " must be bound to the same keycode.  Also, @ and '.

This is going to take some discussion with experienced Mozilla hands to untangle.  Any solution that is backwards-compatible for US keyboard layout necessarily assigns keycodes to punctuation characters differently on the other layouts.
Comment 5 Allan Jacobs 2008-08-09 23:54:59 PDT
"int GdkKeyCodeToDOMKeyCodeExtended(GdkEventKey *event)" is a reasonable start.  The solution, as discussed in the previous comment, is not good enough.

#1.  Current code
=================
GdkKeyCodeToDOMKeyCode is bad enough to get completely replaced, not just supplemented.

#2.  Capitals in Shift state
============================
If a Latin or non-Latin, non-ASCII capital letters is found in Shift state, then it should be used to set the keycode -- just like ASCII capital letters do in the current implementation.  This is tricky, in that the capitals are not in a contiguous block in Unicode space.

There are five exceptions.  In GDK-speak, they are GDK_Agrave, GDK_Ucircumflex, GDK_Yacute, GDK_THORN, and GDK_Udiaeresis


#3. Netscape keycodes
=====================
The keys that are getting mapped to Netscape-values instead of
using native keyvalues to assign keycodes are

        keyvals hw_kc   Netscape_kc
`/~     96/126  49      192     0xc0
1/!     49/33   10      49
2/@     50/64   11      50
3/#     51/35   12      51
4/$     52/36   13      52
5/%     53/37   14      53
6/^     54/94   15      54
7/&     55/38   16      55
8/*     56/39   17      56
9/(     57/40   18      57
0/)     48/19   19      48
-/_     45/95   20      109     0x6d
=/+     61/43   21      61
[/{     91/123  34      219     0xdb
]/}     93/125  35      221     0xdd
;/:     59/58   47      59
'/"     39/34   48      222     0xde
\/|     92/124  51      220     0xdc
,/<     44/60   59      188     0xbc
./>     46/62   60      190     0xbe
//?     47/63   61      191     0xbf

Suppose numerics take precedence in the assignment of a keycode,
overriding characters associated with other key states for a
shared key.  Suppose punctuation on non-numeric keys is used to
assign keycode from the Normal state (not Shift and not AltGr
state).  Then the Netscape keycode is a GDK key value with the
exceptions

        keyvals hw_kc   Netscape_kc
-/_     45/95   20      109     0x6d
[/{     91/123  34      219     0xdb
]/}     93/125  35      221     0xdd
'/"     39/34   48      222     0xde
\/|     92/124  51      220     0xdc
,/<     44/60   59      188     0xbc
./>     46/62   60      190     0xbe
//?     47/63   61      191     0xbf

#4. Exceptional keys
====================
Read #3 above.  Now, we turn the problem around.  What characters from gdk/gdkkeysyms.h have these keyvalues?  The conflicts are with

0x0c0 GDK_Agrave
0x06d GDK_m
0x0db GDK_Ucircumflex
0x0dd GDK_Yacute
0x0de GDK_THORN
0x0dc GDK_Udiaeresis
0x0bc GDK_onequarter
0x0be GDK_threequarters

There are keyboard layouts for which the conflicts are realized:
Canadian Multilingual, second part has three of them.  Icelandic keyboards use THORN.  Users on Linux are free to define their own keyboard layouts.

So, it's best to use an algorithm to assign keycodes that evades the problems.  That is, the final version of GdkKeyCodeToDOMKeyCodeExtended should take these eight keyvalues into account.  If the algorithm it employs attempts to use one of the forbidden values then this should be detected and another keyvalue used instead (from another shift state).

#5. Range of acceptable values
==============================
There are places in the gtk keyboard code where a value between 0 and 255 is assumed.  This has got to be removed.  Many of the values recorded in gdk/gdkkeysyms.h require an unsigned 16 bits.

Dead keys are all >65000.
Comment 6 Allan Jacobs 2008-08-12 01:04:51 PDT
I tried using something like the algorithm in the above comments and it failed miserably.  The problems were caused by a misinterpretation of the meaning of the reference to keyvals in
gdk_keymap_get_entries_for_keycode(keymap, event->hardware_keycode,
        &keys, &keyvals, &n_entries);
The keyvals are not Unicode and they are not key codes.  They seem to be equivalent to the Unicode of the key as if it were typed in the US layout.

Something much simpler is called for.  The following code has been tested in US, Israel, and Iceland...  The arrow keys, Insert, Home, Page Up, Delete, End, Page Down, and numeric keypad keys all seem to work fine.

int
GdkKeyCodeToDOMKeyCodeExtended(GdkEventKey *event)
{
    int aKeysym, i, j, klength;
    int length = sizeof(nsKeycodes) / sizeof(struct nsKeyConverter);
    gint n_entries = 0;
    guint *keyvals = NULL;
    GdkKeymapKey *keys = NULL;
    GdkKeymap *keymap = gdk_keymap_get_default();

    gdk_keymap_get_entries_for_keycode(keymap, event->hardware_keycode,
        &keys, &keyvals, &n_entries);
    klength = sizeof(keyvals) > 4 ? 4 : sizeof(keyvals);

    // Numeric keys
    for (j = 0; j < klength; j++) {
        aKeysym = keyvals[j];
        if (aKeysym >= GDK_0 && aKeysym <= GDK_9)
            return aKeysym - GDK_0 + NS_VK_0;
    }

    // Alphabetic keys
    for (j = 0; j < klength; j++) {
        aKeysym = keyvals[j];
        if (aKeysym >= GDK_A && aKeysym <= GDK_Z)
            return aKeysym - GDK_A + NS_VK_A;
    }

    // keypad numbers
    aKeysym = event->keyval;
    if (aKeysym >= GDK_KP_0 && aKeysym <= GDK_KP_9)
        return aKeysym - GDK_KP_0 + NS_VK_NUMPAD0;

    // map Sun Keyboard special keysyms
    if (IS_XSUN_XSERVER(GDK_DISPLAY())) {
        length = sizeof(nsSunKeycodes) / sizeof(struct nsKeyConverter);
        aKeysym = event->keyval;
        for (i = 0; i < length; i++) {
            if (nsSunKeycodes[i].keysym == aKeysym)
                return(nsSunKeycodes[i].vkCode);
        }
    }

    // key keyvals            return value
    // `/~ (60 7e 0 0)        NS_VK_BACK_QUOTE
    // -/_ (2d 5f 0 0)        NS_VK_SUBTRACT
    // =/+ (3d 2b 0 0)        NS_VK_EQUALS
    // [/{ (5b 7b 0 0)        NS_VK_OPEN_BRACKET
    // ]/} (5d 7d 0 0)        NS_VK_CLOSE_BRACKET
    // ;/: (3b 59 0 0)        NS_VK_SEMICOLON
    // '/" (27 22 0 0)        NS_VK_QUOTE
    // \/| (5c 7c 0 0)        NS_VK_BACK_SLASH
    // ,/< (2c 3c 0 0)        NS_VK_COMMA
    // ./> (2e 3e 0 0)        NS_VK_PERIOD
    // //? (2f 3f 0 0)        NS_VK_SLASH
    // (105) (3c 3e 7c a6)    0xf9

    // misc other things
    aKeysym = keyvals[0];
    if (aKeysym == 0x0060) return NS_VK_BACK_QUOTE;
    if (aKeysym == 0x002d) return NS_VK_SUBTRACT;
    if (aKeysym == 0x003d) return NS_VK_EQUALS;
    if (aKeysym == 0x005b) return NS_VK_OPEN_BRACKET;
    if (aKeysym == 0x005d) return NS_VK_CLOSE_BRACKET;
    if (aKeysym == 0x003b) return NS_VK_SEMICOLON;
    if (aKeysym == 0x0027) return NS_VK_QUOTE;
    if (aKeysym == 0x005c) return NS_VK_BACK_SLASH;
    if (aKeysym == 0x002c) return NS_VK_COMMA;
    if (aKeysym == 0x002e) return NS_VK_PERIOD;
    if (aKeysym == 0x002f) return NS_VK_SLASH;
    if (aKeysym == 0x003c) return 0xe2;

    // misc other things
    // the source of all keyboard evil
    length = sizeof(nsKeycodes) / sizeof(struct nsKeyConverter);
    for (i = 0; i < length; i++) {
        if (nsKeycodes[i].keysym == aKeysym)
            return(nsKeycodes[i].vkCode);
    }

    // function keys
    if (aKeysym >= GDK_F1 && aKeysym <= GDK_F24)
        return aKeysym - GDK_F1 + NS_VK_F1;

    return 0;
}

Dead keys are still not triggering keyup, keypress, or keydown events.  This is covered by another bug report (or bug reports).

The key mapping to 0xe2 (226) is for the 105-th key in a 105-key keyboard.  This choice of NS_VK keycode needs to be checked carefully.  On Windows XP, Firefox uses a keycode of 226 for this key (except for a few stray English language layouts, Gaelic, and Azeri Latin where 220 is used).

This fix is good enough for review.
Comment 7 Allan Jacobs 2008-08-12 20:26:26 PDT
One place where dead keys are getting swallowed is in nsWindow.cpp in the method IMEFilterEvent(GdkEventKey *aEvent).  The GTK method gtk_im_context_filter_keypress is returning true.

Another bug in the original code: the AltGr key (keyval 65027) is not the same as GDK_Alt_R.  So, the AltGr key is not mapped to NS_VK_ALT in nsGtkKeyUtils.cpp. In gdk/gdkkeysyms.h, the AltGr key is called GDK_ISO_Level3_Shift

#define GDK_ISO_Level3_Shift 0xfe03
Comment 8 Nickolay_Ponomarev 2010-09-03 13:27:40 PDT
Ugh, sorry your report wasn't triaged in two years! You didn't find anyone to talk about this via other channels (newsgroups, irc), did you? Is it still a problem in more recent builds?

BTW, here's the process for getting changes into the source tree, if you're still interested https://developer.mozilla.org/en/Getting_your_patch_in_the_tree
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2010-09-06 19:26:49 PDT
*** Bug 583721 has been marked as a duplicate of this bug. ***
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2010-09-06 19:27:47 PDT
Per bug 583721 (and my attempt to reproduce just now), this is in fact a problem.  It could really use an owner, too...
Comment 11 Allan Jacobs 2010-09-09 22:54:02 PDT
I'm looking at it again.  Dead key handling is first.
Comment 12 Allan Jacobs 2010-09-10 16:36:18 PDT
Created attachment 474270 [details]
HTML example appropriate for year 2010
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-11-09 22:53:55 PST
I'll post a patch for all keyboard layouts.
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-11-10 22:24:01 PST
Created attachment 489762 [details] [diff] [review]
Patch v1.0

This fixes this bug temporarily.

This patch computes the keycode from current keyboard layout without modifier keys, first. If it fails, retry with Latin inputtable keyboard layout.

I think that we should rethink about the symbol keycodes like ';', ':' and '/' on all platforms but it should be another bug.
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-11-10 22:31:46 PST
Created attachment 489763 [details] [diff] [review]
Patch v1.0.1
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-11-10 22:32:46 PST
Comment on attachment 489763 [details] [diff] [review]
Patch v1.0.1

fix a nit.
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-11-10 23:06:38 PST
Comment on attachment 489763 [details] [diff] [review]
Patch v1.0.1

Sorry for the spam, I find a problem, I'll post a new patch soon.
Comment 18 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-11-10 23:32:17 PST
Created attachment 489768 [details] [diff] [review]
Patch v1.0.2

If the found Latin inputtable keyboard layout inputs a unicode character for a key, this patch still fails to compute the keycode. (E.g., first layout is Greek, second layout is France Bepo, then, press 'z' key of US layout.)

I checked Windows' behavior, however, Windows build computes wrong keycode too. For some unicode keys, I need to work more in another bug :-(
Comment 19 Karl Tomlinson (:karlt) 2010-11-16 14:04:56 PST
Comment on attachment 489768 [details] [diff] [review]
Patch v1.0.2

This looks good to me, thanks.
Can the US Keyboard shift state correction hacks be removed now?

http://hg.mozilla.org/mozilla-central/annotate/a1e24da8518f/widget/src/gtk2/nsGtkKeyUtils.cpp#l156

>+    if (gdk_keymap_translate_keyboard_state(NULL,
>+            aGdkEvent->hardware_keycode, GdkModifierType(0), aGdkEvent->group,
>+            &keyval, NULL, NULL, NULL) && keyval != 0) {

>+            gdk_keymap_translate_keyboard_state(NULL,
>+                aGdkEvent->hardware_keycode, GdkModifierType(0), group,
>+                &keyval, NULL, NULL, NULL) && keyval != 0) {

AFAICS the "keyval != 0" tests are unnecessary and so can be removed.
r=me with that change.
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-11-17 04:25:29 PST
Created attachment 491163 [details] [diff] [review]
Patch v1.0.3

(In reply to comment #19)
> Can the US Keyboard shift state correction hacks be removed now?
> 
> http://hg.mozilla.org/mozilla-central/annotate/a1e24da8518f/widget/src/gtk2/nsGtkKeyUtils.cpp#l156

If I remove the hack, the keycode is still zero with some keyboard layouts which inputs non-breakable-space by Shift+SPACE.

This patch computes keycode with the keyval of the given event, first.
Next, computes from current keyboard layout without modifier keys.
Finally, computes from Latin inputtable layout.

I think that this is better for now.
Comment 21 Karl Tomlinson (:karlt) 2010-11-17 14:16:35 PST
(In reply to comment #20)
> (In reply to comment #19)
> > Can the US Keyboard shift state correction hacks be removed now?
> > 
> > http://hg.mozilla.org/mozilla-central/annotate/a1e24da8518f/widget/src/gtk2/nsGtkKeyUtils.cpp#l156
> 
> If I remove the hack, the keycode is still zero with some keyboard layouts
> which inputs non-breakable-space by Shift+SPACE.

That is the case with and without the hack, right?
If so, what is the hack providing?  i.e. why keep it?
I looks like it could only cause confusion by mapping to the wrong keys.

> This patch computes keycode with the keyval of the given event, first.
> Next, computes from current keyboard layout without modifier keys.
> Finally, computes from Latin inputtable layout.
> 
> I think that this is better for now.

Can you explain why you think that is better, please?

I thought keyCode should represent a key rather than any particular symbol
produced through the key and a combination of modifiers.
If the keyval of the event is used first then the keyCode would change with
different modifiers, which seems wrong to me.
Comment 22 Karl Tomlinson (:karlt) 2010-11-17 14:45:36 PST
A couple of other situations are worth considering:

* It looks like DOM keycodes normally expect numeric keypads to report the
  number rather than directions (left/right/up/down/home/end/pgup/pgdown).
  However, the number keyvals are typically on level 1 (not 0), and sometimes
  even different levels.

  Passing the numlock modifier to gdk_keymap_translate_keyboard_state would
  probably work here, but it looks like it could be very difficult to actually
  determine which is the numlock modifier.  (It may be GDK_MOD2_MASK, but
  that's not necessarily the case, and I don't know how many exceptions there
  might be.)

* Do some Linux keyboard layouts have the number keyvals on the level 1 (with
  Shift) of the top-row keys?  French or Belgian maybe?  If so, I expect these
  keys should have numeric keycodes rather than punctation keycodes.

These situtations suggest an approach where the keyCode should be chosen from
the keyvals in a way such that the keyval can (at least sometimes) have
priority over the level.  Numbers might have first priority, then Latin letters, then perhaps there could even be a priority order for punctuation (IIRC that's how it seemed the win32 keycodes were chosen).

gdk_keymap_get_entries_for_keycode would be the way to find the keyvals associated with each key, and their levels.
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-11-17 18:08:48 PST
(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > Can the US Keyboard shift state correction hacks be removed now?
> > > 
> > > http://hg.mozilla.org/mozilla-central/annotate/a1e24da8518f/widget/src/gtk2/nsGtkKeyUtils.cpp#l156
> > 
> > If I remove the hack, the keycode is still zero with some keyboard layouts
> > which inputs non-breakable-space by Shift+SPACE.
> 
> That is the case with and without the hack, right?

That is the case without the hack.

> If so, what is the hack providing?  i.e. why keep it?
> I looks like it could only cause confusion by mapping to the wrong keys.

If Shift+SPACE generates non-breakable-space or something except space, the keycode is computed as 0 with given keyval.  However, if the SPACE key inputs a normal space character, the hack succeeds to get the DOM space keycode.

> > This patch computes keycode with the keyval of the given event, first.
> > Next, computes from current keyboard layout without modifier keys.
> > Finally, computes from Latin inputtable layout.
> > 
> > I think that this is better for now.
> 
> Can you explain why you think that is better, please?

If the shifted character isn't ASCII character but unshifted character is ASCII character, the hack can computes the DOM keycode at second step.

> I thought keyCode should represent a key rather than any particular symbol
> produced through the key and a combination of modifiers.
> If the keyval of the event is used first then the keyCode would change with
> different modifiers, which seems wrong to me.

DOM3 spec said: keyCode holds a system- and implementation-dependent numerical code signifying the unmodified identifier associated with the key pressed.
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-KeyboardEvent-keyCode

So, the hack provides same keycode for both unshifted key event and shifted key event.
# But some keys mapped to other keycode only on Linux. That was decided from US keyboard layout. I think this hack should be removed.

(In reply to comment #22)
> * It looks like DOM keycodes normally expect numeric keypads to report the
>   number rather than directions (left/right/up/down/home/end/pgup/pgdown).
>   However, the number keyvals are typically on level 1 (not 0), and sometimes
>   even different levels.

http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/events/nsIDOMKeyEvent.idl#130
The keycode should be NS_VK_NUMPAD*

>   Passing the numlock modifier to gdk_keymap_translate_keyboard_state would
>   probably work here, but it looks like it could be very difficult to actually
>   determine which is the numlock modifier.  (It may be GDK_MOD2_MASK, but
>   that's not necessarily the case, and I don't know how many exceptions there
>   might be.)

We will need to do it for getModifierState.
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-KeyboardEvent-getModifierState

> * Do some Linux keyboard layouts have the number keyvals on the level 1 (with
>   Shift) of the top-row keys?  French or Belgian maybe?  If so, I expect these
>   keys should have numeric keycodes rather than punctation keycodes.

The spec doesn't said so... But on Window, we do so. If we do so on all platforms, it needs special code for such layout on Linux.

> These situtations suggest an approach where the keyCode should be chosen from
> the keyvals in a way such that the keyval can (at least sometimes) have
> priority over the level.  Numbers might have first priority, then Latin
> letters, then perhaps there could even be a priority order for punctuation
> (IIRC that's how it seemed the win32 keycodes were chosen).

We should be CC'ing smaug.

I think that it makes sense that always we use level 0 for computing keycode (as spec said). But it's for us, it may not be for web application developers.

Developers should use keypress event or textinput event if they need actual inputting character. But this logic means they cannot use numeric keys for their application's shortcut key.

> gdk_keymap_get_entries_for_keycode would be the way to find the keyvals
> associated with each key, and their levels.

Yes.
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-11-17 18:26:20 PST
> (In reply to comment #22)
>> * It looks like DOM keycodes normally expect numeric keypads to report the
>>   number rather than directions (left/right/up/down/home/end/pgup/pgdown).
>>   However, the number keyvals are typically on level 1 (not 0), and sometimes
>>   even different levels.
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/events/nsIDOMKeyEvent.idl#130
> The keycode should be NS_VK_NUMPAD*

Oh, but this must break our code.
Comment 25 Karl Tomlinson (:karlt) 2010-11-17 19:17:36 PST
I'm not sure we're talking about the same "hack".  I'm referring only to the
mapping from US keyboard shift-on punctuation keyvals to shift-off keyvals
that would be found on the same key of a US keyboard.

I think this is now unnecessary (now that the unmodified keyval is being
used), and best removed, and I think you are agreeing?
 
http://hg.mozilla.org/mozilla-central/annotate/a1e24da8518f/widget/src/gtk2/nsGtkKeyUtils.cpp#l156
Comment 26 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-11-17 20:41:21 PST
(In reply to comment #25)
> I'm not sure we're talking about the same "hack".  I'm referring only to the
> mapping from US keyboard shift-on punctuation keyvals to shift-off keyvals
> that would be found on the same key of a US keyboard.
> 
> I think this is now unnecessary (now that the unmodified keyval is being
> used), and best removed, and I think you are agreeing?
> 
> http://hg.mozilla.org/mozilla-central/annotate/a1e24da8518f/widget/src/gtk2/nsGtkKeyUtils.cpp#l156

Ah, okay. I misunderstand. I'll remove it.
Comment 27 Karl Tomlinson (:karlt) 2010-11-17 21:13:25 PST
I liked version v1.0.2 better than v1.0.3, because using aGdkEvent->keyval
(from the modified state) will mean that the keyCode could be different with
different modifiers.

Version 1.0.2 follows the spec literally (thanks for the links) always using an unmodified keyval.

There is still the questions of whether to provide NS_VK_NUMPAD* (which will never be produced without modifiers) and whether to prefer ASCII digits when the level 0 keyval is punctation.

In a sense NS_VK_NUMPAD* is keeping with the spec, just that those are the codes we choose to use for the "unmodified identifier" (which is the direction).
Do all numeric keypads have the directional keyvals laid out similarly and the numbers with 1 at the bottom?
Perhaps it would be OK to map GDK_KP_End to DOM_VK_NUMPAD1, etc.?
(That can be done in a separate patch.)

I don't know what is best when ASCII digits are on level 1 of the main keyboard, but I don't even know if that happens on Linux, so we may not need to worry.
Comment 28 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-11-18 00:07:08 PST
(In reply to comment #27)
> I liked version v1.0.2 better than v1.0.3, because using aGdkEvent->keyval
> (from the modified state) will mean that the keyCode could be different with
> different modifiers.
> 
> Version 1.0.2 follows the spec literally (thanks for the links) always using an
> unmodified keyval.

Yes. I like 1.0.2. I understood that the "hack" is the first if block of v1.0.2.

> There is still the questions of whether to provide NS_VK_NUMPAD* (which will
> never be produced without modifiers) and whether to prefer ASCII digits when
> the level 0 keyval is punctation.
> 
> In a sense NS_VK_NUMPAD* is keeping with the spec, just that those are the
> codes we choose to use for the "unmodified identifier" (which is the
> direction).

Um, but my patch converts the keycode to Home/PageUp/End/PageDown/Ins/Del/Arrow keys. I didn't assume that this behavior. So, I need more time for the next patch.

> Do all numeric keypads have the directional keyvals laid out similarly and the
> numbers with 1 at the bottom?
> Perhaps it would be OK to map GDK_KP_End to DOM_VK_NUMPAD1, etc.?
> (That can be done in a separate patch.)

On Windows, if numlocked, the keycodes are NS_VK_NUMPAD*. Otherwise, arrow keys or others. I think that this is reasonable. I think we should think separately about NumLock because on notebook, 7, 8, 9, u, i, o, j, k and l are used for numpad at numlocked. On these cases, NS_VK_NUMPAD* must be more usable than NS_VK_7 and others.

So, I think that if the keyval means numeric or operator and we if can know whether the NumLock is enabled or not. Then, we should use NS_VK_NUMPAD*. Otherwise, we should use the unmodified keycode.

> I don't know what is best when ASCII digits are on level 1 of the main
> keyboard, but I don't even know if that happens on Linux, so we may not need to
> worry.

On Windows, we send the level 1 value for them. I guess that we should use NS_VK_[0-9] for compatibility with Windows and other keyboard layouts.
Comment 29 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-11-18 00:11:30 PST
I think we should be able to use nsWindow::GetToggledKeyState() for checking whether NumLock is locked.
Comment 30 Karl Tomlinson (:karlt) 2010-11-21 15:39:41 PST
Yes, gdk_keyboard_get_modmap_masks has the right logic for finding the mask for the state of the NumLock modifier.

However that fetches quite a lot of information.  It would make sense to store the masks and only recalculate when necessary.  The "keys-changed" signal is available to indicate when the masks may have changed.

http://library.gnome.org/devel/gdk/stable/gdk-Keyboard-Handling.html#GdkKeymap-keys-changed

I wonder whether maybe the state bit corresponding to GDK_LOCK_MASK should also be treated similarly to the NumLock bit.  This won't make any difference with standard caps-lock, but if it is a shift-lock or moves level 1 digits to level 0, then it would make a difference.
Comment 31 Karl Tomlinson (:karlt) 2010-11-21 15:58:38 PST
(In reply to comment #30)
> I wonder whether maybe the state bit corresponding to GDK_LOCK_MASK should also
> be treated similarly to the NumLock bit.  This won't make any difference with
> standard caps-lock, but if it is a shift-lock or moves level 1 digits to level
> 0, then it would make a difference.

I forgot about your commment here:

(In reply to comment #28)
> On Windows, we send the level 1 value for them. I guess that we should use
> NS_VK_[0-9] for compatibility with Windows and other keyboard layouts.

I'm fine with that, so no need to think about GDK_LOCK_MASK, if you choose to do that.
Comment 32 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-08 21:23:21 PST
Created attachment 510939 [details] [diff] [review]
Patch v2.0

You need to apply following patches if you want to apply this patch on current trunk:

* bug 630810
* bug 630817
* bug 630813
Comment 33 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-09 20:41:35 PST
Created attachment 511286 [details] [diff] [review]
Patch v2.1
Comment 34 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-10 20:56:22 PST
Created attachment 511633 [details] [diff] [review]
Patch v2.2
Comment 35 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-10 21:02:09 PST
You can test the patch on following URL:
> data:text/html,<p id="p"></p><script>function keyname(event) { for (var s in event) { if (s.match(/^DOM_VK_/) && event[s] == event.keyCode) { return s; } } return "0x" + event.keyCode.toString(16); } function log(event) { var p = document.getElementById("p"); p.innerHTML = event.type + " keyCode=" + keyname(event) + ", charCode=" + event.charCode + " (0x" + event.charCode.toString(16) + "), shift=" + event.shiftKey + ",ctrl=" + event.ctrlKey + ", alt=" + event.altKey + ", meta=" + event.metaKey + "<BR>" + p.innerHTML; event.preventDefault(); } window.addEventListener("keydown", log, false); window.addEventListener("keypress", log, false); window.addEventListener("keyup", log, false);</script>

I tested on Japanese, French, Icelandic, Greek and Thai keyboard layouts.
Comment 36 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-10 21:31:40 PST
(In reply to comment #30)
> I wonder whether maybe the state bit corresponding to GDK_LOCK_MASK should also
> be treated similarly to the NumLock bit.  This won't make any difference with
> standard caps-lock, but if it is a shift-lock or moves level 1 digits to level
> 0, then it would make a difference.

Oops, I missed catching this. I'll check this issue. Do you know such actual keyboard layouts?

Note that DOM3's KeyboardEvent.key should honor the lock state and AltGr state [*1]. If web applications need to distinguish the difference strictly, they should use it rather than legacy keyCode property. If I can fix bug 631165 related bugs soon, we can implement the new DOM3 property "key" and "char". Then, keyCode and charCode shouldn't be used on new web applications.

*1 http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#keys-Guide
Comment 37 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-10 22:04:28 PST
(In reply to comment #36)
> (In reply to comment #30)
> > I wonder whether maybe the state bit corresponding to GDK_LOCK_MASK should also
> > be treated similarly to the NumLock bit.  This won't make any difference with
> > standard caps-lock, but if it is a shift-lock or moves level 1 digits to level
> > 0, then it would make a difference.
> 
> Oops, I missed catching this. I'll check this issue. Do you know such actual
> keyboard layouts?

Oh, I misread your comment. I misunderstood level as group.

I disagree. keyCode property must be an index of the (physical) key. We shouldn't change the result from the difference of any modifier key state. If we do so, we should ignore AltGr too. But if we do so, the keyCode means the preferred ASCII character of the key with current modifier state. I don't think this is better behavior.

As I said in comment 36, we should hurry to implement the new DOM3 properties. I think that the importance of keyCode behavior:

1. Don't break compatibility with Fx4 and earlier as far as possible.
2. But should return same value for same (similar) keyboard layout on all platforms.
3. And should return non-zero keyCode for non-Latin keyboard layout on all platforms.

To refer the unmodified character of a key is the best way for compatibility between platforms.
Comment 38 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-10 22:07:07 PST
> To refer the unmodified character of a key is the best way for compatibility
between platforms.

For example, AltGred layout on Win and Linux may not be different from Optioned layout.
Comment 39 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-10 22:08:14 PST
(In reply to comment #38)
> > To refer the unmodified character of a key is the best way for compatibility
> between platforms.
> 
> For example, AltGred layout on Win and Linux may not be different from Optioned
> layout.

Optioned layout on Mac, I meant.

Sorry for the spam.
Comment 40 Allan Jacobs 2011-08-05 16:42:43 PDT
Bug 445439 contains information needed to find the unmodified character of a key in a way that will be Windows compatible.  The information is in the attachments. I might have a table JavaScript implementation of the mapping that was intended to fix Firefox.  The table is too heavy to download to every client session, so it was never a good solution for the problem.
Comment 41 Virgil Dicu [:virgil] [QA] 2011-09-16 02:42:10 PDT
Hello.

Does this issue cover not fired Key press events, as in Bug 676259?
Comment 42 Karl Tomlinson (:karlt) 2011-09-18 14:04:34 PDT
(In reply to Virgil Dicu [QA] from comment #41)
No.  Bug 676259 is a separate issue.  This bug is (almost) specific to keyup/keydown events.
Comment 43 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-03-14 20:33:30 PDT
Created attachment 606102 [details] [diff] [review]
part.1 Compute DOM keycode for non-printable key from table first

This patch makes computing keycode of all non-printable keys simpler.

The changing order in ComputeDOMKeyCode() isn't important in this patch. See following patches.
Comment 44 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-03-14 20:35:57 PDT
Created attachment 606103 [details] [diff] [review]
part.2 DOM keycode for printable keys in Numpad should be computed by switch

This patch makes computing keycode of all printable keys in numpad use switch statement. The kKeyParis should be used only for non-printable keys.
Comment 45 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-03-14 21:04:30 PDT
I found a bug in part.3 patch. Please wait a couple of hours...

Note that these patches need the patches in bug 630810 and bug 677252.
Comment 46 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-03-14 21:52:58 PDT
Created attachment 606111 [details] [diff] [review]
part.3 Use unshifted char of printable keys for computing keycode on GTK

This is the most important patch for this bug.

This patch computes keycode of printable keys from:

1. unshifted charcode of the key if it's [a-zA-Z0-9].
2. shifted charcode of the key if it's [a-zA-Z0-9].
3. unshifted latin inputtable keyboard layout if current layout cannot input ASCII alphabet and it's [a-zA-Z0-9].
4. shifted latin inputtable keyboard layout if current layout cannot input ASCII alphabet and it's [a-zA-Z0-9].
5. unshifted charcode of the key in current layout if it's in ASCII range.
6. shifted charcode of the key in current layout if it's in ASCII range.

This patch completely removes printable keys from the key pair table.
Comment 47 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-03-14 21:56:15 PDT
Created attachment 606112 [details] [diff] [review]
part.4 Clean up modifier keycode mapping on GTK

This changes the key mapping between GDK's modifiers and DOM keycodes.

GDK_Meta_L/GDK_Meta_R are not mapped any DOM keycode in this patch. Should be mapped to NS_VK_ALT?
Comment 48 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-03-14 23:45:46 PDT
tryserver builds:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/masayuki@d-toybox.com-27c9738677c3/
Comment 49 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-03-22 08:37:36 PDT
karlt: ping
Comment 50 Karl Tomlinson (:karlt) 2012-03-22 14:26:08 PDT
I have a number of review requests so it will take me time to get to this.
I'll treat this bug as the most important of your review requests (unless I hear otherwise).
Comment 51 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-02 00:07:35 PDT
(In reply to Karl Tomlinson (:karlt) from comment #50)
> I'll treat this bug as the most important of your review requests (unless I
> hear otherwise).

Yes. Especially, this blocks landing similar patches for Win/Mac for consistency between tier-1 platforms. So, I'd like to know when you can review them. Ambiguous schedule is okay, I just want to know it for scheduling my other work.
Comment 52 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-02 00:09:24 PDT
oops, session restore sometimes cause unexpected status change, sorry.
Comment 53 Karl Tomlinson (:karlt) 2012-04-02 00:26:04 PDT
I'm very unlikely to get to this in the 3 days remaining this week and I'm on vacation the following week.  I can aim to start sometime the week of the 16th, but that will change if something more urgent arises.
Comment 54 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-04-02 23:05:15 PDT
(In reply to Karl Tomlinson (:karlt) from comment #53)
> I'm very unlikely to get to this in the 3 days remaining this week and I'm
> on vacation the following week.  I can aim to start sometime the week of the
> 16th, but that will change if something more urgent arises.

Okay, thanks.
Comment 55 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-05-07 20:26:58 PDT
Created attachment 621852 [details] [diff] [review]
part.1 Compute DOM keycode for non-printable key from table first

Updated for latest trunk. Karl, could you review these patches? I need to fix this bug before I work on bug 680830.
Comment 56 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-05-07 20:27:32 PDT
Created attachment 621853 [details] [diff] [review]
part.2 DOM keycode for printable keys in Numpad should be computed by switch
Comment 57 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-05-07 20:28:05 PDT
Created attachment 621854 [details] [diff] [review]
part.3 Use unshifted char of printable keys for computing keycode on GTK
Comment 58 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-05-07 20:28:36 PDT
Created attachment 621856 [details] [diff] [review]
part.4 Clean up modifier keycode mapping on GTK
Comment 59 Karl Tomlinson (:karlt) 2012-05-14 20:53:16 PDT
Comment on attachment 621852 [details] [diff] [review]
part.1 Compute DOM keycode for non-printable key from table first

>-    //{ NS_VK_,        GDK_KP_Begin },
>+    { NS_VK_CLEAR,      GDK_KP_Begin }, // Num-unlocked 5

Why do you pick NS_VK_CLEAR here?
I thought NS_VK_CLEAR was the "clear" key on an Apple keyboard, which is normally where Num Lock would be.
I expect that is what GDK_Clear corresponds to.
Comment 60 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-05-14 21:13:36 PDT
(In reply to Karl Tomlinson (:karlt) from comment #59)
> Comment on attachment 621852 [details] [diff] [review]
> part.1 Compute DOM keycode for non-printable key from table first
> 
> >-    //{ NS_VK_,        GDK_KP_Begin },
> >+    { NS_VK_CLEAR,      GDK_KP_Begin }, // Num-unlocked 5
> 
> Why do you pick NS_VK_CLEAR here?
> I thought NS_VK_CLEAR was the "clear" key on an Apple keyboard, which is
> normally where Num Lock would be.
> I expect that is what GDK_Clear corresponds to.

On Windows, when NunLock is unlocked, "5" key on Numpad sends VK_CLEAR. The VK_CLEAR is mapped to NS_VK_CLEAR. And most keycodes are same value as virtual keycode on Windows. So, I think that using same keycode for same key makes sense.
Comment 61 Karl Tomlinson (:karlt) 2012-05-14 21:21:25 PDT
OK.  Makes sense, thanks.
Comment 62 Karl Tomlinson (:karlt) 2012-05-14 23:11:50 PDT
Comment on attachment 621852 [details] [diff] [review]
part.1 Compute DOM keycode for non-printable key from table first

>+     * GetGDKKeyvalWithoutModifier() returns the keyval for aGdkKeyEvent when
>+     * ignores the modifier state except CapsLock, NumLock and ScrollLock.

s/ignores/ignoring/

Why is it important to consider CapsLock here?
Ignoring CapsLock here would be more consistent with part 3 and would save a
gdk_keymap_translate_keyboard_state call in part 3.

>+    // If the key isn't printable, let's look at the key pairs.
>+    PRUint32 charCode = GetCharCodeFor(aGdkKeyEvent);
>+    if (charCode < ' ') {

Is this essentially equivalent to charCode != 0?
If so, then I think that would be clearer.

Removing the keypair logic from below, and perhaps adding this block, belong
in part 3, because, at this point in the series, even US unshifted
keyvals won't generate keyCodes.
Comment 63 Karl Tomlinson (:karlt) 2012-05-14 23:15:32 PDT
Comment on attachment 621854 [details] [diff] [review]
part.3 Use unshifted char of printable keys for computing keycode on GTK

>+    // Ignore all modifier state except NumLock and ScrollLock.
>+    guint baseState = (aGdkKeyEvent->state &
>+        ~(keymapWrapper->GetModifierMask(SHIFT) |
>+          keymapWrapper->GetModifierMask(CTRL)  |
>+          keymapWrapper->GetModifierMask(ALT)   |
>+          keymapWrapper->GetModifierMask(SUPER) |
>+          keymapWrapper->GetModifierMask(HYPER) |
>+          keymapWrapper->GetModifierMask(META)  |
>+          keymapWrapper->GetModifierMask(ALTGR) |
>+          keymapWrapper->GetModifierMask(CAPS_LOCK)));

baseState = aGdkKeyEvent->state &
           (keymapWrapper->GetModifierMask(NUM_LOCK) |
            keymapWrapper->GetModifierMask(SCROLL_LOCK));

would be simpler.  Would that work fine?
I doubt SCROLL_LOCK makes any difference, but it won't do any harm either, so
I don't mind whether that is there or not.

>+    // If the shited unmodified character isn't an ASCII character, we should

"shifted"

>+    // However, if the key doesn't input such characters on the alternative
>+    // layout, we shouldn't use it because it causes keyCode conflict on
>+    // a keyboard layout.  E.g., a key for "a with umlaut" of German keyboard
>+    // layout is same as "period" key of US keyboard layout.  If we use
>+    // NS_VK_PERIOD for the key, it conflicts with "period" key of German
>+    // keyboard layout.  That causes unexpected behavior for German keyboard
>+    // layout users.

The German keyboard is not a good example here because it is Latin, and so no
other Latin layout will be searched.  But I guess the principal extends to
some non-Latin layouts with ASCII punctuation.

>     /**
>+     * GetMinLatinInputtableGroup() returns group of mGdkKeymap which
>+     * can input an ASCII character by GDK_A.
>+     *
>+     * @return                  group value of GdkEventKey.
>+     */
>+    gint GetMinLatinInputtableGroup();
>+
>+    /**
>+     * IsASCIIAlphabetInputtableLayout() checkes whether the keyboard
>+     * layout of aGdkKeyEvent is ASCII alphabet inputtable or not.
>+     *
>+     * @param aGdkKeyEvent      Must not be NULL.
>+     * @return                  TRUE if the keyboard layout can input
>+     *                          ASCII alphabet.  Otherwise, FALSE.
>+     */
>+    bool IsASCIIAlphabetInputtableLayout(const GdkEventKey* aGdkKeyEvent);

Can the names be simplified to GetFirstLatinGroup and IsLatinGroup?
The parameter for the second method should be a group instead of a GdkKeyEvent.
Comment 64 Karl Tomlinson (:karlt) 2012-05-14 23:34:43 PDT
Comment on attachment 621856 [details] [diff] [review]
part.4 Clean up modifier keycode mapping on GTK

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #47)
> GDK_Meta_L/GDK_Meta_R are not mapped any DOM keycode in this patch. Should
> be mapped to NS_VK_ALT?

The changes in part 1 here should make it unnecessary to map GDK_Meta_* to ALT when they correspond to the same modifier.

When GDK_Meta_* is (rarely) actually a different modifier to GTK_Alt_*, then it may make sense to map it to another modifier.

I see in xkb/keycodes/evdev these lines:

  // Solaris compatibility
  alias <LMTA> = <LWIN>;
  alias <RMTA> = <RWIN>;

So it may make sense to map GDK_Meta_* to NS_VK_WIN, but I don't mind whether or not that happens in this patch.  Perhaps we can let some Solaris people advise us later.
Comment 65 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-05-15 06:21:46 PDT
Created attachment 624017 [details] [diff] [review]
part.1 Compute DOM keycode for non-printable key from table first

(In reply to Karl Tomlinson (:karlt) from comment #62)
> Comment on attachment 621852 [details] [diff] [review]
> part.1 Compute DOM keycode for non-printable key from table first
> 
> >+     * GetGDKKeyvalWithoutModifier() returns the keyval for aGdkKeyEvent when
> >+     * ignores the modifier state except CapsLock, NumLock and ScrollLock.
> 
> Why is it important to consider CapsLock here?
> Ignoring CapsLock here would be more consistent with part 3 and would save a
> gdk_keymap_translate_keyboard_state call in part 3.

You're right. We don't need to ignore CpasLock and ScrollLock because only NumLock changes some keys' meaning.

> >+    // If the key isn't printable, let's look at the key pairs.
> >+    PRUint32 charCode = GetCharCodeFor(aGdkKeyEvent);
> >+    if (charCode < ' ') {
> 
> Is this essentially equivalent to charCode != 0?
> If so, then I think that would be clearer.

I replaced it with |if (!charCode) {|
Comment 66 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-05-15 06:25:25 PDT
Created attachment 624018 [details] [diff] [review]
part.3 Use unshifted char of printable keys for computing keycode on GTK

(In reply to Karl Tomlinson (:karlt) from comment #63)
> Comment on attachment 621854 [details] [diff] [review]
> part.3 Use unshifted char of printable keys for computing keycode on GTK
> 
> >+    // Ignore all modifier state except NumLock and ScrollLock.
> >+    guint baseState = (aGdkKeyEvent->state &
> >+        ~(keymapWrapper->GetModifierMask(SHIFT) |
> >+          keymapWrapper->GetModifierMask(CTRL)  |
> >+          keymapWrapper->GetModifierMask(ALT)   |
> >+          keymapWrapper->GetModifierMask(SUPER) |
> >+          keymapWrapper->GetModifierMask(HYPER) |
> >+          keymapWrapper->GetModifierMask(META)  |
> >+          keymapWrapper->GetModifierMask(ALTGR) |
> >+          keymapWrapper->GetModifierMask(CAPS_LOCK)));
> 
> baseState = aGdkKeyEvent->state &
>            (keymapWrapper->GetModifierMask(NUM_LOCK) |
>             keymapWrapper->GetModifierMask(SCROLL_LOCK));
> 
> would be simpler.  Would that work fine?
> I doubt SCROLL_LOCK makes any difference, but it won't do any harm either, so
> I don't mind whether that is there or not.

Right, it's now:

    // Ignore all modifier state except NumLock.
    guint baseState =
        (aGdkKeyEvent->state & keymapWrapper->GetModifierMask(NUM_LOCK));

> >+    // However, if the key doesn't input such characters on the alternative
> >+    // layout, we shouldn't use it because it causes keyCode conflict on
> >+    // a keyboard layout.  E.g., a key for "a with umlaut" of German keyboard
> >+    // layout is same as "period" key of US keyboard layout.  If we use
> >+    // NS_VK_PERIOD for the key, it conflicts with "period" key of German
> >+    // keyboard layout.  That causes unexpected behavior for German keyboard
> >+    // layout users.
> 
> The German keyboard is not a good example here because it is Latin, and so no
> other Latin layout will be searched.  But I guess the principal extends to
> some non-Latin layouts with ASCII punctuation.

Oh, I rewrite the comment for explaining why we don't fallback to alternative keyboard layout when current keyboard layout is ASCII alphabets inputtable.
Comment 67 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-05-15 06:26:50 PDT
Created attachment 624019 [details] [diff] [review]
part.4 Clean up modifier keycode mapping on GTK
Comment 68 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-05-15 06:35:26 PDT
(In reply to Karl Tomlinson (:karlt) from comment #64)
> Comment on attachment 621856 [details] [diff] [review]
> part.4 Clean up modifier keycode mapping on GTK
> 
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #47)
> > GDK_Meta_L/GDK_Meta_R are not mapped any DOM keycode in this patch. Should
> > be mapped to NS_VK_ALT?
> 
> The changes in part 1 here should make it unnecessary to map GDK_Meta_* to
> ALT when they correspond to the same modifier.
> 
> When GDK_Meta_* is (rarely) actually a different modifier to GTK_Alt_*, then
> it may make sense to map it to another modifier.
> 
> I see in xkb/keycodes/evdev these lines:
> 
>   // Solaris compatibility
>   alias <LMTA> = <LWIN>;
>   alias <RMTA> = <RWIN>;
> 
> So it may make sense to map GDK_Meta_* to NS_VK_WIN, but I don't mind
> whether or not that happens in this patch.  Perhaps we can let some Solaris
> people advise us later.

Hmm, okay. I think if the (physical) keys are not mapped to SUPER or HYPER too, we need to find a smart way. Otherwise, we can compute preferred modifier key from a physical keycode by GetModifierKey().
Comment 69 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-05-15 09:00:38 PDT
Comment on attachment 624019 [details] [diff] [review]
part.4 Clean up modifier keycode mapping on GTK


>+++ b/dom/interfaces/events/nsIDOMKeyEvent.idl
>@@ -205,16 +205,17 @@ interface nsIDOMKeyEvent : nsIDOMUIEvent
>   const unsigned long DOM_VK_SLASH          = 0xBF;
>   const unsigned long DOM_VK_BACK_QUOTE     = 0xC0;
>   const unsigned long DOM_VK_OPEN_BRACKET   = 0xDB; // square bracket
>   const unsigned long DOM_VK_BACK_SLASH     = 0xDC;
>   const unsigned long DOM_VK_CLOSE_BRACKET  = 0xDD; // square bracket
>   const unsigned long DOM_VK_QUOTE          = 0xDE; // Apostrophe
> 
>   const unsigned long DOM_VK_META           = 0xE0;
>+  const unsigned long DOM_VK_ALTGR          = 0xE1;
What code do we get on other platforms when altGr is pressed?
Comment 70 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-05-15 17:16:05 PDT
(In reply to Olli Pettay [:smaug] from comment #69)
> Comment on attachment 624019 [details] [diff] [review]
> part.4 Clean up modifier keycode mapping on GTK
> 
> 
> >+++ b/dom/interfaces/events/nsIDOMKeyEvent.idl
> >@@ -205,16 +205,17 @@ interface nsIDOMKeyEvent : nsIDOMUIEvent
> >   const unsigned long DOM_VK_SLASH          = 0xBF;
> >   const unsigned long DOM_VK_BACK_QUOTE     = 0xC0;
> >   const unsigned long DOM_VK_OPEN_BRACKET   = 0xDB; // square bracket
> >   const unsigned long DOM_VK_BACK_SLASH     = 0xDC;
> >   const unsigned long DOM_VK_CLOSE_BRACKET  = 0xDD; // square bracket
> >   const unsigned long DOM_VK_QUOTE          = 0xDE; // Apostrophe
> > 
> >   const unsigned long DOM_VK_META           = 0xE0;
> >+  const unsigned long DOM_VK_ALTGR          = 0xE1;
> What code do we get on other platforms when altGr is pressed?

On Windows, AltGr (typically, right-alt key) generates Ctrl keydown and Alt keydown because both keys pressed means AltGr key pressed on Windows.

On Mac, AltGr key doesn't exist. Options key behaves similar, but it causes Alt key event.

AltGr key is a little bit important because it's a modifier key. Therefore, I think we should add a new keycode for it.
Comment 71 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-05-16 00:42:00 PDT
Comment on attachment 624019 [details] [diff] [review]
part.4 Clean up modifier keycode mapping on GTK

At least this is better than the current 0 keyCode, 0 charCode for altGr
Comment 72 Karl Tomlinson (:karlt) 2012-05-16 02:52:43 PDT
Comment on attachment 624017 [details] [diff] [review]
part.1 Compute DOM keycode for non-printable key from table first

>+    guint keyvalWithoutModifier = GetGDKKeyvalWithoutModifier(aGdkKeyEvent);

I was thinking that this variable would get used in later patches, but that
didn't end up happening.  I expect gdk_keymap_translate_keyboard_state is
quite a complex function that we don't want to call more than necessary.  So I
suggest moving this variable to within ...

>+    // If the keyval indicates it's a modifier key, we should use unshifted
>+    // key's modifier keyval.
>+    if (GetModifierForGDKKeyval(keyval)) {

... this block ...

>+    // If the key isn't printable, let's look at the key pairs.
>+    PRUint32 charCode = GetCharCodeFor(aGdkKeyEvent);
>+    if (!charCode) {
>+        // Always use unshifted keycode for the non-printable key.
>+        // XXX It might be better to decide DOM keycode from all keyvals of
>+        //     the hardware keycode.  However, I think that it's too excessive.
>+        PRUint32 DOMKeyCode = GetDOMKeyCodeFromKeyPairs(keyvalWithoutModifier);

and adding another call here.

(In reply to Karl Tomlinson (:karlt) from comment #62)
> Removing the keypair logic from below, and perhaps adding this block, belong
> in part 3, because, at this point in the series, even US unshifted
> keyvals won't generate keyCodes.

What I mean here is that I'd like to maintain existing functionality through
each patch in this series.  That can be done by leaving at call to
GetDOMKeyCodeFromKeyPairs at the end of ComputeDOMKeyCode in this patch, and
removing that call in part 3.  The code at the end of the series is still the same, but, with only part 1, it will work at least as well as now.
Comment 73 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-05-16 19:33:44 PDT
Created attachment 624629 [details] [diff] [review]
part.1 Compute DOM keycode for non-printable key from table first
Comment 74 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-05-16 19:34:17 PDT
Created attachment 624630 [details] [diff] [review]
part.2 DOM keycode for printable keys in Numpad should be computed by switch

updated for part.1
Comment 75 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-05-16 19:34:53 PDT
Created attachment 624631 [details] [diff] [review]
part.3 Use unshifted char of printable keys for computing keycode on GTK

updated for part.1

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