Bug 165904 - Move charCode / keyCode / which attributes from UIEvent to KeyboardEvent
Summary: Move charCode / keyCode / which attributes from UIEvent to KeyboardEvent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2016-12-15 10:54 PST by Chris Dumez
Modified: 2016-12-15 17:30 PST (History)
10 users (show)

See Also:


Attachments
Patch (6.90 KB, patch)
2016-12-15 11:44 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.54 KB, patch)
2016-12-15 13:04 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.87 KB, patch)
2016-12-15 13:46 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (31.07 KB, patch)
2016-12-15 16:00 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (31.35 KB, patch)
2016-12-15 16:07 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (31.36 KB, patch)
2016-12-15 16:58 PST, 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 Dumez 2016-12-15 10:54:01 PST
Bring back KeyboardEvent's charCode / keyCode / which attributes.

They are still in the specification: https://w3c.github.io/uievents/#legacy-interface-KeyboardEvent
They are still exposed by Firefox and Chrome.
They are still covered by W3C web-platform-tests.
Our implementation still supports them, we merely did not expose them to the Web.
Comment 1 Chris Dumez 2016-12-15 11:44:00 PST
Created attachment 297206 [details]
Patch
Comment 2 Daniel Bates 2016-12-15 11:59:09 PST
Comment on attachment 297206 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/dom/events/Event-subclasses-constructors-expected.txt:37
> +PASS KeyboardEvent constructor (argument with non-default values) 

Wow, this is the only test we have to cover this functionality. Can we please add more tests?
Comment 3 Chris Dumez 2016-12-15 13:04:50 PST
Created attachment 297214 [details]
Patch
Comment 4 Chris Dumez 2016-12-15 13:46:59 PST
Created attachment 297216 [details]
Patch
Comment 5 Sam Weinig 2016-12-15 15:56:51 PST
Looks good, but please fix GTK. r=me.
Comment 6 Chris Dumez 2016-12-15 16:00:46 PST
Created attachment 297238 [details]
Patch
Comment 7 Chris Dumez 2016-12-15 16:07:55 PST
Created attachment 297241 [details]
Patch
Comment 8 Darin Adler 2016-12-15 16:18:22 PST
Comment on attachment 297241 [details]
Patch

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

> LayoutTests/fast/events/script-tests/init-events.js:110
> +shouldBeUndefined("testInitEvent('Mouse', '\"a\", false, false, window, 1001, 1002, 1003, 1004, 1005, false, false, false, false, 1006, null').keyCode");
> +shouldBeUndefined("testInitEvent('Mouse', '\"a\", false, false, window, 1001, 1002, 1003, 1004, 1005, false, false, false, false, 1006, null').charCode");

I would have thought we’d do this instead:

    shouldBeFalse("'charCode' in xxx");

Why not test that that the property is not even present rather than testing for the weaker condition that the property’s value is undefined?
Comment 9 Chris Dumez 2016-12-15 16:35:21 PST
(In reply to comment #8)
> Comment on attachment 297241 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297241&action=review
> 
> > LayoutTests/fast/events/script-tests/init-events.js:110
> > +shouldBeUndefined("testInitEvent('Mouse', '\"a\", false, false, window, 1001, 1002, 1003, 1004, 1005, false, false, false, false, 1006, null').keyCode");
> > +shouldBeUndefined("testInitEvent('Mouse', '\"a\", false, false, window, 1001, 1002, 1003, 1004, 1005, false, false, false, false, 1006, null').charCode");
> 
> I would have thought we’d do this instead:
> 
>     shouldBeFalse("'charCode' in xxx");
> 
> Why not test that that the property is not even present rather than testing
> for the weaker condition that the property’s value is undefined?

Ok, will fix before landing.
Comment 10 Chris Dumez 2016-12-15 16:58:23 PST
Created attachment 297258 [details]
Patch
Comment 11 WebKit Commit Bot 2016-12-15 17:30:00 PST
Comment on attachment 297258 [details]
Patch

Clearing flags on attachment: 297258

Committed r209895: <http://trac.webkit.org/changeset/209895>
Comment 12 WebKit Commit Bot 2016-12-15 17:30:05 PST
All reviewed patches have been landed.  Closing bug.