Bug 36267

Summary:

Add support for KeyboardEvent.key attribute

Product:

WebKit

Reporter:

Andy Estes <aestes>

Component:

UI Events

Assignee:

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:

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

Andy Estes

Reported 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>

Attachments
Early WIP Patch (20.87 KB, patch) 2016-09-30 22:03 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
WIP Patch (20.90 KB, patch) 2016-09-30 22:33 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
WIP Patch (23.23 KB, patch) 2016-10-01 12:48 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
WIP Patch (23.32 KB, patch) 2016-10-01 13:33 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
WIP Patch (30.04 KB, patch) 2016-10-01 16:41 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
WIP Patch (57.29 KB, patch) 2016-10-01 17:52 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 (6.10 MB, application/zip) 2016-10-01 19:27 PDT, Build Bot	no flags	Details
Patch (65.76 KB, patch) 2016-10-01 20:11 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (66.08 KB, patch) 2016-10-02 14:54 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews103 for mac-yosemite (759.15 KB, application/zip) 2016-10-02 16:08 PDT, Build Bot	no flags	Details
Patch (65.99 KB, patch) 2016-10-02 16:11 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (66.35 KB, patch) 2016-10-03 12:43 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (66.46 KB, patch) 2016-10-03 13:17 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (67.89 KB, patch) 2016-10-03 13:39 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (67.89 KB, patch) 2016-10-03 13:40 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (72.08 KB, patch) 2016-10-03 14:28 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (71.75 KB, patch) 2016-10-03 14:34 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2010-03-17 22:25:05 PDT

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

Andy Estes

Comment 2 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.

Andy Estes

Comment 3 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.

Andy Estes

Comment 4 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.

PhistucK

Comment 5 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.

Piotrek Koszuliński (Reinmar)

Comment 6 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.

mikolaj.konarski

Comment 7 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

Chris Dumez

Comment 8 2016-09-30 21:59:39 PDT

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

Chris Dumez

Comment 9 2016-09-30 22:03:29 PDT

Created attachment 290424 [details] Early WIP Patch

Chris Dumez

Comment 10 2016-09-30 22:33:47 PDT

Created attachment 290425 [details] WIP Patch

Darin Adler

Comment 11 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

Chris Dumez

Comment 12 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.

Chris Dumez

Comment 13 2016-10-01 12:48:39 PDT

Created attachment 290440 [details] WIP Patch

Chris Dumez

Comment 14 2016-10-01 13:33:27 PDT

Created attachment 290442 [details] WIP Patch

Chris Dumez

Comment 15 2016-10-01 16:41:12 PDT

Created attachment 290448 [details] WIP Patch

Chris Dumez

Comment 16 2016-10-01 17:52:28 PDT

Created attachment 290453 [details] WIP Patch

Build Bot

Comment 17 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

Build Bot

Comment 18 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

Chris Dumez

Comment 19 2016-10-01 20:11:43 PDT

Created attachment 290459 [details] Patch

Chris Dumez

Comment 20 2016-10-01 20:39:09 PDT

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

Chris Dumez

Comment 21 2016-10-02 14:54:38 PDT

Created attachment 290474 [details] Patch

Build Bot

Comment 22 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

Build Bot

Comment 23 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

Chris Dumez

Comment 24 2016-10-02 16:11:05 PDT

Created attachment 290476 [details] Patch

Darin Adler

Comment 25 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?

Chris Dumez

Comment 26 2016-10-03 12:43:28 PDT

Created attachment 290506 [details] Patch

Chris Dumez

Comment 27 2016-10-03 13:17:55 PDT

Created attachment 290510 [details] Patch

Chris Dumez

Comment 28 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

Chris Dumez

Comment 29 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.

Chris Dumez

Comment 30 2016-10-03 13:39:22 PDT

Created attachment 290513 [details] Patch

Chris Dumez

Comment 31 2016-10-03 13:40:44 PDT

Created attachment 290514 [details] Patch

Chris Dumez

Comment 32 2016-10-03 14:28:36 PDT

Created attachment 290520 [details] Patch

Chris Dumez

Comment 33 2016-10-03 14:34:24 PDT

Created attachment 290522 [details] Patch

Chris Dumez

Comment 34 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>

Chris Dumez

Comment 35 2016-10-03 14:35:24 PDT

All reviewed patches have been landed. Closing bug.

mikolaj.konarski

Comment 36 2016-10-03 15:13:30 PDT

Yay!

Chris Dumez

Comment 37 2016-10-03 15:15:03 PDT

(In reply to comment #36) > Yay! I am now working on 'code' attribute via Bug 149584.

Csaba Osztrogonác

Comment 38 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

lingtalfi

Comment 39 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)?

Chris Dumez

Comment 40 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).

lingtalfi

Comment 41 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).

Csaba Osztrogonác

Comment 42 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.

Darin Adler

Comment 43 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.

Csaba Osztrogonác

Comment 44 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.

mitz

Comment 45 2016-10-16 12:03:40 PDT

Lucas Forschler

Comment 46 2019-02-06 09:18:58 PST

Mass move bugs into the DOM component.

Note You need to log in before you can comment on or make changes to this bug.