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.

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.

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.

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.

"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.

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.

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

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

Per bug 583721 (and my attempt to reproduce just now), this is in fact a problem. It could really use an owner, too...

I'm looking at it again. Dead key handling is first.

I'll post a patch for all keyboard layouts.

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 on attachment 489763 [details] [diff] [review] Patch v1.0.1 fix a nit.

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.

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 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.

(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.

(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.

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.

(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.

> (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.

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

(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.

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.

(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.

I think we should be able to use nsWindow::GetToggledKeyState() for checking whether NumLock is locked.

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.

(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.

You need to apply following patches if you want to apply this patch on current trunk: * bug 630810 * bug 630817 * bug 630813

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.

(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

(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.

> 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.

(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.

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.

Hello. Does this issue cover not fired Key press events, as in Bug 676259?

(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.

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.

This patch makes computing keycode of all printable keys in numpad use switch statement. The kKeyParis should be used only for non-printable keys.

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.

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.

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?

tryserver builds: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/masayuki@d-toybox.com-27c9738677c3/

karlt: ping

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).

(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.

oops, session restore sometimes cause unexpected status change, sorry.

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.

(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.

Updated for latest trunk. Karl, could you review these patches? I need to fix this bug before I work on bug 680830.

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.

(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.

OK. Makes sense, thanks.

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 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 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.

(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) {|

(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.

(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 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?

(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 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 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.

updated for part.1

updated for part.1

https://hg.mozilla.org/integration/mozilla-inbound/rev/69fc5803d184 https://hg.mozilla.org/integration/mozilla-inbound/rev/fe13d998d074 https://hg.mozilla.org/integration/mozilla-inbound/rev/d94270b8449f https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ce0e49040a

http://hg.mozilla.org/mozilla-central/rev/69fc5803d184 http://hg.mozilla.org/mozilla-central/rev/fe13d998d074 http://hg.mozilla.org/mozilla-central/rev/d94270b8449f http://hg.mozilla.org/mozilla-central/rev/c9ce0e49040a