Bug 189604

Summary: [iOS] Key code is 0 for many hardware keyboard keys
Product: WebKit Reporter: Jeremy Jones <jeremyj-wk>
Component: New BugsAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, dbates, ews-watchlist, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar, WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=189793
https://bugs.webkit.org/show_bug.cgi?id=189815
Bug Depends on: 189804    
Bug Blocks: 190571, 191120    
Attachments:
Description Flags
WIP
none
WIP
none
Patch and layout test
none
Patch and layout test wenson_hsieh: review+

Description Jeremy Jones 2018-09-13 14:54:11 PDT
iOS hardware arrow keys and key up for all keys do not work.
Comment 1 Jeremy Jones 2018-09-13 14:55:11 PDT
rdar://problem/40813799
Comment 2 Jeremy Jones 2018-09-13 15:02:09 PDT
Created attachment 349706 [details]
WIP
Comment 3 EWS Watchlist 2018-09-13 15:04:22 PDT Comment hidden (obsolete)
Comment 4 Daniel Bates 2018-09-14 11:32:07 PDT
Comment on attachment 349706 [details]
WIP

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

> Source/WebCore/platform/cocoa/KeyEventCocoa.h:38
> +int windowsKeyCodeForHIDUsageKeyCode(uint16_t hidUsage);

This file is not the appropriate place for an iOS-specific function. A better place for would be KeyEventIOS.h.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:741
> +int windowsKeyCodeForHIDUsageKeyCode(uint16_t hidUsage)

If you take more suggestion above then this function implementation should move to KeyEventIOS.mm.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:748
> +        VK_A, // kHIDUsage_KeyboardA    = 0x04,    /* a or A */

I suggest that we use a similar formatting for the entries of this array as the entries in the array in windowsKeyCodeForKeyCode() (defined in KeyEventCocoa.mm).

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:908
> +        0, // kHIDUsage_KeyboardSelect    = 0x77,    /* Select */
> +        0, // kHIDUsage_KeyboardStop    = 0x78,    /* Stop */
> +        0, // kHIDUsage_KeyboardAgain    = 0x79,    /* Again */
> +        0, // kHIDUsage_KeyboardUndo    = 0x7A,    /* Undo */
> +        0, // kHIDUsage_KeyboardCut    = 0x7B,    /* Cut */
> +        0, // kHIDUsage_KeyboardCopy    = 0x7C,    /* Copy */
> +        0, // kHIDUsage_KeyboardPaste    = 0x7D,    /* Paste */
> +        0, // kHIDUsage_KeyboardFind    = 0x7E,    /* Find */
> +        0, // kHIDUsage_KeyboardMute    = 0x7F,    /* Mute */
> +        0, // kHIDUsage_KeyboardVolumeUp    = 0x80,    /* Volume Up */
> +        0, // kHIDUsage_KeyboardVolumeDown    = 0x81,    /* Volume Down */
> +        0, // kHIDUsage_KeyboardLockingCapsLock    = 0x82,    /* Locking Caps Lock */
> +        0, // kHIDUsage_KeyboardLockingNumLock    = 0x83,    /* Locking Num Lock */
> +        0, // kHIDUsage_KeyboardLockingScrollLock    = 0x84,    /* Locking Scroll Lock */
> +        0, // kHIDUsage_KeypadComma    = 0x85,    /* Keypad Comma */
> +        0, // kHIDUsage_KeypadEqualSignAS400    = 0x86,    /* Keypad Equal Sign for AS/400 */
> +        0, // kHIDUsage_KeyboardInternational1    = 0x87,    /* International1 */
> +        0, // kHIDUsage_KeyboardInternational2    = 0x88,    /* International2 */
> +        0, // kHIDUsage_KeyboardInternational3    = 0x89,    /* International3 */
> +        0, // kHIDUsage_KeyboardInternational4    = 0x8A,    /* International4 */
> +        0, // kHIDUsage_KeyboardInternational5    = 0x8B,    /* International5 */
> +        0, // kHIDUsage_KeyboardInternational6    = 0x8C,    /* International6 */
> +        0, // kHIDUsage_KeyboardInternational7    = 0x8D,    /* International7 */
> +        0, // kHIDUsage_KeyboardInternational8    = 0x8E,    /* International8 */
> +        0, // kHIDUsage_KeyboardInternational9    = 0x8F,    /* International9 */
> +        0, // kHIDUsage_KeyboardLANG1    = 0x90,    /* LANG1 */
> +        0, // kHIDUsage_KeyboardLANG2    = 0x91,    /* LANG2 */
> +        0, // kHIDUsage_KeyboardLANG3    = 0x92,    /* LANG3 */
> +        0, // kHIDUsage_KeyboardLANG4    = 0x93,    /* LANG4 */
> +        0, // kHIDUsage_KeyboardLANG5    = 0x94,    /* LANG5 */
> +        0, // kHIDUsage_KeyboardLANG6    = 0x95,    /* LANG6 */
> +        0, // kHIDUsage_KeyboardLANG7    = 0x96,    /* LANG7 */
> +        0, // kHIDUsage_KeyboardLANG8    = 0x97,    /* LANG8 */
> +        0, // kHIDUsage_KeyboardLANG9    = 0x98,    /* LANG9 */
> +        0, // kHIDUsage_KeyboardAlternateErase    = 0x99,    /* AlternateErase */
> +        0, // kHIDUsage_KeyboardSysReqOrAttention    = 0x9A,    /* SysReq/Attention */
> +        0, // kHIDUsage_KeyboardCancel    = 0x9B,    /* Cancel */
> +        0, // kHIDUsage_KeyboardClear    = 0x9C,    /* Clear */
> +        0, // kHIDUsage_KeyboardPrior    = 0x9D,    /* Prior */
> +        0, // kHIDUsage_KeyboardReturn    = 0x9E,    /* Return */
> +        0, // kHIDUsage_KeyboardSeparator    = 0x9F,    /* Separator */
> +        0, // kHIDUsage_KeyboardOut    = 0xA0,    /* Out */
> +        0, // kHIDUsage_KeyboardOper    = 0xA1,    /* Oper */
> +        0, // kHIDUsage_KeyboardClearOrAgain    = 0xA2,    /* Clear/Again */
> +        0, // kHIDUsage_KeyboardCrSelOrProps    = 0xA3,    /* CrSel/Props */
> +        0, // kHIDUsage_KeyboardExSel    = 0xA4,    /* ExSel */

It is unnecessary and wastes space in the binary to map all the keys after VK_MENU to 0. I suggest we omit these entries and use an if statement to cover this range of values.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:921
> +    static const int windowsKeyCode2[] = {
> +// /* 0xA5-0xDF Reserved */
> +        0, // kHIDUsage_KeyboardLeftControl    = 0xE0,    /* Left Control */
> +        0, // kHIDUsage_KeyboardLeftShift    = 0xE1,    /* Left Shift */
> +        0, // kHIDUsage_KeyboardLeftAlt    = 0xE2,    /* Left Alt */
> +        0, // kHIDUsage_KeyboardLeftGUI    = 0xE3,    /* Left GUI */
> +        0, // kHIDUsage_KeyboardRightControl    = 0xE4,    /* Right Control */
> +        0, // kHIDUsage_KeyboardRightShift    = 0xE5,    /* Right Shift */
> +        0, // kHIDUsage_KeyboardRightAlt    = 0xE6,    /* Right Alt */
> +        0, // kHIDUsage_KeyboardRightGUI    = 0xE7,    /* Right GUI */
> +    };

Ditto.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3745
> +    ASSERT(event._hidEvent);
> +    uint16_t hidKeyCode = IOHIDEventGetIntegerValue(event._hidEvent, kIOHIDEventFieldKeyboardUsage);

I suggest we forward declare the SPI UIEvent._keyCode in UIKitSPI.h and then make use of that here instead of using IOHIDEventGetIntegerValue().
Comment 5 Daniel Bates 2018-09-14 11:32:52 PDT
Is it possible to write a test(s) for this change?
Comment 6 Daniel Bates 2018-09-14 11:34:06 PDT
We should also fix this in WebKit Legacy on iOS.
Comment 7 Daniel Bates 2018-09-18 15:39:40 PDT
Comment on attachment 349706 [details]
WIP

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

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:786
> +        VK_DELETE, // kHIDUsage_KeyboardDeleteOrBackspace    = 0x2A,    /* Delete (Backspace) */

The key code, VK_DELETE, is inconsistent with the comment for kHIDUsage_KeyboardDeleteOrBackspace. VK_DELETE is the forward deletion key. But the comment saids that this should be the backwards deletion key, VK_BACK.
Comment 8 Jeremy Jones 2018-09-18 16:39:41 PDT
Created attachment 350074 [details]
WIP
Comment 9 EWS Watchlist 2018-09-18 16:43:00 PDT Comment hidden (obsolete)
Comment 10 Daniel Bates 2018-09-20 15:21:44 PDT
Comment on attachment 350074 [details]
WIP

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

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:814
> +        /* 0x46 */ VK_PRINTSCREEN,

VK_PRINTSCREEN is a non-existent virtual key code constant. In take it you meant to write VK_SNAPSHOT.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:827
> +        /* 0x53 */ VK_NUMLOCK,

This is incorrect. This virtual key code is for the key in the position of the num lock clear key on the keyboard. So, it should map to Windows virtual key code constant VK_CLEAR.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:843
> +        /* 0x63 */ VK_OEM_PERIOD, // Keypad . or Delete

This mapping would make the key in the position of the '.' on an ANSI keyboard indistinguishable from the key for the '.' in the num pad portion on the same keyboard. Moreover, this differs from Mac. We should map this virtual key code to VK_DECIMAL.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:846
> +        /* 0x65 */ VK_APPS, // Application
> +        /* 0x66 */ 0, // Power

As far as I can tell we support these virtual key codes neither on iOS nor Mac.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:861
> +        /* 0x75 */ VK_HELP,

This seems correct. However it disagrees with the code in windowsKeyCodeForCharCode(). Specifically line 690, <https://trac.webkit.org/browser/trunk/Source/WebCore/platform/cocoa/KeyEventCocoa.mm?rev=236143#L690>, considers NSInsertFunctionKey and NSHelpFunctionKey as equivalent to VK_INSERT (why?). We currently return 0 for the Windows virtual key code when the help key is pressed on Mac (why?) from quickly reading the code in KeyEventCocoa.mm. I am unclear if we would deliver the help key to the web page on Mac. I do not have a keyboard with a help key at the moment.

On another note, do we want to allow web pages to handle the help key as opposed to reserving that key for standard system/app usage?

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:874
> +        /* 0x82 */ VK_CAPITAL,

This does not seem correct. 0x82 is for the locking caps lock key. That is, not the caps lock key. This seems like an esoteric key to support and as far as I can tell we neither support it on iOS nor Mac.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:875
> +        /* 0x83 */ VK_NUMLOCK,

This does not seem correct. 0x83 is for the locking nums lock key. That is, not the num lock key. This seems like an esoteric key to support and as far as I can tell we neither support it on iOS nor Mac.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:876
> +        /* 0x84 */ VK_SCROLL,

This does not seem correct. 0x84 is for the locking nums scroll key. That is, not the scroll lock key. This seems like an esoteric key to support and as far as I can tell we neither support it on iOS nor Mac.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:898
> +        /* 0x9A */ VK_ATTN, // SysReq/Attention

As far as I can tell we support this virtual key code neither on iOS nor Mac.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:900
> +        /* 0x9C */ VK_CLEAR,

As far as I can tell we support this virtual key code neither on iOS nor Mac. We use VK_CLEAR to refer to the key in the position of the num pad clear key on an extended keyboard.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:902
> +        /* 0x9E */ VK_RETURN,

As far as I can tell we support this virtual key code neither on iOS nor Mac.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:908
> +        /* 0xA3 */ VK_CRSEL,
> +        /* 0xA4 */ VK_EXSEL,

As far as I can tell we support these virtual key codes neither on iOS nor Mac.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:921
> +        /* 0xE7 */ VK_RWIN, // Right GUI

This is not correct. This should be VK_APPS to match Mac WebKit and Chrome for Mac.

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:928
> +        return windowsKeyCode2[hidUsage];

This will do an out-of-bounds read because hidUsage >>>  6 = "the index position for the entry that has value VK_RWIN".

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3751
> +                                                             keyCode:event._keyCode

This property is only defined on UIPhysicalKeyboardEvent.
Comment 11 Daniel Bates 2018-09-20 16:40:14 PDT
Assigning to myself per a conversation with Jeremy Jones.
Comment 12 Daniel Bates 2018-09-20 20:13:33 PDT
Created attachment 350311 [details]
Patch and layout test
Comment 13 EWS Watchlist 2018-09-20 20:15:21 PDT Comment hidden (obsolete)
Comment 14 Daniel Bates 2018-09-20 20:20:28 PDT
Created attachment 350315 [details]
Patch and layout test
Comment 15 EWS Watchlist 2018-09-20 20:28:06 PDT Comment hidden (obsolete)
Comment 16 Jeremy Jones 2018-09-24 11:27:13 PDT
Comment on attachment 350315 [details]
Patch and layout test

provisional r=me
Comment 17 Daniel Bates 2018-09-24 12:13:22 PDT
Committed r236417: <https://trac.webkit.org/changeset/236417>