Bug 202601

Summary: [Cocoa] Right Command key should have location DOM_KEY_LOCATION_RIGHT instead of DOM_KEY_LOCATION_STANDARD
Product: WebKit Reporter: Daniel Bates <dbates>
Component: PlatformAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, esprehn+autocc, ews-watchlist, kangil.han, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
For the bots
none
Patch
none
To Land none

Description Daniel Bates 2019-10-04 13:19:24 PDT
This bug is only applicable to Cocoa platforms.

Steps to reproduce:

1. Visit <https://unixpapa.com/js/testkey.html>.
2. Ensure the show attribute values for DOM 3 are enabled.
3. Press the Command key on the right-side of the keyboard.

Then I see:

keydown  keyCode=93  (])   which=93  (])   charCode=0        
         key=Meta char=undefined location=0 repeat=false
keyup    keyCode=93  (])   which=93  (])   charCode=0        
         key=Meta char=undefined location=0 repeat=false

Notice the location attribute is 0 == DOM_KEY_LOCATION_STANDARD

But I should see:

keydown  keyCode=93  (])   which=93  (])   charCode=0        
         key=Meta char=undefined location=1 repeat=false
keyup    keyCode=93  (])   which=93  (])   charCode=0        
         key=Meta char=undefined location=1 repeat=false

The location attribute should be 1 == DOM_KEY_LOCATION_RIGHT
Comment 1 Radar WebKit Bug Importer 2019-10-04 13:19:35 PDT
<rdar://problem/55992775>
Comment 2 Daniel Bates 2019-10-04 13:24:37 PDT
Created attachment 380245 [details]
For the bots
Comment 3 Daniel Bates 2019-10-22 09:45:50 PDT
Created attachment 381555 [details]
Patch
Comment 4 Darin Adler 2019-10-22 17:33:14 PDT
Comment on attachment 381555 [details]
Patch

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

> Source/WebCore/dom/KeyboardEvent.cpp:93
> +#if PLATFORM(COCOA)
> +    case VK_APPS: // Right Command
> +#endif

What harm would it cause to compile this on non-Cocoa platforms?
Comment 5 Daniel Bates 2019-10-22 17:42:30 PDT
(In reply to Darin Adler from comment #4)
> Comment on attachment 381555 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381555&action=review
> 
> > Source/WebCore/dom/KeyboardEvent.cpp:93
> > +#if PLATFORM(COCOA)
> > +    case VK_APPS: // Right Command
> > +#endif
> 
> What harm would it cause to compile this on non-Cocoa platforms?

Event.location == DOM_KEY_LOCATION_RIGHT for VK_APPS when I don't know where it is on keyboards for non-Cocoa platforms. I don't have a keyboard with this key, yet.

Longer term, need to audit if it is 100% correct to map right command to VK_APPS. Got to boot up Windows to double check. I think I recall USB HID spec saying it should be mapped to what corresponds to VK_RWIN.

Anyway, I didn't have a Windows setup to test so I patched ^^^ to match VK_APPS. Probably add a FIXME before landing to explain this if you bless this patch.
Comment 6 Wenson Hsieh 2019-10-30 16:32:51 PDT
Comment on attachment 381555 [details]
Patch

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

>>> Source/WebCore/dom/KeyboardEvent.cpp:93
>>> +#endif
>> 
>> What harm would it cause to compile this on non-Cocoa platforms?
> 
> Event.location == DOM_KEY_LOCATION_RIGHT for VK_APPS when I don't know where it is on keyboards for non-Cocoa platforms. I don't have a keyboard with this key, yet.
> 
> Longer term, need to audit if it is 100% correct to map right command to VK_APPS. Got to boot up Windows to double check. I think I recall USB HID spec saying it should be mapped to what corresponds to VK_RWIN.
> 
> Anyway, I didn't have a Windows setup to test so I patched ^^^ to match VK_APPS. Probably add a FIXME before landing to explain this if you bless this patch.

A FIXME to clarify this sounds reasonable.
Comment 7 Daniel Bates 2019-11-05 11:48:37 PST
Created attachment 382839 [details]
To Land
Comment 8 Daniel Bates 2019-11-05 11:50:25 PST
Comment on attachment 382839 [details]
To Land

Clearing flags on attachment: 382839

Committed r252066: <https://trac.webkit.org/changeset/252066>
Comment 9 Daniel Bates 2019-11-05 11:50:28 PST
All reviewed patches have been landed.  Closing bug.