Bug 149584 - Implement KeyboardEvent.code from the UI Event spec
Summary: Implement KeyboardEvent.code from the UI Event spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Events (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Chris Dumez
URL: https://w3c.github.io/uievents/#dom-k...
Keywords:
: 162853 (view as bug list)
Depends on:
Blocks: 162852
  Show dependency treegraph
 
Reported: 2015-09-27 00:17 PDT by Chris Rebert
Modified: 2016-10-04 23:48 PDT (History)
16 users (show)

See Also:


Attachments
Early WIP patch (24.72 KB, patch)
2016-10-03 17:00 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-yosemite (1.57 MB, application/zip)
2016-10-03 19:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.26 MB, application/zip)
2016-10-03 19:14 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-yosemite (767.62 KB, application/zip)
2016-10-03 19:25 PDT, Build Bot
no flags Details
Patch (64.66 KB, patch)
2016-10-03 19:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2 (6.48 MB, application/zip)
2016-10-03 20:57 PDT, Build Bot
no flags Details
Patch (82.99 KB, patch)
2016-10-03 21:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (82.80 KB, patch)
2016-10-04 18:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (82.17 KB, patch)
2016-10-04 19:13 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (82.39 KB, patch)
2016-10-04 19:17 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rebert 2015-09-27 00:17:19 PDT
WebKit does not yet implement the KeyboardEvent.code property from the W3C UI Events Specification. See:
* https://w3c.github.io/uievents/#widl-KeyboardEvent-code
* https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code
Comment 1 PhistucK 2015-09-30 09:28:47 PDT
Looks like the specification is stable now. Firefox shipped KeyboardEvent.key and KeyboardEvent.code. Internet Explorer 9+ shipped KeyboardEvent.key (though its implementation is not up to date with the current specification).
Chrome will be shipping KeyboardEvent.code in Chrome 48 and KeyboardEvent.key should also follow soon after (or even in the same version).

Perhaps this is a good time to reconsider implementing this.
Comment 2 mikolaj.konarski 2016-09-30 16:16:39 PDT
(Sorry for the spam on multiple tickets, but it's dire):
Additionally, Chrome will soon drop keyIdentifier
https://www.chromestatus.com/features/5316065118650368
so JS that uses keyIdentifier (because of webkit) will no longer
work on Chrome, so it would be incredibly useful if webkit
implemented the current standard
https://w3c.github.io/uievents/#events-keyboardevents
Comment 3 Chris Dumez 2016-10-01 20:40:17 PDT
*** Bug 162853 has been marked as a duplicate of this bug. ***
Comment 4 Chris Dumez 2016-10-03 14:16:56 PDT
Spec for code:
https://www.w3.org/TR/uievents-code/
Comment 5 PhistucK 2016-10-03 14:26:14 PDT
This one is more up to date -
https://w3c.github.io/uievents-code/

There were some changed since December -
https://github.com/w3c/uievents-code/commits/gh-pages
Comment 6 Chris Dumez 2016-10-03 17:00:04 PDT
Created attachment 290536 [details]
Early WIP patch
Comment 7 WebKit Commit Bot 2016-10-03 17:52:39 PDT
Attachment 290536 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:395:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Build Bot 2016-10-03 19:10:13 PDT
Comment on attachment 290536 [details]
Early WIP patch

Attachment 290536 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2214251

New failing tests:
imported/w3c/web-platform-tests/dom/events/Event-subclasses-constructors.html
Comment 9 Build Bot 2016-10-03 19:10:17 PDT
Created attachment 290550 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 10 Build Bot 2016-10-03 19:14:04 PDT
Comment on attachment 290536 [details]
Early WIP patch

Attachment 290536 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2214274

New failing tests:
imported/w3c/web-platform-tests/dom/events/Event-subclasses-constructors.html
Comment 11 Build Bot 2016-10-03 19:14:09 PDT
Created attachment 290551 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 12 Chris Dumez 2016-10-03 19:21:20 PDT
Manual test: http://w3c-test.org/uievents/keyboard/key-101en-us-manual.html
Comment 13 Build Bot 2016-10-03 19:24:55 PDT
Comment on attachment 290536 [details]
Early WIP patch

Attachment 290536 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2214340

New failing tests:
imported/w3c/web-platform-tests/dom/events/Event-subclasses-constructors.html
Comment 14 Build Bot 2016-10-03 19:25:00 PDT
Created attachment 290552 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 15 Chris Dumez 2016-10-03 19:30:34 PDT
Created attachment 290553 [details]
Patch
Comment 16 Chris Dumez 2016-10-03 19:37:36 PDT
I also uploaded a PR to fix the web-platform-tests as they do not match exactly the latest specification:
- https://github.com/w3c/web-platform-tests/pull/3864
Comment 17 Build Bot 2016-10-03 20:57:01 PDT
Comment on attachment 290553 [details]
Patch

Attachment 290553 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2214683

New failing tests:
imported/w3c/web-platform-tests/dom/events/Event-subclasses-constructors.html
fast/events/keyboardevent-code.html
fast/events/constructors/keyboard-event-constructor.html
Comment 18 Build Bot 2016-10-03 20:57:06 PDT
Created attachment 290562 [details]
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 19 Chris Dumez 2016-10-03 21:36:43 PDT
Created attachment 290566 [details]
Patch
Comment 20 Darin Adler 2016-10-04 16:01:50 PDT
Comment on attachment 290566 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290566&action=review

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:293
> +    /* Keys in the alphanumeric section. */

This use of /* */ seems a bit peculiar.

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:353
> +    case kVK_Return: return ASCIILiteral("Enter"); //  Labelled Return on Apple keyboards.

Spelling error: labeled doesn’t have a double "l". Also there’s an extra space there.

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:376
> +    // Insert: Not present on Apple keyboards.

I don’t understand fully how that is relevant. What about third party keyboards not specifically designed for Mac?

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:387
> +    case kVK_ANSI_KeypadClear: return ASCIILiteral("NumLock"); // On the Mac, the "NumLock" code should be used for the numpad Clear key.

Comment says what should be done, but not why.

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:399
> +    // NumpadBackspace: Found on the Microsoft Natural Keyboard.

Can’t you plug one of those into a Mac?

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:400
> +    // NumpadClear: On the Mac, the numpad Clear key should always be encoded as "NumLock".

Again, what but not why.
Comment 21 Chris Dumez 2016-10-04 16:16:21 PDT
Comment on attachment 290566 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290566&action=review

>> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:293
>> +    /* Keys in the alphanumeric section. */
> 
> This use of /* */ seems a bit peculiar.

Trying to differentiate with codes that are in the specification but commented below, but OK. I'll fix.

>> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:353
>> +    case kVK_Return: return ASCIILiteral("Enter"); //  Labelled Return on Apple keyboards.
> 
> Spelling error: labeled doesn’t have a double "l". Also there’s an extra space there.

Copied from the spec, I'll fix.

>> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:376
>> +    // Insert: Not present on Apple keyboards.
> 
> I don’t understand fully how that is relevant. What about third party keyboards not specifically designed for Mac?

Insert / Help have the same key code. We already map the code to "help" above.

>> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:387
>> +    case kVK_ANSI_KeypadClear: return ASCIILiteral("NumLock"); // On the Mac, the "NumLock" code should be used for the numpad Clear key.
> 
> Comment says what should be done, but not why.

This is what the spec says. I do not know why. I am merely trying to match the specification and other browsers (verified on Chrome/Mac).

>> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:399
>> +    // NumpadBackspace: Found on the Microsoft Natural Keyboard.
> 
> Can’t you plug one of those into a Mac?

I do not know. My guess is that you can but those extra keys will not necessarily work. What I know is that:
- There is no corresponding constant in Events.h
- Chrome/Mac does not map NumpadBackspace on Mac either (checked their code).
- Mozilla does not list it here: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code

This comment applies for other special keys on the Microsoft natural keyboard as well.

>> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:400
>> +    // NumpadClear: On the Mac, the numpad Clear key should always be encoded as "NumLock".
> 
> Again, what but not why.

These comments are copy/pasted from the specification. I do not know why, I just know that this is what the specification says we should do.
Comment 22 Chris Dumez 2016-10-04 18:30:15 PDT
Created attachment 290677 [details]
Patch
Comment 23 Chris Dumez 2016-10-04 19:13:53 PDT
Created attachment 290684 [details]
Patch
Comment 24 Chris Dumez 2016-10-04 19:17:11 PDT
Created attachment 290685 [details]
Patch
Comment 25 WebKit Commit Bot 2016-10-04 20:16:04 PDT
Comment on attachment 290685 [details]
Patch

Clearing flags on attachment: 290685

Committed r206803: <http://trac.webkit.org/changeset/206803>
Comment 26 WebKit Commit Bot 2016-10-04 20:16:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 mikolaj.konarski 2016-10-04 23:48:28 PDT
We are making history.