Bug 165904 – Move charCode / keyCode / which attributes from UIEvent to KeyboardEvent

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 297206 [details]
Patch

[Daniel Bates]

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?

[Chris Dumez]

Created attachment 297214 [details]
Patch

[Chris Dumez]

Created attachment 297216 [details]
Patch

[Sam Weinig]

Looks good, but please fix GTK. r=me.

[Chris Dumez]

Created attachment 297238 [details]
Patch

[Chris Dumez]

Created attachment 297241 [details]
Patch

[Darin Adler]

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?

[Chris Dumez]

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

[Chris Dumez]

Created attachment 297258 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 297258 [details]
Patch

Clearing flags on attachment: 297258

Committed r209895: <http://trac.webkit.org/changeset/209895>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.