We should implement KeyboardEvent.key only for non-printable keys. That makes the patches simpler and may help some bugs.

Hmm, I have no idea to implement this on MetroWidget. MetroInput doesn't use KeyboardLayout.cpp. So, a lot of work are needed...

Comment on attachment 717721 [details] [diff] [review] part.1 Implement D3E KeyboardEvent.key Let's implement D3E KeyboardEvent.key for non-printable keys. Internally, the key names are indicated by an enum named mozilla::widget::KeyNameIndex. nsKeyEvent::GetDOMKeyName() converts from the index to name. These patches use KEY_NAME_INDEX_PrintableKey for printable key such as 'A' key. nsDOMKeyEvent::GetKey() returns "MozPrintableKey" for it. I think that no web applications will handle this key name because it has "Moz" prefix and it's different behavior from current D3E draft. For setting dead key name, the conversion method which converts from input char to key name is implemented in WidgetUtils. It will be used on Windows and Mac. nsDOMKeyNameList.h includes all key names which are defined in D3E spec.

Comment on attachment 717723 [details] [diff] [review] part.2 Implement D3E KeyboardEvent.key on Windows (Desktop) I'd like you to review only KeyboardLayout::ConvertNativeKeyCodeToKeyNameIndex(). I think that this doesn't cause any objection. FYI: compare with IE: https://developer.mozilla.org/en-US/docs/DOM/KeyboardEvent#Key_names_and_Char_values

Comment on attachment 717724 [details] [diff] [review] part.3 Implement D3E KeyboardEvent.key on Windows (Metro) The mapping is same as previous patch.

Comment on attachment 717726 [details] [diff] [review] part.4 Implement D3E KeyboardEvent.key on Cocoa I'd like you to review TISInputSourceWrapper::ComputeGeckoKeyNameIndex(). Please be careful about F13-F15. Our keyCode values for them are for other keys which are the key name of PC's keyboard layout. However, in most cases, user must use keyboard for Mac. Then, using F13-F15 for the key name is better, I think. And even when PC keyboard is connected, Mac doesn't work the 3 keys as the key names. E.g., PrintScreen doesn't take snapshot of the screen.

Comment on attachment 717728 [details] [diff] [review] part.5 Implement D3E KeyboardEvent.key on GTK On X, F25-F35 are also defined. And there are a lot of launching application keys. And defines 'Break', 'Redo' and 'SingleCandidate'. I'll report these additional keys to the bugzilla or w3.org. I think that the name must not be changed since they are enough simple. Some other keys which have "// TODO" comment in KeymapWrapper::ComputeDOMKeyNameIndex() should be named after some discussion with W3C. I'll file a bug for them later.

Comment on attachment 717729 [details] [diff] [review] part.6 Implement D3E KeyboardEvent.key on Android Oops, sorry I requested the review without explanation. In the latest Android SDK, some new keycode values are defined. They are defined for JIS keyboard layout's or Japanese Mac keyboard layout's special keys according to the name. Therefore, I added them to DOM keyCode table too. And ConvertAndroidKeyCodeToKeyNameIndex() is the main stuff of this patch. The keys which have "// TODO" should be implemented later after discussed with W3C. I filed a bug for them: https://www.w3.org/Bugs/Public/show_bug.cgi?id=21083 I'm not sure following key mapping: > + case AndroidKeyEvent::KEYCODE_HOME: return KEY_NAME_INDEX_Exit; // Perhaps The "Home" key of smartphone causes going to home screen. I think that means exiting from current application. Therefore, I mapped 'Exit' for it. But I'm still not sure if this decision is correct. > + case AndroidKeyEvent::KEYCODE_DPAD_CENTER: return KEY_NAME_INDEX_Accept; // Perhaps The key is probably the "OK" button which is surrounded by direction keys. I don't think "Enter" is good for this. In the draft's key name list, "Accept" is the best for this key, I think. > + case AndroidKeyEvent::KEYCODE_NUM: return KEY_NAME_INDEX_Unidentified; // XXX Not sure I don't understand what's this key. http://developer.android.com/reference/android/view/KeyEvent.html#KEYCODE_NUM Perhaps, Android's widget review should judge this? > + case AndroidKeyEvent::KEYCODE_NOTIFICATION: return KEY_NAME_INDEX_Unidentified; // XXX Not sure Same.

Comment on attachment 717730 [details] [diff] [review] part.7 Implement D3E KeyboardEvent.key on Qt This is almost same as GTK's patch. I confirmed that this can be built with Qt4.8.4 SDK. However, the binary always crash at showing first window. So, I've not test this patch.

Comment on attachment 717731 [details] [diff] [review] part.8 Implement D3E KeyboardEvent.key on Gonk Similar to Android's patch. The actual mapping should be reviewed by Gonk's reviewer later.

Oops, dead key event support on Mac causes new orange of test_keycodes.xul. I'll fix it before Cocoa widget reviewer's review. (I'll fix it in the test)

Note that I've not tested on Android and Gonk too. I don't have such environment.

FYI, it may take few days before I can start reviewing these (I have some large patches in my review queue).

Okay, no problem. I have some other work too :-)

https://www.w3.org/Bugs/Public/buglist.cgi?query_format=advanced&list_id=5743&short_desc=Define%20key&short_desc_type=allwordssubstr&component=DOM3%20Events&product=WebAppsWG I filed some bugs for // TODO comment in the patches. I'm updating part.1 for latest Nightly and part.5 and part.7 for fixing some bugs.

Updated for the latest m-c.

Some comments and mapping are fixed.

Sync with GTK.

Comment on attachment 718948 [details] [diff] [review] part.1 Implement D3E KeyboardEvent.key ># HG changeset patch ># User Masayuki Nakano <masayuki@d-toybox.com> ># Parent e7632ab657e5458571a01f705e17a5473dee2f00 >Bug 842927 part.1 Implement D3E KeyboardEvent.key r=, sr= > >diff --git a/content/events/public/Makefile.in b/content/events/public/Makefile.in >--- a/content/events/public/Makefile.in >+++ b/content/events/public/Makefile.in >@@ -17,16 +17,17 @@ EXPORTS = \ >+ * It must have two arguments, (aSafeKeyName, aDOMKeyName) >+ * aSafeKeyName is safe name for a part of C++ constants. aSafeKeyNAme is ... what? I don't understand the comment >+ * aDOMKeyName is an actual value. s/an/the/ > NS_IMETHODIMP >+nsDOMKeyboardEvent::GetKey(nsAString& aKeyName) >+{ >+ if (mEvent->mFlags.mIsTrusted) { This doesn't look right. Synthetic key events can be trusted or un-trusted. Why do we need any if check here? Since you're adding stuff to nsKeyEvent, you should probably hack nsDOMEvent::DuplicatePrivateData() too > #endif

So we end up having several implementations of GetDOMKeyNameIndex. We really should try to consolidate the common bits, at least on Win/Metro

Comment on attachment 717723 [details] [diff] [review] part.2 Implement D3E KeyboardEvent.key on Windows (Desktop) r- because of that.

Would it work if we had some macro list for each key index and the list would include all the platforms. NATIVE_TO_DOM_KEY(kVK_PC_Backspace, GDK_BackSpace, <and rest of the supported platforms>, KEY_NAME_INDEX_Spacebar) Then using some macro magic generate the code.

Comment on attachment 718949 [details] [diff] [review] part.5 Implement D3E KeyboardEvent.key on GTK I think we should aim for just having one list somewhere. That way it should be less likely to forget to update platform bits when adding support for new key.

(In reply to Olli Pettay [:smaug] from comment #34) > >+ * It must have two arguments, (aSafeKeyName, aDOMKeyName) > >+ * aSafeKeyName is safe name for a part of C++ constants. > aSafeKeyNAme is ... what? I don't understand the comment I meant that it's usable name for defining the C++ constants. If DOM key name has illegal character(s) for that, aSafeKeyName and aDOMKeyName must be different. So, aCPPName is better? > > NS_IMETHODIMP > >+nsDOMKeyboardEvent::GetKey(nsAString& aKeyName) > >+{ > >+ if (mEvent->mFlags.mIsTrusted) { > This doesn't look right. Synthetic key events can be trusted or un-trusted. > Why do we need any if check here? When we will support constructor of KeyboardEvent (D4E), then, web apps can initialize the .key value with any key name. So, it's impossible to use keyname index for untrused event which is created with KeyboardEvent constructor. When we support it, we need to add |nsString mKeyName| to the nsDOMKeyEvent class and it should be used only on untrusted event. So, I don't think this |if| check is wrong. > Since you're adding stuff to nsKeyEvent, you should probably hack > nsDOMEvent::DuplicatePrivateData() too Oh, really? I'll check it, sorry for the mistake.

(In reply to Olli Pettay [:smaug] from comment #35) > So we end up having several implementations of GetDOMKeyNameIndex. > We really should try to consolidate the common bits, at least on Win/Metro The Metro widget doesn't implement keyboard event dispatchers correctly. The widget should use widget::KeyboardLayout in the future (bug 843236). So, currently, the keyboard event handler is completely separated between Win-desktop and Metro. For now, we should implement them separately. (In reply to Olli Pettay [:smaug] from comment #37) > Would it work if we had some macro list for each key index and the list > would include all the > platforms. > > NATIVE_TO_DOM_KEY(kVK_PC_Backspace, GDK_BackSpace, <and rest of the > supported platforms>, KEY_NAME_INDEX_Spacebar) > > Then using some macro magic generate the code. Hmm, I don't think that it's good idea. (1) Such way needs very long lines. However, our coding rule doesn't allow that. If we breaks a definition to multiple lines, then, I guess it's not easy to read. (2) And also, although I don't include the code in the patches for now, we will need to improve some IME keys support on Windows. On Windows, OEM specific virtual keycodes have special meaning on several keyboard layout such as Japanese keyboard layout and Korean keyboard layout. In such case, we may need to map same virtual keycode to different key name index. So, we cannot implement such key values with simple table. (3) Additionally, some key events are not notified by standard key event on Windows and GTK. On Windows, some key events are notified only by WM_APPCOMMAND. So, they cannot be mapped from virtual keycode value. (4) Gonk doesn't map key name index from constants. By such reasons, the table like implementation isn't enough for all cases. And I don't find the merit of the approach because most developers don't need to check the table.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #39) > When we will support constructor of KeyboardEvent (D4E), then, web apps can > initialize the .key value with any key name. So, it's impossible to use > keyname index for untrused event which is created with KeyboardEvent > constructor. When we support it, we need to add |nsString mKeyName| to the > nsDOMKeyEvent class and it should be used only on untrusted event. > > So, I don't think this |if| check is wrong. Still don't understand the trustness check. Should you perhaps use mEventIsInternal flag?

I mean, if (!mEventIsInternal) ....

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #40) > By such reasons, the table like implementation isn't enough for all cases. > And I don't find the merit of the approach because most developers don't > need to check the table. But I'm still worried that various tables get out of sync easily. We need some way to ensure that the tables are updated correctly. Coding style rules isn't a good reason to not make code less error prone. Also, we should try to have less platform specific code, not more, IMO. For special cases we can add different macros. (In general I'm thinking something similar to nsEventNameList.h which, when it was added, simplified event management quite significantly. No need to update lots of different arrays when adding a new event.)

(In reply to Olli Pettay [:smaug] from comment #41) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #39) > > When we will support constructor of KeyboardEvent (D4E), then, web apps can > > initialize the .key value with any key name. So, it's impossible to use > > keyname index for untrused event which is created with KeyboardEvent > > constructor. When we support it, we need to add |nsString mKeyName| to the > > nsDOMKeyEvent class and it should be used only on untrusted event. > > > > So, I don't think this |if| check is wrong. > Still don't understand the trustness check. Should you perhaps use > mEventIsInternal flag? Ah, you're right!

(In reply to Olli Pettay [:smaug] from comment #43) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #40) > > By such reasons, the table like implementation isn't enough for all cases. > > And I don't find the merit of the approach because most developers don't > > need to check the table. > > But I'm still worried that various tables get out of sync easily. We need > some way to ensure that the tables are > updated correctly. Coding style rules isn't a good reason to not make code > less error prone. I agree with that but is it allowed to break the rule in the definition table file? > Also, we should try to have less platform specific code, not more, IMO. > > For special cases we can add different macros. > > (In general I'm thinking something similar to nsEventNameList.h which, when > it was added, simplified event > management quite significantly. No need to update lots of different arrays > when adding a new event.) Hmm, I have no idea of smart solution for special cases. But I'll try to think something better implementation tomorrow.

> NATIVE_TO_DOM_KEY(kVK_PC_Backspace, GDK_BackSpace, <and rest of the > > supported platforms>, KEY_NAME_INDEX_Spacebar) I assume that you think the macro is used for making switch statement. If so, when the key isn't available on the platform, then, how to do that? I assume that the macro is NATIVE_TO_DOM_KEY(aWinVK, aGDKVK,... aKeyNameIndex); And then, defined as following style: NATIVE_TO_DOM_KEY(VK_NULL /* means not availabe */, GDK_ISOLevel3_Shift, KEY_NAME_INDEX_AltGr); NATIVE_TO_DOM_KEY(VK_NULL /* means not availabe */, GDK_ISOLevel5_Shift, KEY_NAME_INDEX_AltGr); Then, on Windows, these definitions should be ignored at solving the macro. However, I have no idea how to do that.

Note that I think that we have to use switch statement for mapping native key code and DOM key name for run time performance... So, I'm not thinking that the macro is used for defining array.

Okay, I find a way to do the macro magic. I'll make it today.

Each block (separated by empty lines) is for a same DOM key name. This is easy to compare between platforms.

We need additional work for WM_APPCOMMAND and some OEM keys whose meaning depends on keyboard layout locale. It should be done in another bug.

This has a bug. When printable key which has key name is pressed, the keypress event's .key value becomes MozPrintableKey. For fixing this, I need additional work. However, I have a plan to share keyboard input handling with desktop. So, I'll fix it at that time. This is not a big problem for now.

I don't have any environment for testing this. Widget reviewer should check it if it's possible.

Same as above, I don't test this.

I cannot test this because it always crashes at launching. I confirmed that this can be built, though.

try builds: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=6f0222396f8a

I would like to test B2G Gonk part. But I wonder how do you test them?

(In reply to Shawn Huang from comment #58) > I would like to test B2G Gonk part. But I wonder how do you test them? I just confirmed the code can be built on tryserver. If you can test key events on B2G, use this testcase: attachment 630086 [details]

smaug: ping I think that B2G people need this fix for improving their media key support. So, I think that we should fix this as far as possible.

Sorry about the delay. Unfortunately reviewing this is now 4th in my todo list. So takes still some time, unless I manage to do the reviewing during some break.

Comment on attachment 724716 [details] [diff] [review] part.2 Make convertion table from native keycode to DOM key name index GDK_KP_Prioer -> GDK_KP_Prior Odd that Android AndroidKeyEvent::KEYCODE_DEL is backspace, but looks like that is the case http://source.android.com/tech/input/keyboard-devices.html Do we not get AltGraph on Windows?

Comment on attachment 724752 [details] [diff] [review] part.6 Implement D3E KeyboardEvent.key on GTK Btw, could you provide new tryserver builds. I'd like to see how this all works using Finnish keyboard, especially how å, ä and ö keys work. >+ // If the key causes inputting a white space or no character with >+ // modifiers, the key might cause inputting white space without the >+ // modifiers. Then, we can think that the key is a spacebar. >+ uint32_t ch = GetCharCodeFor(aGdkKeyEvent); >+ if ((!ch && (aGdkKeyEvent->state & >+ (GetModifierMask(CTRL) | GetModifierMask(ALT) | >+ GetModifierMask(META) | GetModifierMask(SUPER) | >+ GetModifierMask(HYPER) | GetModifierMask(LEVEL3) | >+ GetModifierMask(LEVEL5))) != 0) || >+ (ch >= 0x2000 && ch <= 0x200D) || >+ ch == 0x202F || ch == 0x3000) { These values (0x2000, 0x200D, 0x200F, 0x3000) need some explanation.

Comment on attachment 724755 [details] [diff] [review] part.7 Implement D3E KeyboardEvent.key on Android >+ // Needs to confirm the behavior. If the key switches the open state >+ // of Japanese IME (or switches input character between Hiragana and >+ // Roman numeric characters), then, it might be better to use >+ // NS_VK_KANJI which is used for Alt+Zenkaku/Hankaku key on Windows. Can't really comment on this. >+static KeyNameIndex ConvertAndroidKeyCodeToKeyNameIndex(int androidKeyCode) aAndroidKeyCode

(In reply to Olli Pettay [:smaug] from comment #62) > Do we not get AltGraph on Windows? I'm not sure. However, I have a plan to work more on IME related keys with keyboard layout locale. At that time, I'll check it too. (In reply to Olli Pettay [:smaug] from comment #64) > >+ // Needs to confirm the behavior. If the key switches the open state > >+ // of Japanese IME (or switches input character between Hiragana and > >+ // Roman numeric characters), then, it might be better to use > >+ // NS_VK_KANJI which is used for Alt+Zenkaku/Hankaku key on Windows. > Can't really comment on this. Yep, anyway, it's very rare issue for now. I'd check it later if I got such environment. I don't think we need to fix complex and rare cases at this bug.

Comment on attachment 724758 [details] [diff] [review] part.5 Implement D3E KeyboardEvent.key on Cocoa >+ // Compute the key for non-printable keys and some special printable keys. >+ aKeyEvent.mKeyNameIndex = ComputeGeckoKeyNameIndex(nativeKeyCode); >+ if (isPrintableKey && >+ aKeyEvent.mKeyNameIndex == KEY_NAME_INDEX_Unidentified) { >+ // If the key name isn't in the list and the key is a printable key but >+ // inserting no characters without control key nor command key, then, >+ // check if the key is dead key. >+ if (insertString.IsEmpty() && >+ !aKeyEvent.IsControl() && !aKeyEvent.IsMeta()) { >+ UInt32 state = >+ nsCocoaUtils::ConvertToCarbonModifier([aNativeKeyEvent modifierFlags]); >+ uint32_t ch = TranslateToChar(nativeKeyCode, state, kbType); >+ if (ch) { >+ aKeyEvent.mKeyNameIndex = >+ WidgetUtils::GetDeadKeyNameIndex(static_cast<PRUnichar>(ch)); >+ } >+ } >+ // If the printable key isn't a dead key, we should set printable key name >+ // for now. >+ if (aKeyEvent.mKeyNameIndex == KEY_NAME_INDEX_Unidentified) { >+ aKeyEvent.mKeyNameIndex = KEY_NAME_INDEX_PrintableKey; >+ } >+ } Could you explain this some more. And this might require a review also from smichaud

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #65) > (In reply to Olli Pettay [:smaug] from comment #62) > > Do we not get AltGraph on Windows? > > I'm not sure. However, I have a plan to work more on IME related keys with > keyboard layout locale. At that time, I'll check it too. Er, IIRC, there is a problem. On Windows, when user presses AltGr key, ALT key message and Ctrl key message are used. So, pressing AltGr causes two key events. If we check the keyboard layout, then, the value could be changed to AltGr. Do you think the both key events' key values are AltGr?

Comment on attachment 724758 [details] [diff] [review] part.5 Implement D3E KeyboardEvent.key on Cocoa First, don't mind about the review process. I'll request additional reviews to each widget's reviewers after fix the points you mentioned. On Mac, Option (Alt) + a printable key might become dead key on (probably) all keyboard layouts. If it causes dead character, the insertString becomes empty but TranslateToChar() returns the non-combining dead character. Therefore, this code can resolve the dead key name. Additionally, if control or command (meta) key is pressed, they never become dead key. Therefore, we can skip the check in such cases.

Comment on attachment 724754 [details] [diff] [review] part.4 Implement D3E KeyboardEvent.key on Windows (Metro) Would it be useful to have something like KeyboardLayout::GetKeyIndex in Metro too? I guess that is a question to Metrofox devs too. I wish we could share as much code as possible between Window and Metro backends (well, with other backends too).

Comment on attachment 724758 [details] [diff] [review] part.5 Implement D3E KeyboardEvent.key on Cocoa I still think the change to TISInputSourceWrapper::InitKeyEvent could use better comment. rs=me, but hopefully widget peer looks that code closely

(In reply to Olli Pettay [:smaug] from comment #69) > Comment on attachment 724754 [details] [diff] [review] > part.4 Implement D3E KeyboardEvent.key on Windows (Metro) > > Would it be useful to have something like KeyboardLayout::GetKeyIndex in > Metro too? I guess that is a question to Metrofox devs too. > I wish we could share as much code as possible between > Window and Metro backends (well, with other backends too). Yep, I'm working on that, see bug 855975. Thank you for your review, smaug. I'll make tryserver build soon.

Building and testing: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=f05078451ff4

Comment on attachment 724716 [details] [diff] [review] part.2 Make convertion table from native keycode to DOM key name index I'd like to check this patch by each widget's reviewer. The D3E key name's document is here: http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#key-values-list I'd like to each reviewer check: jimm: KEY_MAP_WIN() smicuahd: KEY_MAP_COCOA() karlt: KEY_MAP_GTK() and KEY_MAP_QT() cpeterson: KEY_MAP_ANDROID() # nchen is better? If so, feel free to fwd it to him. cjones: KEY_MAP_GONK() #mwu is better? If so, feel free to fwd it to him.

Comment on attachment 724716 [details] [diff] [review] part.2 Make convertion table from native keycode to DOM key name index Sorry, I requested the review for obsoleted patch.

Comment on attachment 734615 [details] [diff] [review] part.2 Make convertion table from native keycode to DOM key name index (r=smaug) This is the latest patch. Sorry for the bug spam. See previous comment for the detail.

Comment on attachment 734616 [details] [diff] [review] part.3 Implement D3E KeyboardEvent.key on Windows (Desktop) (r=smaug) D3E needs to give special key name for dead key. This patch provides the code in its most part.

Comment on attachment 724754 [details] [diff] [review] part.4 Implement D3E KeyboardEvent.key on Windows (Metro) Just let's add key name support. We need to share the desktop's code after bug 855975.

Comment on attachment 724758 [details] [diff] [review] part.5 Implement D3E KeyboardEvent.key on Cocoa Now, Cocoa widget doesn't dispatch dead key event. It's different from other platforms. So, this patch fixes the difference and supports D3E key name for dead keys too.

Comment on attachment 724761 [details] [diff] [review] part.8 Implement D3E KeyboardEvent.key on Qt Nothing special for Qt's patch.

Comment on attachment 734617 [details] [diff] [review] part.6 Implement D3E KeyboardEvent.key on GTK (r=smaug) GDK's key code is decided with inputting character. Therefore, we need special handler for "Spacebar" key. See the comment in the patch for the detail. This helps to fix bug 479942.

Comment on attachment 734618 [details] [diff] [review] part.7 Implement D3E KeyboardEvent.key on Android (r=smaug) This also improves keyCode support for new key code values of the latest SDK.

Comment on attachment 734620 [details] [diff] [review] part.9 Implement D3E KeyboardEvent.key on Gonk (r=smaug) Nothing special about this patch. Note that I've never tested this because I don't have such environment.

Comment on attachment 734615 [details] [diff] [review] part.2 Make convertion table from native keycode to DOM key name index (r=smaug) Review of attachment 734615 [details] [diff] [review]: ----------------------------------------------------------------- Android LGTM with some questions. ::: widget/shared/NativeKeyToDOMKeyName.h @@ +371,5 @@ > + > +// Accept > +KEY_MAP_WIN (Accept, VK_ACCEPT) > +KEY_MAP_ANDROID (Accept, AndroidKeyEvent::KEYCODE_DPAD_CENTER) // Perhaps? > +KEY_MAP_GONK (Accept, AKEYCODE_DPAD_CENTER) Yes, I think KEYCODE_DPAD_CENTER is a reasonable "Accept" button. @@ +388,5 @@ > +KEY_MAP_GTK (Enter, GDK_3270_Enter) > +KEY_MAP_QT (Enter, Qt::Key_Return) > +KEY_MAP_QT (Enter, Qt::Key_Enter) > +KEY_MAP_ANDROID (Enter, AndroidKeyEvent::KEYCODE_ENTER) > +KEY_MAP_GONK (Enter, AKEYCODE_ENTER) Android has separate keycodes for the "return key" (KEYCODE_ENTER) and the numpad's "Enter" key (KEYCODE_NUMPAD_ENTER). Is the "return key" what you want here? I see that Cocoa has KEY_MAP_COCOA macros for vKB_Return, kVK_ANSI_KeypadEnter, and kVK_Powerbook_KeypadEnter. Should we also map KEYCODE_NUMPAD_ENTER to Enter? @@ +391,5 @@ > +KEY_MAP_ANDROID (Enter, AndroidKeyEvent::KEYCODE_ENTER) > +KEY_MAP_GONK (Enter, AKEYCODE_ENTER) > + > +// Find > +KEY_MAP_GTK (Find, GDK_Find) Some older Android devices have a "search" hardware button (with a picture of a magnifying glass) that generates a KEYCODE_SEARCH keycode. Is that similar to what GDK_Find means? However, we can probably ignore Find for Android without losing much functionality because few new Android devices have a "search" hardware button. @@ +454,5 @@ > +KEY_MAP_GONK (Exit, AKEYCODE_HOME) > + > +// Zoom > +KEY_MAP_WIN (Zoom, VK_ZOOM) > +KEY_MAP_QT (Zoom, Qt::Key_Zoom) Android defines KEYCODE_ZOOM_IN and KEYCODE_ZOOM_OUT. Would these keycodes be appropriate for Zoom? @@ +466,5 @@ > + > +// Spacebar > +KEY_MAP_WIN (Spacebar, VK_SPACE) > +KEY_MAP_COCOA (Spacebar, kVK_Space) > +// GTK widget assumes following keyvals comes from space bar. Additionaly, s/Additionaly/Additionally/ @@ +550,5 @@ > +KEY_MAP_GTK (Power, GDK_PowerDown) > +KEY_MAP_GTK (Power, GDK_PowerOff) // What's the difference from PowerDown? > +KEY_MAP_QT (Power, Qt::Key_PowerOff) > +KEY_MAP_ANDROID (Power, AndroidKeyEvent::KEYCODE_POWER) > +KEY_MAP_GONK (Power, AKEYCODE_POWER) Android also defines a number of media power buttons: KEYCODE_AVR_POWER (Audio/Video Receiver), KEYCODE_STB_POWER (Set-top Box), and KEYCODE_TV_POWER. @@ +964,5 @@ > +KEY_MAP_QT (FullWidth, Qt::Key_Zenkaku) > + > +// HalfWidth > +KEY_MAP_GTK (HalfWidth, GDK_Hankaku) > +KEY_MAP_QT (HalfWidth, Qt::Key_Hankaku) Android defines a KEYCODE_ZENKAKU_HANKAKU keycode for Japanese full-width/half-width key. Is that keycode appropriate for the FullWidth and HalfWidth keys? @@ +1003,5 @@ > +KEY_MAP_COCOA (KanjiMode, kVK_JIS_Kana) // Kana key opens IME > +KEY_MAP_GTK (KanjiMode, GDK_Kanji) // Typically, Alt + Hankaku/Zenkaku key > +KEY_MAP_QT (KanjiMode, Qt::Key_Kanji) > +// Assuming that KANA key of Android is the Kana key on Mac keyboard. > +KEY_MAP_ANDROID (KanjiMode, AndroidKeyEvent::KEYCODE_KANA) Is mapping KEYCODE_KANA to the KanjiMode key more appropriate than mapping it to the KanaMode key? @@ +1007,5 @@ > +KEY_MAP_ANDROID (KanjiMode, AndroidKeyEvent::KEYCODE_KANA) > + > +// Katakana > +KEY_MAP_GTK (Katakana, GDK_Katakana) > +KEY_MAP_QT (Katakana, Qt::Key_Katakana) Android defines a KEYCODE_KATAKANA_HIRAGANA. Would that be appropriate for the Katakana key? @@ +1030,5 @@ > +KEY_MAP_GTK (VolumeMute, GDK_AudioMute) > +KEY_MAP_QT (VolumeMute, Qt::Key_VolumeMute) > +KEY_MAP_ANDROID (VolumeMute, AndroidKeyEvent::KEYCODE_MUTE) > +KEY_MAP_ANDROID (VolumeMute, AndroidKeyEvent::KEYCODE_VOLUME_MUTE) > +KEY_MAP_GONK (VolumeMute, AKEYCODE_MUTE) Android's KEYCODE_MUTE button mutes microphone recording, not speaker volume.

Comment on attachment 734618 [details] [diff] [review] part.7 Implement D3E KeyboardEvent.key on Android (r=smaug) Review of attachment 734618 [details] [diff] [review]: ----------------------------------------------------------------- LGTM ::: widget/android/nsWindow.cpp @@ +1530,5 @@ > + case AndroidKeyEvent::KEYCODE_NUMPAD_5: > + case AndroidKeyEvent::KEYCODE_NUMPAD_6: > + case AndroidKeyEvent::KEYCODE_NUMPAD_7: > + case AndroidKeyEvent::KEYCODE_NUMPAD_8: > + case AndroidKeyEvent::KEYCODE_NUMPAD_9:; Did you mean to include a semicolon after KEYCODE_NUMPAD_9?

Comment on attachment 724242 [details] [diff] [review] part.1 Implement D3E KeyboardEvent.key Review of attachment 724242 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/nsGUIEvent.h @@ +1065,5 @@ > bool isChar; > + // DOM KeyboardEvent.key > + mozilla::widget::KeyNameIndex mKeyNameIndex; > + > + inline void GetDOMKeyName(nsAString& aKeyName) btw, member functions defined within a C++ class declaration are implicitly inline. You don't need to label them with `inline`. @@ +1071,5 @@ > + GetDOMKeyName(mKeyNameIndex, aKeyName); > + } > + > + static void GetDOMKeyName(mozilla::widget::KeyNameIndex aKeyNameIndex, > + nsAString& aKeyName) Even static member functions defined within a C++ class declaration are implicitly inline. This GetDOMKeyName() function #includes "nsDOMKeyNameList.h" into a switch statement, so the entire nsDOMKeyNameList.h header file will be inlined into every file that calls nsKeyEvent::GetDOMKeyName().

(In reply to Chris Peterson (:cpeterson) from comment #88) > @@ +388,5 @@ > > +KEY_MAP_GTK (Enter, GDK_3270_Enter) > > +KEY_MAP_QT (Enter, Qt::Key_Return) > > +KEY_MAP_QT (Enter, Qt::Key_Enter) > > +KEY_MAP_ANDROID (Enter, AndroidKeyEvent::KEYCODE_ENTER) > > +KEY_MAP_GONK (Enter, AKEYCODE_ENTER) > > Android has separate keycodes for the "return key" (KEYCODE_ENTER) and the > numpad's "Enter" key (KEYCODE_NUMPAD_ENTER). Is the "return key" what you > want here? I see that Cocoa has KEY_MAP_COCOA macros for vKB_Return, > kVK_ANSI_KeypadEnter, and kVK_Powerbook_KeypadEnter. Should we also map > KEYCODE_NUMPAD_ENTER to Enter? Yes, it should be mapped here too. I'll add it before landing. > > +// Find > > +KEY_MAP_GTK (Find, GDK_Find) > > Some older Android devices have a "search" hardware button (with a picture > of a magnifying glass) that generates a KEYCODE_SEARCH keycode. Is that > similar to what GDK_Find means? However, we can probably ignore Find for > Android without losing much functionality because few new Android devices > have a "search" hardware button. Thank you for the information. Indeed, it's not exactly same as GDK_Find. GDK_Find should cause "Find in this page". So, Find toolbar should be appear on the desktop if web applications doesn't consume the event. Does the key can be consumed by application (like Firefox)? If so, it should be mapped as the patch. Otherwise, it might be better to be removed from the patch. But I agree with you, it doesn't cause anything, actually. > > @@ +454,5 @@ > > +KEY_MAP_GONK (Exit, AKEYCODE_HOME) > > + > > +// Zoom > > +KEY_MAP_WIN (Zoom, VK_ZOOM) > > +KEY_MAP_QT (Zoom, Qt::Key_Zoom) > > Android defines KEYCODE_ZOOM_IN and KEYCODE_ZOOM_OUT. Would these keycodes > be appropriate for Zoom? No, I'm suggesting ZoomIn and ZoomOut key to W3C: https://www.w3.org/Bugs/Public/show_bug.cgi?id=21119 If it'll be granted, I'll file a bug for it. > @@ +550,5 @@ > > +KEY_MAP_GTK (Power, GDK_PowerDown) > > +KEY_MAP_GTK (Power, GDK_PowerOff) // What's the difference from PowerDown? > > +KEY_MAP_QT (Power, Qt::Key_PowerOff) > > +KEY_MAP_ANDROID (Power, AndroidKeyEvent::KEYCODE_POWER) > > +KEY_MAP_GONK (Power, AKEYCODE_POWER) > > Android also defines a number of media power buttons: KEYCODE_AVR_POWER > (Audio/Video Receiver), KEYCODE_STB_POWER (Set-top Box), and > KEYCODE_TV_POWER. Yep, see https://www.w3.org/Bugs/Public/show_bug.cgi?id=21120 > @@ +964,5 @@ > > +KEY_MAP_QT (FullWidth, Qt::Key_Zenkaku) > > + > > +// HalfWidth > > +KEY_MAP_GTK (HalfWidth, GDK_Hankaku) > > +KEY_MAP_QT (HalfWidth, Qt::Key_Hankaku) > > Android defines a KEYCODE_ZENKAKU_HANKAKU keycode for Japanese > full-width/half-width key. Is that keycode appropriate for the FullWidth and > HalfWidth keys? It's difficult question. I'm not sure how it works actually on Android. On Windows, the key generates a keyup and a keydown event for toggling two modifier state. For example, when turning to FullWidthMode, HalfWidth key's keyup event and FullWidth key's keydown event fired. However, it might cause just toggling the width with same key code on other platforms. Then, I think that "ToggleWidth" key is needed. https://www.w3.org/Bugs/Public/show_bug.cgi?id=21139 > > +KEY_MAP_COCOA (KanjiMode, kVK_JIS_Kana) // Kana key opens IME > > +KEY_MAP_GTK (KanjiMode, GDK_Kanji) // Typically, Alt + Hankaku/Zenkaku key > > +KEY_MAP_QT (KanjiMode, Qt::Key_Kanji) > > +// Assuming that KANA key of Android is the Kana key on Mac keyboard. > > +KEY_MAP_ANDROID (KanjiMode, AndroidKeyEvent::KEYCODE_KANA) > > Is mapping KEYCODE_KANA to the KanjiMode key more appropriate than mapping > it to the KanaMode key? No. It's very very complicated issue for non-Japanese people. Kana-mode should be input only Kana. However, Kanji-mode is, user can input Kana characters *and* convert them to Kanji character(s). Therefore, Mac JIS keyboard's Kana key should be "Kanji-mode" according to the actual behavior. Then, Android should generate same key value with Mac. Additionally, "Kana-mode" is hardly used actually. Most Japanese users don't use that in normal use cases. So, the key must not be actual "Kana-mode" key on Android. > @@ +1007,5 @@ > > +KEY_MAP_ANDROID (KanjiMode, AndroidKeyEvent::KEYCODE_KANA) > > + > > +// Katakana > > +KEY_MAP_GTK (Katakana, GDK_Katakana) > > +KEY_MAP_QT (Katakana, Qt::Key_Katakana) > > Android defines a KEYCODE_KATAKANA_HIRAGANA. Would that be appropriate for > the Katakana key? Similar to toggling width key, I'm still trying to find the best approach. And I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=21139 > @@ +1030,5 @@ > > +KEY_MAP_GTK (VolumeMute, GDK_AudioMute) > > +KEY_MAP_QT (VolumeMute, Qt::Key_VolumeMute) > > +KEY_MAP_ANDROID (VolumeMute, AndroidKeyEvent::KEYCODE_MUTE) > > +KEY_MAP_ANDROID (VolumeMute, AndroidKeyEvent::KEYCODE_VOLUME_MUTE) > > +KEY_MAP_GONK (VolumeMute, AKEYCODE_MUTE) > > Android's KEYCODE_MUTE button mutes microphone recording, not speaker volume. Oh, thank you for the information. I'll remove it from the patch. Thank you for your great check!!!

(In reply to Chris Peterson (:cpeterson) from comment #89) > Comment on attachment 734618 [details] [diff] [review] > part.7 Implement D3E KeyboardEvent.key on Android (r=smaug) > > Review of attachment 734618 [details] [diff] [review]: > ----------------------------------------------------------------- > > LGTM > > ::: widget/android/nsWindow.cpp > @@ +1530,5 @@ > > + case AndroidKeyEvent::KEYCODE_NUMPAD_5: > > + case AndroidKeyEvent::KEYCODE_NUMPAD_6: > > + case AndroidKeyEvent::KEYCODE_NUMPAD_7: > > + case AndroidKeyEvent::KEYCODE_NUMPAD_8: > > + case AndroidKeyEvent::KEYCODE_NUMPAD_9:; > > Did you mean to include a semicolon after KEYCODE_NUMPAD_9? Of course, no. Thank you. I have no environment for testing Android's patch. (In reply to Chris Peterson (:cpeterson) from comment #90) > > + inline void GetDOMKeyName(nsAString& aKeyName) > > btw, member functions defined within a C++ class declaration are implicitly > inline. You don't need to label them with `inline`. Thank you, I'll remove it. > @@ +1071,5 @@ > > + GetDOMKeyName(mKeyNameIndex, aKeyName); > > + } > > + > > + static void GetDOMKeyName(mozilla::widget::KeyNameIndex aKeyNameIndex, > > + nsAString& aKeyName) > > Even static member functions defined within a C++ class declaration are > implicitly inline. This GetDOMKeyName() function #includes > "nsDOMKeyNameList.h" into a switch statement, so the entire > nsDOMKeyNameList.h header file will be inlined into every file that calls > nsKeyEvent::GetDOMKeyName(). I think that it doesn't cause any problem since the method is called only a few points. And in such case, switch statement's performance is necessary.

Comment on attachment 734615 [details] [diff] [review] part.2 Make convertion table from native keycode to DOM key name index (r=smaug) I haven't really looked at KEY_MAP_QT() much as the Qt port is not maintained AFAIK. A reasonable effort is more than good enough here. KEY_MAP_GTK (Enter, GDK_Linefeed) KEY_MAP_GTK (Enter, GDK_Return) KEY_MAP_GTK (Enter, GDK_KP_Enter) KEY_MAP_GTK (Enter, GDK_ISO_Enter) KEY_MAP_GTK (Enter, GDK_3270_Enter) I expect linefeed is quite different from return/enter, but I don't see anything in the DOM3 key value set for linefeed. If linefeed were to serve as a function key then 2.2 of http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#key-algorithm seems to indicate that "a key value must be devised". However, there is actually a Unicode point for linefeed, so perhaps linefeed could be treated as a key that generates a character in 1.2. But I doubt many keyboards have linefeed anyway, so I don't think it matters here. KEY_MAP_GTK (Spacebar, GDK_space) KEY_MAP_GTK (Spacebar, GDK_KP_Space) KEY_MAP_GTK (Spacebar, GDK_nobreakspace) KEY_MAP_GTK (Spacebar, GDK_emspace) KEY_MAP_GTK (Spacebar, GDK_enspace) KEY_MAP_GTK (Spacebar, GDK_em3space) KEY_MAP_GTK (Spacebar, GDK_em4space) KEY_MAP_GTK (Spacebar, GDK_digitspace) KEY_MAP_GTK (Spacebar, GDK_punctspace) KEY_MAP_GTK (Spacebar, GDK_thinspace) KEY_MAP_GTK (Spacebar, GDK_hairspace) These are all printable keysyms. I don't know whether or not that matters, but it doesn't seem right to map them all to the same key value. Many, if not all, of these would have a Unicode code point so doesn't 1.2 of "6.2.6 Guidelines for selecting and defining key values" mean that the Unicode point should be used? KEY_MAP_GTK (Power, GDK_PowerDown) KEY_MAP_GTK (Power, GDK_PowerOff) // What's the difference from PowerDown? XF86keysym.h says PowerDown is to "Deep sleep the system" while PowerOff is to "Power off system entirely". It looks like Suspend and Hibernate were added to give different names but probably usually correspond to Sleep and PowerDown. http://cgit.freedesktop.org/xorg/proto/xproto/commit/XF86keysym.h?id=1e33337d4dd151da4f0898a86608a1ee67588163 KEY_MAP_GTK (PrintScreen, GDK_3270_PrintScreen) GDK_Print is what is used for the symbol often on the same key as SysRq. I expect that should map to PrintScreen. (I don't care whether 3270_PrintScreen also maps or not.) KEY_MAP_GTK (BrowserRefresh, GDK_Refresh) KEY_MAP_GTK (BrowserRefresh, GDK_Reload) // perhaps? #define XF86XK_Reload 0x1008FF73 /* Reload web page, file, etc. */ #define XF86XK_Refresh 0x1008FF29 /* Refresh the page */ so yes, these seem very similar. I think it is correct to have Reload here. No keyboard maps seem to use Refresh. // AltGraph KEY_MAP_GTK (AltGraph, GDK_Mode_switch /* same as GDK_kana_switch, GDK_ISO_Group_Shift and GDK_script_switch */) // Let's care both Level 3 shift and Level 5 shift as AltGr. // And also, let's care Latch key and Lock key as AltGr key too like // GDK_Shift_Lock. KEY_MAP_GTK (AltGraph, GDK_ISO_Level3_Shift) KEY_MAP_GTK (AltGraph, GDK_ISO_Level3_Latch) KEY_MAP_GTK (AltGraph, GDK_ISO_Level3_Lock) KEY_MAP_GTK (AltGraph, GDK_ISO_Level5_Shift) KEY_MAP_GTK (AltGraph, GDK_ISO_Level5_Latch) KEY_MAP_GTK (AltGraph, GDK_ISO_Level5_Lock) KEY_MAP_QT (AltGraph, Qt::Key_AltGr) KEY_MAP_QT (AltGraph, Qt::Key_Mode_switch) "Let's *treat* Latch key and Lock key as" I wonder whether "a key value must be devised" for GDK_ISO_Level5_* because this is different from level3. KEY_MAP_GTK (Shift, GDK_Shift_Lock) // Let's care as Shift key (bug 769159) "Let's *treat*" // FastFwd GDK_KEY_AudioForward I may not be able to look at part 6 until late next week.

romaxa should perhaps look at the Qt part.

(In reply to Karl Tomlinson (:karlt) from comment #93) > Comment on attachment 734615 [details] [diff] [review] > part.2 Make convertion table from native keycode to DOM key name index > (r=smaug) > > I haven't really looked at KEY_MAP_QT() much as the Qt port is not > maintained AFAIK. A reasonable effort is more than good enough here. Yeah, I could build with qt, however, I couldn't launch the build without crash :-( Anyway, the most Qt's keycode values are same as GDK. Therefore, I want you to check it same time. Sorry for increasing your work. > KEY_MAP_GTK (Enter, GDK_Linefeed) > KEY_MAP_GTK (Enter, GDK_Return) > KEY_MAP_GTK (Enter, GDK_KP_Enter) > KEY_MAP_GTK (Enter, GDK_ISO_Enter) > KEY_MAP_GTK (Enter, GDK_3270_Enter) > > I expect linefeed is quite different from return/enter, but I don't see > anything in the DOM3 key value set for linefeed. If linefeed were to serve > as a function key then 2.2 of > http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#key- > algorithm > seems to indicate that "a key value must be devised". > However, there is actually a Unicode point for linefeed, so perhaps linefeed > could be treated as a key that generates a character in 1.2. > But I doubt many keyboards have linefeed anyway, so I don't think it matters > here. > > KEY_MAP_GTK (Spacebar, GDK_space) > KEY_MAP_GTK (Spacebar, GDK_KP_Space) > KEY_MAP_GTK (Spacebar, GDK_nobreakspace) > KEY_MAP_GTK (Spacebar, GDK_emspace) > KEY_MAP_GTK (Spacebar, GDK_enspace) > KEY_MAP_GTK (Spacebar, GDK_em3space) > KEY_MAP_GTK (Spacebar, GDK_em4space) > KEY_MAP_GTK (Spacebar, GDK_digitspace) > KEY_MAP_GTK (Spacebar, GDK_punctspace) > KEY_MAP_GTK (Spacebar, GDK_thinspace) > KEY_MAP_GTK (Spacebar, GDK_hairspace) > > These are all printable keysyms. I don't know whether or not that matters, > but it doesn't seem right to map them all to the same key value. Many, if > not > all, of these would have a Unicode code point so doesn't 1.2 of "6.2.6 > Guidelines for selecting and defining key values" mean that the Unicode point > should be used? Enter, Spacebar and operator keys on NumPad are special case (). They have their own key name. So, even if they cause inputting printable characters, the .key value doesn't become same character as inputting character. So, these mapping *assumes* the characters are inputted with the key. Unfortunately, we cannot know the physical key label at least for now. Therefore, anyway, we need to *guess* which physical key caused such character. I believe that in most cases, space like character comes only from Spacebar key. Similarly, I also believe that CR or LF comes from Enter key. Therefore, I used rule 1.1 of 6.2.6. Anyway, the main scenario of usage of .key is physical key event handling. On the other hand, .char is for checking the inputting character. So, the key value, Enter or Spacebar, is useful than unicode point for web applications. If we will have some reports which reports such characters are inputted with neither Enter nor space key. Then, I think that we should try to look for stricter way. So, I think that my assumption is enough at least for now. > KEY_MAP_GTK (Power, GDK_PowerDown) > KEY_MAP_GTK (Power, GDK_PowerOff) // What's the difference from > PowerDown? > > XF86keysym.h says PowerDown is to "Deep sleep the system" while PowerOff is > to > "Power off system entirely". > > It looks like Suspend and Hibernate were added to give different names but > probably usually correspond to Sleep and PowerDown. > http://cgit.freedesktop.org/xorg/proto/xproto/commit/XF86keysym. > h?id=1e33337d4dd151da4f0898a86608a1ee67588163 Oh, thank you for the information. Then, I'll remove the GDK_PowerDown from here. It should be "Sleep" if it's defined by the spec. https://www.w3.org/Bugs/Public/show_bug.cgi?id=21118 > KEY_MAP_GTK (PrintScreen, GDK_3270_PrintScreen) > > GDK_Print is what is used for the symbol often on the same key as SysRq. > I expect that should map to PrintScreen. (I don't care whether > 3270_PrintScreen also maps or not.) Hmm, I filed a spec bug for this: https://www.w3.org/Bugs/Public/show_bug.cgi?id=21117 Do you think that we should cancel it? > // AltGraph > KEY_MAP_GTK (AltGraph, GDK_Mode_switch /* same as GDK_kana_switch, > GDK_ISO_Group_Shift and > GDK_script_switch */) > // Let's care both Level 3 shift and Level 5 shift as AltGr. > // And also, let's care Latch key and Lock key as AltGr key too like > // GDK_Shift_Lock. > KEY_MAP_GTK (AltGraph, GDK_ISO_Level3_Shift) > KEY_MAP_GTK (AltGraph, GDK_ISO_Level3_Latch) > KEY_MAP_GTK (AltGraph, GDK_ISO_Level3_Lock) > KEY_MAP_GTK (AltGraph, GDK_ISO_Level5_Shift) > KEY_MAP_GTK (AltGraph, GDK_ISO_Level5_Latch) > KEY_MAP_GTK (AltGraph, GDK_ISO_Level5_Lock) > KEY_MAP_QT (AltGraph, Qt::Key_AltGr) > KEY_MAP_QT (AltGraph, Qt::Key_Mode_switch) > > "Let's *treat* Latch key and Lock key as" > > I wonder whether "a key value must be devised" for GDK_ISO_Level5_* because > this is different from level3. Indeed, hmm, should we suggest new modifier key name for ISO_Level5_* to W3C? > I may not be able to look at part 6 until late next week. Okay, I see, no problem.

(In reply to Olli Pettay [:smaug] from comment #94) > romaxa should perhaps look at the Qt part. Okay, I'll request review to romaxa after I modify the part.2 patch with the review result of karlt.

Comment on attachment 724761 [details] [diff] [review] part.8 Implement D3E KeyboardEvent.key on Qt I'll request the review for this to romaxa.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #95) > > KEY_MAP_GTK (PrintScreen, GDK_3270_PrintScreen) > > > > GDK_Print is what is used for the symbol often on the same key as SysRq. > > I expect that should map to PrintScreen. (I don't care whether > > 3270_PrintScreen also maps or not.) > > Hmm, I filed a spec bug for this: > https://www.w3.org/Bugs/Public/show_bug.cgi?id=21117 > Do you think that we should cancel it? While it is possible to configure SysRq on a different key from Print, the only pre-configured layout in xkeyboard-config where that is the case is a type6 US sun keyboard, but there the keysym is SunXK_Sys_Req instead of the normal XK_Sys_Req. I don't know why the distinction is there or even what the key might normally do, so I don't know how important this is. Similarly there are some exceptions where Break is not on the Pause key, including a jp NEC PC98 layout, but I'm not expecting this to be important.

Oh, okay. Then, I'll change them tomorrow. And you don't need to worry about the PC98 layout. PC98 used its original connector (not PS/2 or USB). Additionally, Win2k is the last version which supports PC98. So, even Linux, modern OS which we support cannot run on PC98 due to not enough power. So, I believe nobody can use PC98 keyboard with our product.

Android and GTK2 part are already reviewed (Thanks!). romaxa: Could you review KEY_MAP_QT() for Qt? The DOM3 key name definition is here: http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#key-values-list

Comment on attachment 735698 [details] [diff] [review] part.8 Implement D3E KeyboardEvent.key on Qt (r=smaug) Review of attachment 735698 [details] [diff] [review]: ----------------------------------------------------------------- Quick check and build test, looks good so far

Comment on attachment 735694 [details] [diff] [review] part.2 Make convertion table from native keycode to DOM key name index (r=smaug+cpeterson+karlt) Review of attachment 735694 [details] [diff] [review]: ----------------------------------------------------------------- Quick check and build test, looks good so far

Comment on attachment 735694 [details] [diff] [review] part.2 Make convertion table from native keycode to DOM key name index (r=smaug+cpeterson+karlt) The KEY_MAP_COCOA parts look fine to me.

Comment on attachment 724754 [details] [diff] [review] part.4 Implement D3E KeyboardEvent.key on Windows (Metro) Review of attachment 724754 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/winrt/MetroInput.cpp @@ +1560,5 @@ > return sVirtualKeyMap[aKey]; > } > > +KeyNameIndex > +MetroInput::GetDOMKeyNameIndex(uint32_t aVirtualKey) Please add a comment header to this explaining what it's doing.

Comment on attachment 735694 [details] [diff] [review] part.2 Make convertion table from native keycode to DOM key name index (r=smaug+cpeterson+karlt) Review of attachment 735694 [details] [diff] [review]: ----------------------------------------------------------------- Didn't see anything missing here. Nice work. ::: widget/shared/NativeKeyToDOMKeyName.h @@ +826,5 @@ > + > +// Alt > +KEY_MAP_WIN (Alt, VK_MENU) > +KEY_MAP_WIN (Alt, VK_LMENU) > +KEY_MAP_WIN (Alt, VK_RMENU) How unfortunate. Differentiating these types of keys might have been useful in gaming.

Comment on attachment 724758 [details] [diff] [review] part.5 Implement D3E KeyboardEvent.key on Cocoa This looks fine to me. I built it and briefly tested some dead keys (like Option-e and Option-u), to make sure they still work properly -- they do.

Thank you, jimm and Seven. (In reply to Jim Mathies [:jimm] from comment #109) > > +// Alt > > +KEY_MAP_WIN (Alt, VK_MENU) > > +KEY_MAP_WIN (Alt, VK_LMENU) > > +KEY_MAP_WIN (Alt, VK_RMENU) > > How unfortunate. Differentiating these types of keys might have been useful > in gaming. Web developers can use .location for distinguishing the left or right Alt key.

I'm having trouble interpreting the normative section "Guidelines for selecting and defining key values" http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#keys-Guide because of its apparent inconsistencies. It says "Consider the current function of the key (i.e., with modifiers), taking into consideration the keyboard layout mapping in use" but then the algorithm uses the "*primary* current function" (emphasis added). "primary current function" is not defined, but the first example includes a key with "'5' (unmodified) and '%' (shifted)" and says "The primary function of this key is to generate the character '5', so apparently the "primary current function" should not include modifiers, or at least not the "shift" modifier. That seems to leave the effect of other modifiers and the current layout unclear, but at least some of these must have an effect or it would not be the "primary *current* function". Then 1.1 says "If there exists an appropriate character in the key values set, then" "the KeyboardEvent.key attribute must be a string consisting of the key value of that character". However, the second example is for a "dead key for the circumflex diacritical mark", corresponding to the character '\u0302'. The key value set includes 'DeadCircumflex' for this character, and so 1.1 means that "key" should be "DeadCircumflex" but the example says "the key value will be '\u0302'".

Comment on attachment 735695 [details] [diff] [review] part.6 Implement D3E KeyboardEvent.key on GTK (r=smaug) I expected to see this use GetGDKKeyvalWithoutModifier(), much like ComputeDOMKeyCode() does for non-printable keys at least. That would be consistent with what I'd interpret "primary current function" to be. It would mean that the Alt key doesn't send "Meta" when shift is down, and also (I expect) that special white space characters would no longer need to be handled specially and ... (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #95) > > KEY_MAP_GTK (Enter, GDK_Linefeed) > > KEY_MAP_GTK (Spacebar, GDK_KP_Space) > > KEY_MAP_GTK (Spacebar, GDK_nobreakspace) > > KEY_MAP_GTK (Spacebar, GDK_emspace) > > KEY_MAP_GTK (Spacebar, GDK_enspace) > > KEY_MAP_GTK (Spacebar, GDK_em3space) > > KEY_MAP_GTK (Spacebar, GDK_em4space) > > KEY_MAP_GTK (Spacebar, GDK_digitspace) > > KEY_MAP_GTK (Spacebar, GDK_punctspace) > > KEY_MAP_GTK (Spacebar, GDK_thinspace) > > KEY_MAP_GTK (Spacebar, GDK_hairspace) > So, these mapping *assumes* the characters are inputted with the key. > Unfortunately, we cannot know the physical key label at least for now. > Therefore, anyway, we need to *guess* which physical key caused such > character. > > I believe that in most cases, space like character comes only from Spacebar > key. Similarly, I also believe that CR or LF comes from Enter key. ... these assumptions and special cases would not be necessary. >+ return KEY_NAME_INDEX_Unidentified; >+ >+ default: >+ // Otherwise, it could be a spacebar. >+ break; I don't want to maintain a long list of keysyms that won't be on the spacebar. Can we remove this? >// ISO level5 shift is supported on GTK3 >-#ifndef GDK_ISO_Level5_Shift >+// new keysym values which are not defined in older gdkkeysyms.h > #define GDK_ISO_Level5_Shift 0xFE11 I expect this will generate warnings on systems with this already defined. One option to consider is adding a widget/gtk2/compat/gdk/gdkkeysyms-compat.h file.

(In reply to Karl Tomlinson (:karlt) from comment #113) > It says "Consider the current function of the key (i.e., with modifiers), > taking into consideration the keyboard layout mapping in use" but then the > algorithm uses the "*primary* current function" (emphasis added). I think that it's just a spec bug. The example should say "current function" at the list. I discuss a lot of things with W3C ML about KeyboardEvent.key, .char and .code (D4E). Then, the W3C people thinks that .key's primary usage is for handling non-printable keys. They thinks printable keys should be handled by .char. Although, the .char spec is still unstable. https://www.w3.org/Bugs/Public/show_bug.cgi?id=18867 https://www.w3.org/Bugs/Public/show_bug.cgi?id=21113 According to their idea, respecting modifier state is what they expects. So, you don't mind about the keys whose meaning is changed by modifier state. > Then 1.1 says "If there exists an appropriate character in the key values > set, > then" "the KeyboardEvent.key attribute must be a string consisting of the key > value of that character". However, the second example is for a "dead key for > the circumflex diacritical mark", corresponding to the character '\u0302'. > The key value set includes 'DeadCircumflex' for this character, and so 1.1 > means that "key" should be "DeadCircumflex" but the example says "the key > value will be '\u0302'". "the KeyboardEvent.key attribute must be a string consisting of the key value of that character" must mean that UA should use one of the key names in the list which matches to the inputting character. Therefore, .key should be "DeadCircumflex" and .char should be '\u0302' at your example.

Comment on attachment 735695 [details] [diff] [review] part.6 Implement D3E KeyboardEvent.key on GTK (r=smaug) (In reply to Karl Tomlinson (:karlt) from comment #114) > I expected to see this use GetGDKKeyvalWithoutModifier(), much like > ComputeDOMKeyCode() does for non-printable keys at least. That would be > consistent with what I'd interpret "primary current function" to be. It > would > mean that the Alt key doesn't send "Meta" when shift is down, and also (I > expect) that special white space characters would no longer need to be > handled > specially and ... So, I'd like you to re-review the patch with my explanation (comment 115). > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #95) > > > KEY_MAP_GTK (Enter, GDK_Linefeed) > > > > KEY_MAP_GTK (Spacebar, GDK_KP_Space) > > > KEY_MAP_GTK (Spacebar, GDK_nobreakspace) > > > KEY_MAP_GTK (Spacebar, GDK_emspace) > > > KEY_MAP_GTK (Spacebar, GDK_enspace) > > > KEY_MAP_GTK (Spacebar, GDK_em3space) > > > KEY_MAP_GTK (Spacebar, GDK_em4space) > > > KEY_MAP_GTK (Spacebar, GDK_digitspace) > > > KEY_MAP_GTK (Spacebar, GDK_punctspace) > > > KEY_MAP_GTK (Spacebar, GDK_thinspace) > > > KEY_MAP_GTK (Spacebar, GDK_hairspace) > > > So, these mapping *assumes* the characters are inputted with the key. > > Unfortunately, we cannot know the physical key label at least for now. > > Therefore, anyway, we need to *guess* which physical key caused such > > character. > > > > I believe that in most cases, space like character comes only from Spacebar > > key. Similarly, I also believe that CR or LF comes from Enter key. > > ... these assumptions and special cases would not be necessary. Hmm, okay, anyway, they should be handled at the end of ComputeDOMKeyNameIndex(). > >+ return KEY_NAME_INDEX_Unidentified; > >+ > >+ default: > >+ // Otherwise, it could be a spacebar. > >+ break; > > I don't want to maintain a long list of keysyms that won't be on the > spacebar. > Can we remove this? Yes, I like they stay there, but it's not matter for me.

Comment on attachment 735695 [details] [diff] [review] part.6 Implement D3E KeyboardEvent.key on GTK (r=smaug) (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #115) > According to their idea, respecting modifier state is what they expects. OK. That makes sense thanks. I misunderstood the intention of the "key" attribute. I was confused by the name of the attribute ("key"), the use of virtual key codes in the WINNT and MacOS ports, the word "primary" (which I still don't understand) used in the spec with "current function", the mapping of multiple keysyms to Spacebar, and the mention of physical keys in comment 95. I now follow that virtual key codes were used for non-printable keys merely because that works on those systems. However, with this interpretation it is now clear that the key value "Spacebar" should be used only if the "current function" of the key is to generate the U+0020 character. It seems unfortunate to name a key value "Spacebar" because that implies a virtual key code rather than a key value, but if the current function of the key is to generate other printable characters then "Spacebar" is not the appropriate key value. That makes the special case handling unnecessary. It seems that you are trying to use the "key" attribute to solve bug 479942 but the "key" attribute is not intended to help that situation. Perhaps the "code" attribute, or perhaps even the existing keyCode, would be more useful for bug 479942.

(In reply to Karl Tomlinson (:karlt) from comment #117) > However, with this interpretation it is now clear that the key value > "Spacebar" should be used only if the "current function" of the key is to > generate the U+0020 character. It seems unfortunate to name a key value > "Spacebar" because that implies a virtual key code rather than a key value, > but if the current function of the key is to generate other printable > characters then "Spacebar" is not the appropriate key value. Hmm, I don't think so. KeyboardEvent.key was introduced by IE 9 or earlier. I tested the behavior with IE 10, then, even if the space bar causes non-breakable space (U+00A0), the .key value is "Spacebar". I think that the .key value on IE doesn't depend on the character inputted by space key because IE probably maps the virtual keycode VK_SPACE to "Spacebar". The patches for Windows and Mac, they don't depend on the input character since mapped with virtual keycode rather than actual input character. So, I believe that even for compatibility with other platform's Gecko, GTK should use "Spacebar" for the key which inputs U+0020 or U+00A0 without modifiers. > That makes the special case handling unnecessary. > > It seems that you are trying to use the "key" attribute to solve bug 479942 > but the "key" attribute is not intended to help that situation. Perhaps the > "code" attribute, or perhaps even the existing keyCode, would be more useful > for bug 479942. Absolutely. Additionally, with your idea, web application developers are not happy if they want to handle (Ctrl or Shift +) Spacebar as a shortcut key like us. If you don't like the special handling with this explanation, I'll remove it. And I'll post the question to W3C. Then, I'll make consistent behavior on all platforms. # I'm creating new patches now.

Please chose better patch with my previous explanation.

Oops, sorry for the spam.

Comment on attachment 738932 [details] [diff] [review] part.6 Implement D3E KeyboardEvent.key on GTK (r=smaug) (special handling for Spacebar removed) I'm happy with ComputeDOMKeyNameIndex() as one interpretation of the DOM3 key value algorithm, and it is at least a start that we can improve on. However, even compat/gdk/gdkkeysyms.h needs to test whether the each macro is already defined so as not to produce compiler warnings by redefining it slightly differently (perhaps with different case or decimal instead of hex). r+ if you test each preprocessor symbol before defining it, like #ifndef GDK_Sleep #define GDK_Sleep 0x1008ff2f #endif

Comment on attachment 738929 [details] [diff] [review] part.6 Implement D3E KeyboardEvent.key on GTK (r=smaug) (special handling for Spacebar not removed) I would also be happy with an interpretation that makes "key" correspond to a virtual key if that is what we think it should be. The other ports using the virtual key is a good reason for the GTK port to also treat it as a virtual key. And if IE is the only browser to have implemented this and it treats "key" as a virtual key, then that would be another reason to use this interpretation. But IE doesn't use the virtual key for alphanumeric and punctuation keys, so its not clear whether the behavior for non-break space is intentional or just a quirk of implementation. What I'm not happy about with this patch is the apparently arbitrary and inconsistent treatment of the space key value. Other keys such as Alt (perhaps having Meta on another level), Tab (perhaps having ISO_Left_Tab on another level), and Print (having Sys_Req on another level) are other keys that that should be treated similarly, and perhaps there are more. If the other ports are using the virtual key code to select "key" then GetGDKKeyvalWithoutModifier() seems the closest match for the values in the key value set. Perhaps the core issue is that you want "key" to be virtual key for some physical keys but a current key value/symbol for other keys. Then the question is how do you choose/know which it will be? Perhaps if there is a corresponding key value in the key value set then "key" can be a virtual key? Is there a reason why we would need to use current key values/symbols for dead keys instead of a virtual key, given they would have a char value?

Comment on attachment 738928 [details] [diff] [review] part.2 Make convertion table from native keycode to DOM key name index (r=smaug+cpeterson+romaxa+smichaud+jimm) cjones may no longer be available for reviews, but I don't know whom to suggest, sorry. Perhaps look to see who else has been doing reviews in widget/gonk. >+// PrintScreen >+KEY_MAP_WIN (PrintScreen, VK_SNAPSHOT) >+KEY_MAP_GTK (PrintScreen, GDK_3270_PrintScreen) >+// Perhaps, "SysReq" is mapped to PrintScreen. >+KEY_MAP_GTK (PrintScreen, GDK_Sys_Req) >+KEY_MAP_QT (PrintScreen, Qt::Key_SysReq) >+KEY_MAP_ANDROID (PrintScreen, AndroidKeyEvent::KEYCODE_SYSRQ) >+KEY_MAP_GONK (PrintScreen, AKEYCODE_SYSRQ) I think GDK_Print should be here, probably instead of GDK_Sys_Req.

(In reply to Karl Tomlinson (:karlt) from comment #124) > If the other ports are using the virtual key code to select "key" then > GetGDKKeyvalWithoutModifier() seems the closest match for the values in the > key value set. Hmm, on Windows, some physical keys cause different virtual keycode values with different modifier state. So, you don't need to worry about some keys such as Alt-Meta key. For example, pause/break key causes VK_PAUSE. However, Ctrl+pause/break causes VK_CANCEL. So, they cause different .keyCode and .key values. But anyway, I'll post about Spacebar issue to W3C. The reason why I believe so is, the key is actually printable key but it's also a function key like Enter.

(In reply to Karl Tomlinson (:karlt) from comment #125) > cjones may no longer be available for reviews, but I don't know whom to > suggest, sorry. Perhaps look to see who else has been doing reviews in > widget/gonk. Hmm, I'm pinging cjones with email, but still no response. Michael Wu, do you have permission to review the patches? Or do you know good person to review them? > >+// PrintScreen > >+KEY_MAP_WIN (PrintScreen, VK_SNAPSHOT) > >+KEY_MAP_GTK (PrintScreen, GDK_3270_PrintScreen) > >+// Perhaps, "SysReq" is mapped to PrintScreen. > >+KEY_MAP_GTK (PrintScreen, GDK_Sys_Req) > >+KEY_MAP_QT (PrintScreen, Qt::Key_SysReq) > >+KEY_MAP_ANDROID (PrintScreen, AndroidKeyEvent::KEYCODE_SYSRQ) > >+KEY_MAP_GONK (PrintScreen, AKEYCODE_SYSRQ) > > I think GDK_Print should be here, probably instead of GDK_Sys_Req. I investigate it more today.

(In reply to Karl Tomlinson (:karlt) from comment #125) > >+// PrintScreen > >+KEY_MAP_WIN (PrintScreen, VK_SNAPSHOT) > >+KEY_MAP_GTK (PrintScreen, GDK_3270_PrintScreen) > >+// Perhaps, "SysReq" is mapped to PrintScreen. > >+KEY_MAP_GTK (PrintScreen, GDK_Sys_Req) > >+KEY_MAP_QT (PrintScreen, Qt::Key_SysReq) > >+KEY_MAP_ANDROID (PrintScreen, AndroidKeyEvent::KEYCODE_SYSRQ) > >+KEY_MAP_GONK (PrintScreen, AKEYCODE_SYSRQ) > > I think GDK_Print should be here, probably instead of GDK_Sys_Req. Confirmed. But for the consistency with GDK_Break and GDK_Pause, GDK_Sys_Req should be also defined, isn't it? So, both GDK_Print and GDK_Sys_Req should be there.

I can review any gonk related pieces. ( see https://wiki.mozilla.org/Modules/FirefoxOS )

Comment on attachment 734620 [details] [diff] [review] part.9 Implement D3E KeyboardEvent.key on Gonk (r=smaug) Okay, then, could you review the patches of this bug?

Comment on attachment 738928 [details] [diff] [review] part.2 Make convertion table from native keycode to DOM key name index (r=smaug+cpeterson+romaxa+smichaud+jimm) See comment 78 about the detail. I'd like you to check KEY_MAP_GONK().

Comment on attachment 738928 [details] [diff] [review] part.2 Make convertion table from native keycode to DOM key name index (r=smaug+cpeterson+romaxa+smichaud+jimm) Review of attachment 738928 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/shared/NativeKeyToDOMKeyName.h @@ +399,5 @@ > +KEY_MAP_WIN (Help, VK_HELP) > +KEY_MAP_COCOA (Help, kVK_Help) > +KEY_MAP_GTK (Help, GDK_Help) > +KEY_MAP_QT (Help, Qt::Key_Help) > +KEY_MAP_ANDROID (Help, AndroidKeyEvent::KEYCODE_ASSIST) We can add a definition for AKEYCODE_ASSIST on Gonk - we use widget/gonk/libui/android_keycodes.h, which comes from http://androidxref.com/4.2.2_r1/xref/frameworks/native/include/android/keycodes.h in Jellybean. Updating that would let us match the keycodes in Android, and you could even have just one macro for both Android and Gonk. AKEYCODE_* is suppose to always be synced with AndroidKeyEvent::KEYCODE_*.

Comment on attachment 734620 [details] [diff] [review] part.9 Implement D3E KeyboardEvent.key on Gonk (r=smaug) Review of attachment 734620 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/GonkKeyMapping.h @@ +188,5 @@ > NS_VK_EQUALS, > // There are more but we don't map them > }; > + > +inline KeyNameIndex GetKeyNameIndex(int aKeyCode) I think we can just s/inline/static/. gcc will figure it out. @@ +198,5 @@ > +#include "NativeKeyToDOMKeyName.h" > + > +#undef NS_NATIVE_KEY_TO_DOM_KEY_NAME_INDEX > + > + case AKEYCODE_0: Gonk widget code puts case on the same indentation level as switch. ::: widget/gonk/nsAppShell.cpp @@ +51,5 @@ > #include "libui/InputDispatcher.h" > > #include "GeckoProfiler.h" > > +// Defines kKeyMapping and kKeyNameMapping I don't see a kKeyNameMapping @@ +236,5 @@ > static void > maybeSendKeyEvent(int keyCode, bool pressed, uint64_t timeMs) > { > + int32_t DOMKeyCode = > + (keyCode < ArrayLength(kKeyMapping)) ? kKeyMapping[keyCode] : 0; kKeyMapping is unsigned

(In reply to Michael Wu [:mwu] from comment #132) > Comment on attachment 738928 [details] [diff] [review] > part.2 Make convertion table from native keycode to DOM key name index > (r=smaug+cpeterson+romaxa+smichaud+jimm) > > Review of attachment 738928 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/shared/NativeKeyToDOMKeyName.h > @@ +399,5 @@ > > +KEY_MAP_WIN (Help, VK_HELP) > > +KEY_MAP_COCOA (Help, kVK_Help) > > +KEY_MAP_GTK (Help, GDK_Help) > > +KEY_MAP_QT (Help, Qt::Key_Help) > > +KEY_MAP_ANDROID (Help, AndroidKeyEvent::KEYCODE_ASSIST) > > We can add a definition for AKEYCODE_ASSIST on Gonk - we use > widget/gonk/libui/android_keycodes.h, which comes from > http://androidxref.com/4.2.2_r1/xref/frameworks/native/include/android/ > keycodes.h in Jellybean. Updating that would let us match the keycodes in > Android, and you could even have just one macro for both Android and Gonk. > AKEYCODE_* is suppose to always be synced with AndroidKeyEvent::KEYCODE_*. How do I update the keycodes.h? Where is frameworks/base/core/java/android/view/KeyEvent.java?? I'd like to make the patches small as far as possible. So, I'd like Gonk team will add new key code values if they need after this bug. And even I update the header file, it's impossible to use one macro for both Android and Gonk since C++ macro cannot join AndroidKeyEvent::KEYCODE_ and specified key name (:: causes build error).

I just add new keycodes to android_keycodes.h. If I need to modify other files, let me know. But if they are a lot of files, please consider that it will be fixed in follow-up bug. And now, KEY_MAP_GONK and KEY_MAP_ANDROID are always pair.

Smaug, do you think that we should replace .keyCode and GetKeyCode() for non-printable keys in our code with .key and GetKey() (or directly accessing mKeyNameIndex)?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #134). > How do I update the keycodes.h? Where is > frameworks/base/core/java/android/view/KeyEvent.java?? I'd like to make the > patches small as far as possible. So, I'd like Gonk team will add new key > code values if they need after this bug. > They are in the upstream repos. See: https://android.googlesource.com/platform/frameworks/native/+/master/include/android/keycodes.h https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/view/KeyEvent.java We don't have a local copy of KeyEvent.java so you don't have to do anything with that. If you need new key codes to match Android, you can just copy keycodes.h from the latest version in upstream. > And even I update the header file, it's impossible to use one macro for both > Android and Gonk since C++ macro cannot join AndroidKeyEvent::KEYCODE_ and > specified key name (:: causes build error). Ok.

Thank you, mwu!

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=24d49dec3bab I'll land them after karlt answers my question (comment 128).

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #137) > Smaug, do you think that we should replace .keyCode and GetKeyCode() for > non-printable keys in our code with .key and GetKey() (or directly accessing > mKeyNameIndex)? Well, not in this bug. Maybe a followup for such bug. Hard to say whether it simplifies any code or makes it easier to maintain. Btw, you probably need to update the patch so that the .webidl file has 'key'.

Thank you for the information. I updated the patch, could you check it?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #134) > And even I update the header file, it's impossible to use one macro for both > Android and Gonk since C++ macro cannot join AndroidKeyEvent::KEYCODE_ and > specified key name (:: causes build error). masayuki, mwu: We can update our Fennec code to use AKEYCODE_ enum names instead of AndroidKeyEvent::KEYCODE_ names, if that will make your NativeKeyToDOMKeyName.h patch simpler. I have long wondered why Fennec used different names. :)

(In reply to Chris Peterson (:cpeterson) from comment #143) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #134) > > And even I update the header file, it's impossible to use one macro for both > > Android and Gonk since C++ macro cannot join AndroidKeyEvent::KEYCODE_ and > > specified key name (:: causes build error). > > masayuki, mwu: > > We can update our Fennec code to use AKEYCODE_ enum names instead of > AndroidKeyEvent::KEYCODE_ names, if that will make your > NativeKeyToDOMKeyName.h patch simpler. I have long wondered why Fennec used > different names. :) Huh, I didn't realize (or forgot) Fennec just mirrored the constants in KeyEvent.java. AKEYCODE_* names are probably the right way to go. They come from an official Android repo, are guaranteed to be synced with KeyEvent.java, and you don't have to worry about manually inserting new entries. Just copy a new file in to update.

masayuki: I just landed bug 864521, which replaces Fennec's AndroidKeyEvent::KEYCODE enum with Android's AKEYCODE enum. I hope this change will be helpful for your KEY_MAP_ANDROID and KEY_MAP_GONK macros. <:)

Yeah, thank you very much. I'm testing the updated patches on tryserver :-) https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=832f674086c6

Updated for bug 864521.

Comment on attachment 740648 [details] [diff] [review] part.2 Make convertion table from native keycode to DOM key name index (r=smaug+romaxa+smichaud+jimm+karlt+mwu) Review of attachment 740648 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/shared/NativeKeyToDOMKeyName.h @@ +37,5 @@ > +#elif defined(MOZ_WIDGET_QT) > +#undef KEY_MAP_QT > +#define KEY_MAP_QT(aCPPKeyName, aNativeKey) \ > + NS_NATIVE_KEY_TO_DOM_KEY_NAME_INDEX(aNativeKey, KEY_NAME_INDEX_##aCPPKeyName) > +#elif defined(ANDROID) || defined(MOZ_WIDGET_GONK) #elif defined(ANDROID) is sufficient here. ANDROID is defined on both Android and Gonk. MOZ_WIDGET_ANDROID and MOZ_WIDGET_GONK would be used to distinguish between the two if necessary.

Thanks for the information!

Comment on attachment 740731 [details] [diff] [review] part.2 Make convertion table from native keycode to DOM key name index (r=smaug+romaxa+smichaud+jimm+karlt) Review of attachment 740731 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! :D

Comment on attachment 740649 [details] [diff] [review] part.7 Implement D3E KeyboardEvent.key on Android (r=smaug) Review of attachment 740649 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfe45fd605f8 https://hg.mozilla.org/integration/mozilla-inbound/rev/c85f2e0fad49 https://hg.mozilla.org/integration/mozilla-inbound/rev/2c7dc9880f78 https://hg.mozilla.org/integration/mozilla-inbound/rev/e8e2a7e4ab5f https://hg.mozilla.org/integration/mozilla-inbound/rev/e4b70c31bc9e https://hg.mozilla.org/integration/mozilla-inbound/rev/fb7e97b09d0d https://hg.mozilla.org/integration/mozilla-inbound/rev/32c3d5e2fa51 https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3bdd9232fc https://hg.mozilla.org/integration/mozilla-inbound/rev/97fea0af174d karlt: If you disagree with my decision (comment 128), please file a new bug. Thank you all guys who contributed this bug!

I documented the mapping in MDN: Win: https://developer.mozilla.org/en-US/docs/DOM/KeyboardEvent#keyname_table_win Mac: https://developer.mozilla.org/en-US/docs/DOM/KeyboardEvent#keyname_table_mac GTK: https://developer.mozilla.org/en-US/docs/DOM/KeyboardEvent#keyname_table_gtk Android (Gonk): https://developer.mozilla.org/en-US/docs/DOM/KeyboardEvent#keyname_table_android

https://hg.mozilla.org/mozilla-central/rev/97fea0af174d https://hg.mozilla.org/mozilla-central/rev/fa3bdd9232fc https://hg.mozilla.org/mozilla-central/rev/32c3d5e2fa51 https://hg.mozilla.org/mozilla-central/rev/fb7e97b09d0d https://hg.mozilla.org/mozilla-central/rev/e4b70c31bc9e https://hg.mozilla.org/mozilla-central/rev/e8e2a7e4ab5f https://hg.mozilla.org/mozilla-central/rev/2c7dc9880f78 https://hg.mozilla.org/mozilla-central/rev/c85f2e0fad49 https://hg.mozilla.org/mozilla-central/rev/bfe45fd605f8

http://tbpl-dev.callek.net/php/getParsedLog.php?id=20768925&tree=SeaMonkey#error0 ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:342:6: error: ‘GDK_Mail’ was not declared in this scope In file included from ../../../../mozilla/widget/gtk2/nsGtkKeyUtils.cpp:828:0: ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:490:6: error: ‘GDK_MonBrightnessDown’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:494:6: error: ‘GDK_MonBrightnessUp’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:502:6: error: ‘GDK_Eject’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:507:6: error: ‘GDK_PowerOff’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:527:6: error: ‘GDK_HomePage’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:532:6: error: ‘GDK_Refresh’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:533:6: error: ‘GDK_Reload’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:539:6: error: ‘GDK_Search’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:545:6: error: ‘GDK_Stop’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:550:6: error: ‘GDK_Back’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:556:6: error: ‘GDK_Forward’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:660:6: error: ‘GDK_Copy’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:664:6: error: ‘GDK_Cut’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:687:6: error: ‘GDK_Paste’ was not declared in this scope In file included from ../../../../mozilla/widget/gtk2/nsGtkKeyUtils.cpp:828:0: ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:959:6: error: ‘GDK_AudioMute’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:966:6: error: ‘GDK_AudioLowerVolume’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:973:6: error: ‘GDK_AudioRaiseVolume’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:978:6: error: ‘GDK_AudioPause’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:983:6: error: ‘GDK_AudioPlay’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:989:6: error: ‘GDK_AudioStop’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:995:6: error: ‘GDK_AudioNext’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:1001:6: error: ‘GDK_AudioPrev’ was not declared in this scope In file included from ../../../../mozilla/widget/gtk2/nsGtkKeyUtils.cpp:828:0: ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:1020:6: error: ‘GDK_Blue’ was not declared in this scope In file included from ../../../../mozilla/widget/gtk2/nsGtkKeyUtils.cpp:828:0: ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:1040:6: error: ‘GDK_BrightnessAdjust’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:1050:6: error: ‘GDK_Green’ was not declared in this scope In file included from ../../../../mozilla/widget/gtk2/nsGtkKeyUtils.cpp:828:0: ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:1094:6: error: ‘GDK_AudioRandomPlay’ was not declared in this scope In file included from ../../../../mozilla/widget/gtk2/nsGtkKeyUtils.cpp:828:0: ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:1106:6: error: ‘GDK_AudioRecord’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:1113:6: error: ‘GDK_Red’ was not declared in this scope ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:1117:6: error: ‘GDK_AudioRewind’ was not declared in this scope In file included from ../../../../mozilla/widget/gtk2/nsGtkKeyUtils.cpp:828:0: ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:1141:6: error: ‘GDK_Subtitle’ was not declared in this scope In file included from ../../../../mozilla/widget/gtk2/nsGtkKeyUtils.cpp:828:0: ../../../../mozilla/widget/gtk2/../shared/NativeKeyToDOMKeyName.h:1156:6: error: ‘GDK_Yellow’ was not declared in this scope make[6]: *** [nsGtkKeyUtils.o] Error 1

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (almost offline 5/1-5/6) from comment #128) > Confirmed. But for the consistency with GDK_Break and GDK_Pause, GDK_Sys_Req > should be also defined, isn't it? So, both GDK_Print and GDK_Sys_Req should > be there. That depends on whether "key" is meant represent a physical key or one of the symbols/values generated by the key. I think we should choose one or the other and be consistent. But, if you want to mimic IE10, then I'm guessing that uses the so-called virtual key and so that would require detecting and copying its quirks. I'd prefer not to do that. If you want a physical key in some cases and a value in other cases, then I'd want a sensible rule for deciding when to do each.

(In reply to Karl Tomlinson (back Apr 29 :karlt) from comment #157) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (almost offline > 5/1-5/6) from comment #128) > > Confirmed. But for the consistency with GDK_Break and GDK_Pause, GDK_Sys_Req > > should be also defined, isn't it? So, both GDK_Print and GDK_Sys_Req should > > be there. > > That depends on whether "key" is meant represent a physical key or one of > the symbols/values generated by the key. I think we should choose one or > the other and be consistent. But, if you want to mimic IE10, then I'm > guessing that uses the so-called virtual key and so that would require > detecting and copying its quirks. I'd prefer not to do that. If you want a > physical key in some cases and a value in other cases, then I'd want a > sensible rule for deciding when to do each. The W3C stuff thinks that .key indicates the function of the key with current modifier state. So, I think that GTK can separate the keysyms by modifier state, therefore, I think that it's better to map SysRq and Break should have independent key name. According to UI Events (a.k.a D4E), KeyboardEvent.code represents the physical key identifier.