Bug 36267 – Add support for KeyboardEvent.key attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Andy Estes]

2010-03-17 17:38:10 Andy Estes:
Safari fails the following test: http://samples.msdn.microsoft.com/ietestcenter/domevents/domevents_harness.htm?url=./keyboardevent.key.html

Safari does not implement the 'key' property on KeyboardEvent objects as specified by the W3C DOM Level 3 Events Editor's Draft at http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html.

<rdar://problem/7765874>

[Alexey Proskuryakov]

It is likely that out keyIdentifier property has very different behavior than this new one.

[Andy Estes]

(In reply to comment #1)
> It is likely that out keyIdentifier property has very different behavior than
> this new one.

Yes, I think that's true after a more careful reading of the spec.

[Andy Estes]

I have the bulk of this patch written but it only works on the mac.  I need to change the other platform bindings and update some tests.

[Andy Estes]

Based on this message from Doug Schepers on www-dom: http://lists.w3.org/Archives/Public/www-dom/2010JanMar/0074.html, I think it would be unwise to post my patch.

There has been significant discussion/disagreement on www-dom (see http://lists.w3.org/Archives/Public/www-dom/2009OctDec/0099.html) on how they key property should be implemented, so I think it would be premature for us to proceed with an implementation before there is consensus in the spec.

Doug indicates a significant revision to DOM Level 3 Events will be published in early April including feedback from the i18n community (which had been missing in previous revisions).  I'll revisit this issue at that time.

[PhistucK]

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.

[Piotrek Koszuliński (Reinmar)]

Now, when Chrome 51 got KeyboardEvent.key support (https://bugs.chromium.org/p/chromium/issues/detail?id=227231#c80), Safari is the last browser to not support it. Please consider implementing this feature.

(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

[Chris Dumez]

Test: http://w3c-test.org/uievents/keyboard/key-101en-us-manual.html

[Chris Dumez]

Created attachment 290424 [details]
Early WIP Patch

[Chris Dumez]

Created attachment 290425 [details]
WIP Patch

[Darin Adler]

Comment on attachment 290424 [details]
Early WIP Patch

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

Not really happy about the term "DOM key" to mean the key string. Especially when it spelled "domKey". Even though this does come from the UIEvents KeyboardEvent specification. Note that keyIdentifier also came from an earlier DOM specification but we did not use the word DOM.

> Source/WebCore/dom/KeyboardEvent.idl:48
> +    // Everythin below is legacy.

Typo: Everythin

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:47
> +        return "ArrowUp";

ASCIILiteral

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:250
> +            return "Meta";

ASCIILiteral

[Chris Dumez]

(In reply to comment #11)
> Comment on attachment 290424 [details]
> Early WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290424&action=review
> 
> Not really happy about the term "DOM key" to mean the key string. Especially
> when it spelled "domKey". Even though this does come from the UIEvents
> KeyboardEvent specification. Note that keyIdentifier also came from an
> earlier DOM specification but we did not use the word DOM.
> 
> > Source/WebCore/dom/KeyboardEvent.idl:48
> > +    // Everythin below is legacy.
> 
> Typo: Everythin
> 
> > Source/WebCore/platform/cocoa/KeyEventCocoa.mm:47
> > +        return "ArrowUp";
> 
> ASCIILiteral
> 
> > Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:250
> > +            return "Meta";
> 
> ASCIILiteral

Ok, I think I will just use "key" in the next iteration.

[Chris Dumez]

Created attachment 290440 [details]
WIP Patch

[Chris Dumez]

Created attachment 290442 [details]
WIP Patch

[Chris Dumez]

Created attachment 290448 [details]
WIP Patch

[Chris Dumez]

Created attachment 290453 [details]
WIP Patch

[Build Bot]

Comment on attachment 290453 [details]
WIP Patch

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

New failing tests:
fast/events/keyboardevent-key.html

[Build Bot]

Created attachment 290458 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 290459 [details]
Patch

[Chris Dumez]

*** Bug 69029 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Created attachment 290474 [details]
Patch

[Build Bot]

Comment on attachment 290474 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/dom/events/Event-init-while-dispatching.html

[Build Bot]

Created attachment 290475 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Chris Dumez]

Created attachment 290476 [details]
Patch

[Darin Adler]

Comment on attachment 290476 [details]
Patch

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

> Source/WebCore/platform/PlatformKeyboardEvent.h:113
> +        String key() const { return m_key; }

const String& for return value instead?

> Source/WebCore/platform/cocoa/KeyEventCocoa.h:34
> +String keyForCharCode(unichar charCode);
>  String keyIdentifierForCharCode(unichar charCode);

Is “CharCode” really the right name for this in Cocoa? Might be worth renaming these later if we determine that the correct term for this is slightly different.

> Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:134
> +    NSString *s = event.characters;

I would have named this "characters".

> Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:137
> +        return ASCIILiteral("Dead");

Might be worth commenting why "Dead" is the right name here.

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:241
> +String keyForKeyEvent(NSEvent* event)

Should be NSEvent * with a space.

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:244
> +    switch ([event keyCode]) {
> +    case 0x14:

Should there be a comment about where these key code numbers came from?

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:269
> +    bool isControlDown = ([event modifierFlags] & NSControlKeyMask);

What is the non-deprecated way of doing this?

[Chris Dumez]

Created attachment 290506 [details]
Patch

[Chris Dumez]

Created attachment 290510 [details]
Patch

[Chris Dumez]

(In reply to comment #25)
> Comment on attachment 290476 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290476&action=review
> 
> > Source/WebCore/platform/PlatformKeyboardEvent.h:113
> > +        String key() const { return m_key; }
> 
> const String& for return value instead?
> 
> > Source/WebCore/platform/cocoa/KeyEventCocoa.h:34
> > +String keyForCharCode(unichar charCode);
> >  String keyIdentifierForCharCode(unichar charCode);
> 
> Is “CharCode” really the right name for this in Cocoa? Might be worth
> renaming these later if we determine that the correct term for this is
> slightly different.
> 
> > Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:134
> > +    NSString *s = event.characters;
> 
> I would have named this "characters".
> 
> > Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:137
> > +        return ASCIILiteral("Dead");
> 
> Might be worth commenting why "Dead" is the right name here.
> 
> > Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:241
> > +String keyForKeyEvent(NSEvent* event)
> 
> Should be NSEvent * with a space.
> 
> > Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:244
> > +    switch ([event keyCode]) {
> > +    case 0x14:
> 
> Should there be a comment about where these key code numbers came from?
> 
> > Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:269
> > +    bool isControlDown = ([event modifierFlags] & NSControlKeyMask);
> 
> What is the non-deprecated way of doing this?

I will try to find a non-deprecated way to do this in a follow-up. Unfortunately, the document is not clear on the alternative:
- https://developer.apple.com/reference/appkit/nsevent/1534405-modifierflags?language=objc
- https://developer.apple.com/reference/appkit/nsevent/modifier_flags?language=objc

[Chris Dumez]

(In reply to comment #28)
> (In reply to comment #25)
> > Comment on attachment 290476 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=290476&action=review
> > 
> > > Source/WebCore/platform/PlatformKeyboardEvent.h:113
> > > +        String key() const { return m_key; }
> > 
> > const String& for return value instead?
> > 
> > > Source/WebCore/platform/cocoa/KeyEventCocoa.h:34
> > > +String keyForCharCode(unichar charCode);
> > >  String keyIdentifierForCharCode(unichar charCode);
> > 
> > Is “CharCode” really the right name for this in Cocoa? Might be worth
> > renaming these later if we determine that the correct term for this is
> > slightly different.
> > 
> > > Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:134
> > > +    NSString *s = event.characters;
> > 
> > I would have named this "characters".
> > 
> > > Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:137
> > > +        return ASCIILiteral("Dead");
> > 
> > Might be worth commenting why "Dead" is the right name here.
> > 
> > > Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:241
> > > +String keyForKeyEvent(NSEvent* event)
> > 
> > Should be NSEvent * with a space.
> > 
> > > Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:244
> > > +    switch ([event keyCode]) {
> > > +    case 0x14:
> > 
> > Should there be a comment about where these key code numbers came from?
> > 
> > > Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:269
> > > +    bool isControlDown = ([event modifierFlags] & NSControlKeyMask);
> > 
> > What is the non-deprecated way of doing this?
> 
> I will try to find a non-deprecated way to do this in a follow-up.
> Unfortunately, the document is not clear on the alternative:
> -
> https://developer.apple.com/reference/appkit/nsevent/1534405-
> modifierflags?language=objc
> -
> https://developer.apple.com/reference/appkit/nsevent/
> modifier_flags?language=objc

Oh, it looks like AppKit merely renamed those to things like NSControlKeyMask -> NSEventModifierFlagControl. I'll look into this in a follow-up.

[Chris Dumez]

Created attachment 290513 [details]
Patch

[Chris Dumez]

Created attachment 290514 [details]
Patch

[Chris Dumez]

Created attachment 290520 [details]
Patch

[Chris Dumez]

Created attachment 290522 [details]
Patch

[Chris Dumez]

Comment on attachment 290522 [details]
Patch

Clearing flags on attachment: 290522

Committed r206750: <http://trac.webkit.org/changeset/206750>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

Yay!

[Chris Dumez]

(In reply to comment #36)
> Yay!

I am now working on 'code' attribute via Bug 149584.

[Csaba Osztrogonác]

(In reply to comment #34)
> Comment on attachment 290522 [details]
> Patch
> 
> Clearing flags on attachment: 290522
> 
> Committed r206750: <http://trac.webkit.org/changeset/206750>

It broke the Apple Mac cmake build:
https://build.webkit.org/builders/Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/8842

[lingtalfi]

Sorry I'm late for the party.
Does this mean we will be able to use KeyboardEvent.key in Safari soon?
If so, when (or which version of Safari)?

[Chris Dumez]

(In reply to comment #39)
> Sorry I'm late for the party.
> Does this mean we will be able to use KeyboardEvent.key in Safari soon?
> If so, when (or which version of Safari)?

I cannot comment on when a particular feature will ship in a stable version of Safari.
However, I encourage you to test the feature using a WebKit nightly build [1] or Safari Technology Preview [2] before it ships. Please file bugs if you find issues.

[1] https://webkit.org/nightly/ (Already has KeyboardEvent.key)
[2] https://developer.apple.com/safari/download/ (Does not have KeyboardEvent.key but should hopefully have it in the next version).

[lingtalfi]

(In reply to comment #40)
> (In reply to comment #39)
> > Sorry I'm late for the party.
> > Does this mean we will be able to use KeyboardEvent.key in Safari soon?
> > If so, when (or which version of Safari)?
> 
> I cannot comment on when a particular feature will ship in a stable version
> of Safari.
> However, I encourage you to test the feature using a WebKit nightly build
> [1] or Safari Technology Preview [2] before it ships. Please file bugs if
> you find issues.
> 
> [1] https://webkit.org/nightly/ (Already has KeyboardEvent.key)
> [2] https://developer.apple.com/safari/download/ (Does not have
> KeyboardEvent.key but should hopefully have it in the next version).

Thanks, I was unaware of the existence of the webkit browser (good one).
I wish safari had a release calendar like chrome (https://www.chromium.org/developers/calendar) or firefox (https://wiki.mozilla.org/RapidRelease/Calendar).

[Csaba Osztrogonác]

(In reply to comment #38)
> (In reply to comment #34)
> > Comment on attachment 290522 [details]
> > Patch
> > 
> > Clearing flags on attachment: 290522
> > 
> > Committed r206750: <http://trac.webkit.org/changeset/206750>
> 
> It broke the Apple Mac cmake build:
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/8842

still broken:
/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:36:9: fatal error: 'HIToolbox/Events.h' file not found
#import <HIToolbox/Events.h>

It seems HIToolbox should be installed to the bot or should be found by cmake.

[Darin Adler]

(In reply to comment #42)
> It seems HIToolbox should be installed to the bot or should be found by
> cmake.

This is most likely something wrong with the include paths passed to the compiler in the cmake configuration.

I’m not sure who’s working on keeping that build working. I know some people are trying to advocate it as the future of the Mac port, but it’s not one of the builds on https://build.webkit.org/dashboard/ or in EWS.

[Csaba Osztrogonác]

(In reply to comment #43)
> (In reply to comment #42)
> > It seems HIToolbox should be installed to the bot or should be found by
> > cmake.
> 
> This is most likely something wrong with the include paths passed to the
> compiler in the cmake configuration.
> 
> I’m not sure who’s working on keeping that build working. I know some people
> are trying to advocate it as the future of the Mac port, but it’s not one of
> the builds on https://build.webkit.org/dashboard/ or in EWS.

As far as I know Alex is the only one who works on Apple Mac cmake build,
that's why I cc-ed him to the bug. I haven't seen any other Apple employee
fixing the cmake build.

(In reply to comment #34)
> Comment on attachment 290522 [details]
> Patch
> 
> Clearing flags on attachment: 290522
> 
> Committed r206750: <http://trac.webkit.org/changeset/206750>

This appears to have caused bug 163506.

[Lucas Forschler]

Mass move bugs into the DOM component.