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: | 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
Andy Estes
2010-03-17 19:52:21 PDT
It is likely that out keyIdentifier property has very different behavior than this new one. (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. 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. 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. 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. 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 Created attachment 290424 [details]
Early WIP Patch
Created attachment 290425 [details]
WIP Patch
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 (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. Created attachment 290440 [details]
WIP Patch
Created attachment 290442 [details]
WIP Patch
Created attachment 290448 [details]
WIP Patch
Created attachment 290453 [details]
WIP Patch
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 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
Created attachment 290459 [details]
Patch
*** Bug 69029 has been marked as a duplicate of this bug. *** Created attachment 290474 [details]
Patch
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 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
Created attachment 290476 [details]
Patch
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? Created attachment 290506 [details]
Patch
Created attachment 290510 [details]
Patch
(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 (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. Created attachment 290513 [details]
Patch
Created attachment 290514 [details]
Patch
Created attachment 290520 [details]
Patch
Created attachment 290522 [details]
Patch
Comment on attachment 290522 [details] Patch Clearing flags on attachment: 290522 Committed r206750: <http://trac.webkit.org/changeset/206750> All reviewed patches have been landed. Closing bug. Yay! (In reply to comment #36) > Yay! I am now working on 'code' attribute via Bug 149584. (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 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)? (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). (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). (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. (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. (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. Mass move bugs into the DOM component. |