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+

Jeremy Jones
Reported 2018-09-13 14:54:11 PDT
iOS hardware arrow keys and key up for all keys do not work.
Attachments
WIP (16.85 KB, patch)
2018-09-13 15:02 PDT, Jeremy Jones 		no flags
DetailsFormatted DiffDiff
WIP (9.93 KB, patch)
2018-09-18 16:39 PDT, Jeremy Jones 		no flags
DetailsFormatted DiffDiff
Patch and layout test (61.23 KB, patch)
2018-09-20 20:13 PDT, Daniel Bates 		no flags
DetailsFormatted DiffDiff
Patch and layout test (61.28 KB, patch)
2018-09-20 20:20 PDT, Daniel Bates 		wenson_hsieh: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Jeremy Jones
Comment 1 2018-09-13 14:55:11 PDT
rdar://problem/40813799
Jeremy Jones
Comment 2 2018-09-13 15:02:09 PDT
Created attachment 349706 [details] WIP
EWS Watchlist
Comment 3 2018-09-13 15:04:22 PDT Comment hidden (obsolete)
Daniel Bates
Comment 4 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().
Daniel Bates
Comment 5 2018-09-14 11:32:52 PDT
Is it possible to write a test(s) for this change?
Daniel Bates
Comment 6 2018-09-14 11:34:06 PDT
We should also fix this in WebKit Legacy on iOS.
Daniel Bates
Comment 7 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.
Jeremy Jones
Comment 8 2018-09-18 16:39:41 PDT
Created attachment 350074 [details] WIP
EWS Watchlist
Comment 9 2018-09-18 16:43:00 PDT Comment hidden (obsolete)
Daniel Bates
Comment 10 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.
Daniel Bates
Comment 11 2018-09-20 16:40:14 PDT
Assigning to myself per a conversation with Jeremy Jones.
Daniel Bates
Comment 12 2018-09-20 20:13:33 PDT
Created attachment 350311 [details] Patch and layout test
EWS Watchlist
Comment 13 2018-09-20 20:15:21 PDT Comment hidden (obsolete)
Daniel Bates
Comment 14 2018-09-20 20:20:28 PDT
Created attachment 350315 [details] Patch and layout test
EWS Watchlist
Comment 15 2018-09-20 20:28:06 PDT Comment hidden (obsolete)
Jeremy Jones
Comment 16 2018-09-24 11:27:13 PDT
Comment on attachment 350315 [details] Patch and layout test provisional r=me
Daniel Bates
Comment 17 2018-09-24 12:13:22 PDT
Committed r236417: <https://trac.webkit.org/changeset/236417>
Note You need to log in before you can comment on or make changes to this bug.