Bug 36267

Summary: Add support for KeyboardEvent.key attribute
Product: WebKit Reporter: Andy Estes <aestes>
Component: UI EventsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, arv, benjamin, bfulgham, buildbot, cdumez, cmarcelo, commit-queue, darin, dbates, enrica, esprehn+autocc, kangil.han, kondapallykalyan, lingtalfi, mikolaj.konarski, mitz, ossy, phistuck, pkoszulinski, rniwa, sam, venkatpenukonda, webkit-bug-importer, webkit
Priority: P5 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Mac   
OS: OS X 10.11   
URL: https://w3c.github.io/uievents/#dom-keyboardevent-key
Bug Depends on:    
Bug Blocks: 69029, 162852    
Attachments:
Description Flags
Early WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-yosemite
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Andy Estes 2010-03-17 19:52:21 PDT
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>
Comment 1 Alexey Proskuryakov 2010-03-17 22:25:05 PDT
It is likely that out keyIdentifier property has very different behavior than this new one.
Comment 2 Andy Estes 2010-03-17 22:56:21 PDT
(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.
Comment 3 Andy Estes 2010-03-23 11:28:20 PDT
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.
Comment 4 Andy Estes 2010-03-25 15:05:47 PDT
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.
Comment 5 PhistucK 2015-09-30 09:26:01 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 6 Piotrek Koszuliński (Reinmar) 2016-04-04 02:08:36 PDT
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.
Comment 7 mikolaj.konarski 2016-09-30 16:16:13 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 9 Chris Dumez 2016-09-30 22:03:29 PDT
Created attachment 290424 [details]
Early WIP Patch
Comment 10 Chris Dumez 2016-09-30 22:33:47 PDT
Created attachment 290425 [details]
WIP Patch
Comment 11 Darin Adler 2016-10-01 10:09:15 PDT
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
Comment 12 Chris Dumez 2016-10-01 10:56:09 PDT
(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.
Comment 13 Chris Dumez 2016-10-01 12:48:39 PDT
Created attachment 290440 [details]
WIP Patch
Comment 14 Chris Dumez 2016-10-01 13:33:27 PDT
Created attachment 290442 [details]
WIP Patch
Comment 15 Chris Dumez 2016-10-01 16:41:12 PDT
Created attachment 290448 [details]
WIP Patch
Comment 16 Chris Dumez 2016-10-01 17:52:28 PDT
Created attachment 290453 [details]
WIP Patch
Comment 17 Build Bot 2016-10-01 19:27:15 PDT
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
Comment 18 Build Bot 2016-10-01 19:27:21 PDT
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
Comment 19 Chris Dumez 2016-10-01 20:11:43 PDT
Created attachment 290459 [details]
Patch
Comment 20 Chris Dumez 2016-10-01 20:39:09 PDT
*** Bug 69029 has been marked as a duplicate of this bug. ***
Comment 21 Chris Dumez 2016-10-02 14:54:38 PDT
Created attachment 290474 [details]
Patch
Comment 22 Build Bot 2016-10-02 16:08:14 PDT
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
Comment 23 Build Bot 2016-10-02 16:08:20 PDT
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
Comment 24 Chris Dumez 2016-10-02 16:11:05 PDT
Created attachment 290476 [details]
Patch
Comment 25 Darin Adler 2016-10-03 12:19:22 PDT
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?
Comment 26 Chris Dumez 2016-10-03 12:43:28 PDT
Created attachment 290506 [details]
Patch
Comment 27 Chris Dumez 2016-10-03 13:17:55 PDT
Created attachment 290510 [details]
Patch
Comment 28 Chris Dumez 2016-10-03 13:20:59 PDT
(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
Comment 29 Chris Dumez 2016-10-03 13:25:59 PDT
(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.
Comment 30 Chris Dumez 2016-10-03 13:39:22 PDT
Created attachment 290513 [details]
Patch
Comment 31 Chris Dumez 2016-10-03 13:40:44 PDT
Created attachment 290514 [details]
Patch
Comment 32 Chris Dumez 2016-10-03 14:28:36 PDT
Created attachment 290520 [details]
Patch
Comment 33 Chris Dumez 2016-10-03 14:34:24 PDT
Created attachment 290522 [details]
Patch
Comment 34 Chris Dumez 2016-10-03 14:35:15 PDT
Comment on attachment 290522 [details]
Patch

Clearing flags on attachment: 290522

Committed r206750: <http://trac.webkit.org/changeset/206750>
Comment 35 Chris Dumez 2016-10-03 14:35:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 mikolaj.konarski 2016-10-03 15:13:30 PDT
Yay!
Comment 37 Chris Dumez 2016-10-03 15:15:03 PDT
(In reply to comment #36)
> Yay!

I am now working on 'code' attribute via Bug 149584.
Comment 38 Csaba Osztrogonác 2016-10-05 05:06:34 PDT
(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
Comment 39 lingtalfi 2016-10-06 04:31:34 PDT
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)?
Comment 40 Chris Dumez 2016-10-06 09:02:42 PDT
(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).
Comment 41 lingtalfi 2016-10-06 11:38:44 PDT
(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).
Comment 42 Csaba Osztrogonác 2016-10-07 07:35:57 PDT
(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.
Comment 43 Darin Adler 2016-10-07 09:19:51 PDT
(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.
Comment 44 Csaba Osztrogonác 2016-10-10 03:07:23 PDT
(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.
Comment 45 mitz 2016-10-16 12:03:40 PDT
(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.
Comment 46 Lucas Forschler 2019-02-06 09:18:58 PST
Mass move bugs into the DOM component.