Closed Bug 865649 Opened 11 years ago Closed 10 years ago

Implement KeyboardEvent.code (only for physical keyboard)

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mounir, Assigned: masayuki)

References

(Depends on 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 21 obsolete files)

28.91 KB, patch
smaug
: review+
Details | Diff | Splinter Review
16.27 KB, patch
jimm
: review+
Details | Diff | Splinter Review
19.67 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
30.90 KB, patch
karlt
: review+
romaxa
: review+
Details | Diff | Splinter Review
42.36 KB, patch
jchen
: review+
mwu
: review+
Details | Diff | Splinter Review
Basically, it would work like .key but without applying the modifiers, see $URL.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24740
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24739

These spec bugs should be fixed before I work on this.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Some keys are supported only with virtual keycodes, without scancode...
Attachment #8401781 - Attachment is obsolete: true
Attachment #8401780 - Attachment is obsolete: true
Attachment #8405187 - Attachment is obsolete: true
Attachment #8406062 - Attachment is obsolete: true
Attachment #8408170 - Attachment description: part.5 Set KeyboardEvent.code value on Android and Firefox OS → part.5 Set KeyboardEvent.code value on Android and Firefox OS (WIP)
TODO:

* Support special keys on Sun keyboard
* Fix NumLock vs Clear on Mac.
* Test Android version with USB connected keyboards (I've tested via Bluetooth converter of USB HID).
* Needs to investigate Android's scancode of F3 - F5 and NoConvert, Convert and Katakana/Hiragana of PC keyboard, Eisu and Kana of Mac keyboard.
This bug should support only physical keyboard. For virtual keycode or a11y tools which don't generate proper key event whose scancode is invalid, we need more work.
Summary: Implement KeyboardEvent.code → Implement KeyboardEvent.code (only for physical keyboard)
Okay, let's go forward.

* These patches don't support special keys on Sun keyboard. I'll file follow up bugs after they are defined in the spec.
* These patches don't support key events which are synthesized with not proper scancode (Windows) or hardware_keycode (X11). And also don't support virtual keyboard on Android for now.
Attachment #8408080 - Attachment is obsolete: true
Attachment #8416334 - Flags: review?(bugs)
Please review each patches which set .code value in widget roughly. I'll request additional review to each widget reviewer.
Attachment #8416335 - Flags: review?(bugs)
On Mac, it's too difficult to get scancode or USB HID Usage ID. So, we should use virtual keycode, instead...
Attachment #8408081 - Attachment is obsolete: true
Attachment #8408083 - Attachment is obsolete: true
Attachment #8416337 - Flags: review?(bugs)
GTK and Qt widget must be able to share the code.
Attachment #8408084 - Attachment is obsolete: true
Attachment #8416338 - Flags: review?(bugs)
Similarly, Android and Gonk must be able to share the scancode -> .code value mapping.
Attachment #8408170 - Attachment is obsolete: true
Attachment #8416339 - Flags: review?(bugs)
FYI: The .code spec is here:
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#keys-codevalues
The code values are defined in:
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3Events-code.html

KeyboardEvent.code represents the physical key. So, it must dependent on neither modifier state nor user selected keyboard layout.
Updating for https://www.w3.org/Bugs/Public/show_bug.cgi?id=25603
Attachment #8416334 - Attachment is obsolete: true
Attachment #8416334 - Flags: review?(bugs)
Attachment #8421557 - Flags: review?(bugs)
Attachment #8416335 - Attachment is obsolete: true
Attachment #8416335 - Flags: review?(bugs)
Attachment #8421558 - Flags: review?(bugs)
Attachment #8416337 - Attachment is obsolete: true
Attachment #8416337 - Flags: review?(bugs)
Attachment #8421559 - Flags: review?(bugs)
Sorry about delay with this stuff. Not too easy to review and I've had easier-to-review patches in my
queue. But reviews are coming...
Comment on attachment 8421557 [details] [diff] [review]
part.1 Implement KeyboardEvent.code

>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Convert)
>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(KanaMode)
>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Lang1)
>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Lang2)
>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Lang3)
>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Lang4)
>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Lang5)
I don't see Lang* in the spec, and there is HangulMode and Hanja
>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(MediaPlayPause)
>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(MediaSelect)
>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(MediaStop)
>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(MediaTrackNext)
>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(MediaTrackPrevious)
Looks like the spec has changed a bit.
MediaNextTract, MediaPreviousTrack

>+// Legacy keys
>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Abort)
>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Hyper)
>+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Meta)
Hmm, so Meta shouldn't be used as .code, but OSLeft should be used


How stable is this stuff in the spec.
Perhaps we should add a Pref to the KeyboardEvent.webidl code attribute and
do some testing with various keyboards and OSes, and only then enable it.
Attachment #8421557 - Flags: review?(bugs) → review+
Comment on attachment 8421558 [details] [diff] [review]
part.2 Set KeyboardEvent.code value on Windows

>+// Media keys
>+// CODE_MAP_WIN(BrowserBack,               0xE000) // VK_BROWSER_BACK
>+
>+// CODE_MAP_WIN(BrowserFavorites,          0xE000) // VK_BROWSER_FAVORITES
>+
>+// CODE_MAP_WIN(BrowserForward,            0xE000) // VK_BROWSER_FORWARD
>+
>+// CODE_MAP_WIN(BrowserHome,               0xE000) // VK_BROWSER_HOME
>+
>+// CODE_MAP_WIN(BrowserRefresh,            0xE000) // VK_BROWSER_REFRESH
>+
>+// CODE_MAP_WIN(BrowserSearch,             0xE000) // VK_BROWSER_SEARCH
>+
>+// CODE_MAP_WIN(BrowserStop,               0xE000) // VK_BROWSER_STOP
>+
>+// CODE_MAP_WIN(Eject) // not available?
>+
>+// CODE_MAP_WIN(LaunchApp1,                0xE000) // VK_LAUNCH_APP1
>+
>+// CODE_MAP_WIN(LaunchApp2,                0xE000) // VK_LAUNCH_APP2
>+
>+// CODE_MAP_WIN(LaunchMail,                0xE000) // VK_LAUNCH_MAIL
>+
>+// CODE_MAP_WIN(MediaPlayPause,            0xE000) // VK_MEDIA_PLAY_PAUSE
>+
>+// CODE_MAP_WIN(MediaSelect,               0xE000) // VK_LAUNCH_MEDIA_SELECT
>+
>+// CODE_MAP_WIN(MediaStop,                 0xE000) // VK_MEDIA_STOP
>+
>+// CODE_MAP_WIN(MediaTrackNext,            0xE000) // VK_MEDIA_NEXT_TRACK
>+
>+// CODE_MAP_WIN(MediaTrackPrevious,        0xE000) // VK_MEDIA_PREV_TRACK
Why these all are commented out? Please add some comment that we special case stuff in ConvertScanCodeToCodeNameIndex
Attachment #8421558 - Flags: review?(bugs) → review+
Comment on attachment 8421559 [details] [diff] [review]
part.3 Set KeyboardEvent.code value on Mac

rs+
This like other widget/* need a peer review.
Attachment #8421559 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #33)
> Comment on attachment 8421557 [details] [diff] [review]
> part.1 Implement KeyboardEvent.code
> 
> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Convert)
> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(KanaMode)
> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Lang1)
> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Lang2)
> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Lang3)
> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Lang4)
> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Lang5)
> I don't see Lang* in the spec, and there is HangulMode and Hanja

https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3Events-code.html#table-key-code-alphanumeric-functional-2

Don't you refer older draft or key value spec? In the key value spec, there are HangulMode and Hanja, but not Lang*.

> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(MediaPlayPause)
> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(MediaSelect)
> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(MediaStop)
> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(MediaTrackNext)
> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(MediaTrackPrevious)
> Looks like the spec has changed a bit.
> MediaNextTract, MediaPreviousTrack

https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3Events-code.html#code-MediaTrackNext

> >+// Legacy keys
> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Abort)
> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Hyper)
> >+DEFINE_PHYSICAL_KEY_CODE_NAME_WITH_SAME_NAME(Meta)
> Hmm, so Meta shouldn't be used as .code, but OSLeft should be used

Oh, I'll file a spec bug about this. (This value is not used by us)

> How stable is this stuff in the spec.
> Perhaps we should add a Pref to the KeyboardEvent.webidl code attribute and
> do some testing with various keyboards and OSes, and only then enable it.

I think that KeyboardEvent.code value spec is enough stable at least for physical keyboard. Although, there are some minor bugs.

If we would disable the feature, wouldn't nobody test this feature on various keyboard and OSes?

FYI: All mapping in the patches from ScanCode is, I tested with a lot of keyboard including programmable keyboard. Therefore, for more testing, I believe that it should be enabled in default prefs for more feedback. Fortunately, next ESR is 31, so, even if we will change something about this, it shouldn't cause breaking the web. And if it would become unstable by being found some critical bugs, we could disable at that time.

Do you still think we should disable .code in default prefs? There is another option, it could be changed by a pref but should be enabled in default settings.
Flags: needinfo?(bugs)
Comment on attachment 8421560 [details] [diff] [review]
part.4 Set KeyboardEvent.code value on Linux

rs+
Attachment #8421560 - Flags: review?(bugs) → review+
Flags: needinfo?(bugs)
Comment on attachment 8421561 [details] [diff] [review]
part.5 Set KeyboardEvent.code value on Android and Firefox OS

rs+
Attachment #8421561 - Flags: review?(bugs) → review+
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #36)
> Don't you refer older draft or key value spec? In the key value spec, there
> are HangulMode and Hanja, but not Lang*.
Ah, yes, I was using the link in this bug comment 1


> If we would disable the feature, wouldn't nobody test this feature on
> various keyboard and OSes?
I was thinking some internal testing. But if the spec is stable enough, perhaps we can
just keep the API on, at least in Nightlies.

> Do you still think we should disable .code in default prefs? There is
> another option, it could be changed by a pref but should be enabled in
> default settings.
Yeah, could we perhaps get one extra Nightly cycle for testing. So enable for 32 Nightly, 
and for 33 everywhere?
(In reply to Olli Pettay [:smaug] from comment #39)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #36)
> > Do you still think we should disable .code in default prefs? There is
> > another option, it could be changed by a pref but should be enabled in
> > default settings.
> Yeah, could we perhaps get one extra Nightly cycle for testing. So enable
> for 32 Nightly, 
> and for 33 everywhere?

Ah, it's reasonable. Does "dom.keyboardevent.code" sound good to you?
Flags: needinfo?(bugs)
oops, perhaps, "dom.keyboardevent.code.enabled". Anyway, I'll post new patch which includes fix of new tests and request review again.
Flags: needinfo?(bugs)
Adding a pref, "dom.keyboardevent.code.enabled" and it's referred by the new tests.
Attachment #8426089 - Flags: review?(bugs)
This patch maps KeyboardEvent.code value which represents physical key with scancode. I confirmed the scancode values on Win8.1 and a lot of types of keyboards.

This patch doesn't support key events which are synthesized by something like a11y tools *without* proper scancode value. I'll file a follow up bug for it.
Attachment #8421557 - Attachment is obsolete: true
Attachment #8421558 - Attachment is obsolete: true
Attachment #8426090 - Flags: review?(jmathies)
This patch maps KeyboardEvent.code value which represents physical key with virtual keycode value.

On Mac, there is no way to get scancode of an NSEvent. Therefore, we cannot implement this strictly. E.g., on Mac, we cannot distinguish F13, F14, F15 vs. PrintScreen, ScrollLock, Pause since the PC keyboard's keys are mapped to F13 - F15.

FYI: clear key on numpad is same key as NumLock in USB spec. Therefore, clear key is mapped to "NumLock". See also the spec:
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3Events-code.html#code-NumLock
Attachment #8421559 - Attachment is obsolete: true
Attachment #8426091 - Flags: review?(smichaud)
This patch maps KeyboardEvent.code value which represents physical key with GdkEventKey::hardware_keycode on GTK. I confirmed all values of them are Ubuntu 14.04 (VM on Windows) and Fedora 20 (on physical machine) with a lot of types of keyboards.

I assume that Qt's scancode is same as the hardware_keycode of GDK. However, I cannot confirm it because I've not succeeded to build with Qt on my environment.

I'm not familiar with X11's scancode spec. If you know complete table of scancode, let me know it.
Attachment #8421560 - Attachment is obsolete: true
Attachment #8426092 - Flags: review?(romaxa)
Attachment #8426092 - Flags: review?(karlt)
This patch maps KeyboardEvent.code value which represents physical key with scancode.

I assume that Android and Gonk's scancode values are same. I confirmed the scancode values on Android 4.4 with a lot of types of keyboards.

This patch does NOT support virtual keyboard if it doesn't set scancode. I'll file a follow up bug at least for Android (I cannot confirm how this patch work on Firefox OS...).
Attachment #8421561 - Attachment is obsolete: true
Attachment #8426095 - Flags: review?(nchen)
Attachment #8426095 - Flags: review?(mwu)
Attachment #8426089 - Flags: review?(bugs) → review+
Attachment #8426091 - Flags: review?(smichaud) → review+
Attachment #8426090 - Flags: review?(jmathies) → review+
Comment on attachment 8426095 [details] [diff] [review]
part.5 Set KeyboardEvent.code value on Android and Firefox OS (r=smaug)

Review of attachment 8426095 [details] [diff] [review]:
-----------------------------------------------------------------

Android part looks okay.

::: widget/shared/NativeKeyToDOMCodeName.h
@@ +53,5 @@
>  // Writing system keys
>  CODE_MAP_WIN(Backquote,                 0x0029)
>  CODE_MAP_MAC(Backquote,                 kVK_ANSI_Grave)
>  CODE_MAP_X11(Backquote,                 0x0031)
> +CODE_MAP_ANDROID(Backquote,             0x0029)

What's the source for these values? The Android documentation for getScanCode() says "These values are not reliable and vary from device to device."
Attachment #8426095 - Flags: review?(nchen) → review+
(In reply to Jim Chen [:jchen :nchen] from comment #47)
> Comment on attachment 8426095 [details] [diff] [review]
> part.5 Set KeyboardEvent.code value on Android and Firefox OS (r=smaug)
> 
> Review of attachment 8426095 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Android part looks okay.
> 
> ::: widget/shared/NativeKeyToDOMCodeName.h
> @@ +53,5 @@
> >  // Writing system keys
> >  CODE_MAP_WIN(Backquote,                 0x0029)
> >  CODE_MAP_MAC(Backquote,                 kVK_ANSI_Grave)
> >  CODE_MAP_X11(Backquote,                 0x0031)
> > +CODE_MAP_ANDROID(Backquote,             0x0029)
> 
> What's the source for these values? The Android documentation for
> getScanCode() says "These values are not reliable and vary from device to
> device."

Thank you for your review.

I picked them up with actual behavior of my environment. Nexus 7 (2013) with USB-Bluetooth adapter and a lot of keyboards, and also connected the keyboards directly with USB (via USB host adapter).
Comment on attachment 8426092 [details] [diff] [review]
part.4 Set KeyboardEvent.code value on Linux (r=smaug)

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #45)
> This patch maps KeyboardEvent.code value which represents physical key with
> GdkEventKey::hardware_keycode on GTK. I confirmed all values of them are
> Ubuntu 14.04 (VM on Windows) and Fedora 20 (on physical machine) with a lot
> of types of keyboards.

> I'm not familiar with X11's scancode spec. If you know complete table of
> scancode, let me know it.

I think most modern Linux systems will get these from
http://cgit.freedesktop.org/xkeyboard-config/tree/keycodes/evdev

If you look at other files in that directory, you'll notice that the codes can
vary.  I suspect the 4-char code (e.g. TLDE) would be the ideal thing to map.
That can be obtained (on systems with XKB) with XkbGetNames() from
_XkbNamesRec::keys, which seems to be a pointer to an array indexed by key
code.  However, that's probably usually unnecessarily complex.

Given this is working on Ubuntu and Fedora systems, it seems a sensible first
implementation at least.
Attachment #8426092 - Flags: review?(karlt) → review+
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #49)
> That can be obtained (on systems with XKB) with XkbGetNames() from
> _XkbNamesRec::keys, which seems to be a pointer to an array indexed by key
> code.  However, that's probably usually unnecessarily complex.

Just for reference:
http://www.x.org/releases/current/doc/libX11/XKB/xkblib.html#The_XkbNamesRec_Structure
Attachment #8426092 - Flags: review?(romaxa) → review+
Thank you, Karl, for the information and review.

Indeed, I don't want too complicated implementation for rare environment. However, if it would be a problem for some users, we should discuss about that.
Do you have a Nexus 4 or Nexus 5? We have images available for those devices if you want to test B2G there - you just need a USB OTG cable to hook up your keyboard. Alternately, I can test this for you if you have a test page.
(In reply to Michael Wu [:mwu] from comment #52)
> Do you have a Nexus 4 or Nexus 5? We have images available for those devices
> if you want to test B2G there - you just need a USB OTG cable to hook up
> your keyboard. Alternately, I can test this for you if you have a test page.

I don't have both of them. Would you check it?

Test pages are here:
editable: https://dvcs.w3.org/hg/d4e/raw-file/tip/key-event-test.html
read-only: http://www.d-toybox.com/samples/key-event-test-on-readonly.html

.code value spec is here:
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3Events-code.html

The value doesn't depend on OS's active keyboard layout. It represents physical keyboard's key.

On the former, IME works. Therefore, I guess the latter is useful for testing Firefox OS.

A try build will be here:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=70f94bec3a36
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #53)
> (In reply to Michael Wu [:mwu] from comment #52)
> A try build will be here:
> https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=70f94bec3a36

Hmm, the binary image isn't uploaded...
Comment on attachment 8426095 [details] [diff] [review]
part.5 Set KeyboardEvent.code value on Android and Firefox OS (r=smaug)

Thanks for the info. I tested the patches on a Nexus 5 and the test page seems to indicate correct behavior.
Attachment #8426095 - Flags: review?(mwu) → review+
What is the status of this? I'm trying to catch when I click on play/pause, next and previous media keys on my corded apple keyboard but when I hit "play/pause" iTunes starts and after maybe a couple of seconds the event is shown in Firefox but the event.code is OSLeft and the keyCode is 224. If I hit next or previous keys nothing happens at all. All I'm doing right now is console.log in a handler for 'keydown' on the jsbin site.

I've read above that there seem to be problems checking some keys on mac. Is this not supported?

Thanks!
This is now disabled by pref in release builds because we still have additional work for supporting Sun keyboard's special keys (https://www.w3.org/Bugs/Public/show_bug.cgi?id=24739). But we could ignore the spec bug because such users are very minority, though.
I'm using Firefox Developer Edition and in about:config 'dom.keyboardevent.code.enabled' is set to "true".
Yes, in beta and release channel, it's false in the default settings. Dev Edition is based on Aurora. Therefore, it's enabled for tested by testers in default settings.
I'm sorry but I didn't understand that. Should it work for me since I've got Dev Edition and the option set to true?
But the 3 Apple Keyboard media keys seen here: http://support.apple.com/library/content/dam/edam/applecare/images/en_US/macbookpro/macbook_air_early.png does not work at all for me. Outputs nothing and when I hit the play/pause all that happends is that iTunes starts.

My previous text about the play/pause button outputting OSLeft after a while was just something I misunderstood, it came from when I hit Command-Tab to close iTunes.

Are the media keys on the Apple Keyboard supposed to work?
(In reply to Martin Lundberg from comment #64)
> Are the media keys on the Apple Keyboard supposed to work?

They will only work if the browser receives events from the OS.
Comment 58 suggests that the OS is capturing the events before they get to the focused application, for iTunes.
I found some more information about handling media keys in OS X and I'm not sure but should I create a new  bug about this instead of talking about it here or do you want it here?
Sorry for the double posting but so that you know what I'm talking about in the last comment what I found was http://overooped.com/post/2593597587/mediakeys which linked to https://github.com/nevyn/SPMediaKeyTap

I'm not sure how Firefox on OS X works if this is something it could use but I feel that Mozilla wanting things to be web-based instead of native apps things like this should work :)
I guess this is documented (thank you, :mayasuki; it is much much appreciated)
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.code
and
https://developer.mozilla.org/en-US/Firefox/Releases/32#Interfaces.2FAPIs.2FDOM

(I don't think there is a bug to enable this on release builds, don't forget to add ddn when this one is created!)
You need to log in before you can comment on or make changes to this bug.