Bug 99188 - Incorrect keycodes for numpad /, -, +, .
Summary: Incorrect keycodes for numpad /, -, +, .
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Sailesh Agrawal
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-12 10:16 PDT by Viet-Trung Luu
Modified: 2013-01-02 15:28 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.19 KB, patch)
2012-10-14 18:36 PDT, Sailesh Agrawal
no flags Details | Formatted Diff | Diff
Patch (13.31 KB, patch)
2012-10-16 15:09 PDT, Sailesh Agrawal
no flags Details | Formatted Diff | Diff
Patch (13.31 KB, patch)
2012-10-23 12:49 PDT, Sailesh Agrawal
no flags Details | Formatted Diff | Diff
Patch for landing (13.29 KB, patch)
2012-10-23 13:42 PDT, Sailesh Agrawal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Viet-Trung Luu 2012-10-12 10:16:21 PDT
(Okay, I didn't check them all. But the ones I did check were wrong. Safari gets them right.)

E.g., it should send VK_MULTIPLY (keycode 106) for the numpad *.

To check easily, just do something like

document.body.innerHTML = '<input onkeydown="console.log(window.event);">';

from the dev console (from about:blank, say), and type in the input box.

This affects Pepper plugins (and in particular Pepper Flash).
Comment 1 Viet-Trung Luu 2012-10-12 10:17:41 PDT
I should have pointed out that Safari gets it right.
Comment 2 Sailesh Agrawal 2012-10-12 10:19:16 PDT
Working on this.
Comment 3 Viet-Trung Luu 2012-10-12 10:29:29 PDT
Agh, everyone gets * correct.

Recent Safari and Chrome get - (probably / also, dunno about +, .) wrong.

Old Safari (5.1.7, on 10.6) gets - (and probably the others) right.
Comment 4 Viet-Trung Luu 2012-10-12 10:41:43 PDT
I confirmed that on Windows IE9 sends the numpad-specific keycodes for /, *, -, +, and . (when numlock is down).

Chrome gets /, -, +, and . wrong. (Old Safari gets these right.)
Comment 5 Sailesh Agrawal 2012-10-14 18:36:18 PDT
Created attachment 168604 [details]
Patch
Comment 6 Sailesh Agrawal 2012-10-14 18:40:53 PDT
For reference, here are the windows virtual key codes that changed
  decimal 110 -> 190
  plus 107 -> 187
  subtract 109 -> 189
  divide 111 -> 191

This regressed in http://trac.webkit.org/changeset/57951.
That change fixes key codes when the user has a different keyboard mapping (dvorak, azerty, etc..).

I don't think my change regress the original because those keyboard mappings don't modify the num pad keys.
Comment 7 Alexey Proskuryakov 2012-10-15 09:38:35 PDT
Please don't use [chromium] prefix on any bugs that touch cross-platform code. Such a prefix is a sign for non-chromium filks that they can ignore the bug.
Comment 8 Alexey Proskuryakov 2012-10-15 09:47:07 PDT
Comment on attachment 168604 [details]
Patch

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

The change makes sense.

Please add WebKitTestRunner and DumpRenderTree tests - there are similar ones already.

> Source/WebKit/chromium/ChangeLog:8
> +                In r57951 we switched to mapping keys by character code. This regressed the numpad keys which no longer match the Windows virtual key codes. This CL fixes this by never mapping numpad keys by character code.

Too many spaces here.
Comment 9 Sailesh Agrawal 2012-10-15 10:43:41 PDT
(In reply to comment #8)
> (From update of attachment 168604 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168604&action=review
> 
> The change makes sense.
> 
> Please add WebKitTestRunner and DumpRenderTree tests - there are similar ones already.

Hi Alexey, would you mind pointing out the existing tests? Sorry, I'm not very familiar with WebKit.

Thanks

> 
> > Source/WebKit/chromium/ChangeLog:8
> > +                In r57951 we switched to mapping keys by character code. This regressed the numpad keys which no longer match the Windows virtual key codes. This CL fixes this by never mapping numpad keys by character code.
> 
> Too many spaces here.
Comment 10 Alexey Proskuryakov 2012-10-15 11:28:39 PDT
See e.g. platform/mac/fast/events/non-roman-key-code.html. Depending on how widely you implement DumpRenderTree/WebKitTestRunner support for the additional key, the test can be either in platform/mac or just in fast/events.
Comment 11 Sailesh Agrawal 2012-10-16 15:09:54 PDT
Created attachment 169038 [details]
Patch
Comment 12 Sailesh Agrawal 2012-10-16 15:10:41 PDT
Added a layout test for platform/mac. Please take another look.
Comment 13 Sailesh Agrawal 2012-10-23 10:16:17 PDT
Alexey, Ping!
Comment 14 Tony Chang 2012-10-23 11:09:38 PDT
Comment on attachment 169038 [details]
Patch

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

> Source/WebKit/chromium/tests/WebInputEventFactoryTestMac.mm:47
> +struct KeyMappingEntry
> +{

Opening brace goes in the same line as struct.

> Source/WebKit/chromium/tests/WebInputEventFactoryTestMac.mm:120
> +    for (size_t i = 0; i < arraysize(table); ++i)
> +    {

Opening brace goes on the same line as 'for'.

> Tools/DumpRenderTree/mac/EventSendingController.mm:61
> +struct KeyMappingEntry
> +{

Opening brace goes on the same line as struct.

> Tools/DumpRenderTree/mac/EventSendingController.mm:739
> +    for (unsigned i = 0; i < WTF_ARRAY_LENGTH(table); i++) {

Nit: ++i
Comment 15 Alexey Proskuryakov 2012-10-23 11:56:16 PDT
Comment on attachment 169038 [details]
Patch

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

Looks good to me.

> Source/WebCore/ChangeLog:8
> +        In r57951 we switched to mapping keys by character code. This regressed the numpad keys which no longer match the Windows virtual key codes. This CL fixes this by never mapping numpad keys by character code.

This line is long, I think that it needs to be wrapped.

> LayoutTests/platform/mac/fast/events/numpad-keycode-mapping-expected.txt:9
> +PASS lastKeyboardEvent.keyCode is 110
> +PASS lastKeyboardEvent.keyCode is 106
> +PASS lastKeyboardEvent.keyCode is 107
> +PASS lastKeyboardEvent.keyCode is 12

Ideally, shouldBe test output should be self-descriptive. For example, if you change the tests to something like shouldBe("getKeyCode('.', 0x6E)",  "110"), the lines would not all look the same.

> LayoutTests/platform/mac/fast/events/numpad-keycode-mapping.html:19
> +    eventSender.keyDown(keyName, 0, 3);

Which one of these constants is keypad key location? Could you please define a named constant for it for clarity?
Comment 16 Sailesh Agrawal 2012-10-23 12:49:28 PDT
Created attachment 170207 [details]
Patch
Comment 17 Sailesh Agrawal 2012-10-23 12:50:36 PDT
Comment on attachment 169038 [details]
Patch

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

Addressed review comments. Please take another look.

>> Source/WebCore/ChangeLog:8
>> +        In r57951 we switched to mapping keys by character code. This regressed the numpad keys which no longer match the Windows virtual key codes. This CL fixes this by never mapping numpad keys by character code.
> 
> This line is long, I think that it needs to be wrapped.

Done.

>> Source/WebKit/chromium/tests/WebInputEventFactoryTestMac.mm:47
>> +{
> 
> Opening brace goes in the same line as struct.

Done.

>> Source/WebKit/chromium/tests/WebInputEventFactoryTestMac.mm:120
>> +    {
> 
> Opening brace goes on the same line as 'for'.

Done.

>> Tools/DumpRenderTree/mac/EventSendingController.mm:61
>> +{
> 
> Opening brace goes on the same line as struct.

Done.

>> Tools/DumpRenderTree/mac/EventSendingController.mm:739
>> +    for (unsigned i = 0; i < WTF_ARRAY_LENGTH(table); i++) {
> 
> Nit: ++i

Done.

>> LayoutTests/platform/mac/fast/events/numpad-keycode-mapping-expected.txt:9
>> +PASS lastKeyboardEvent.keyCode is 12
> 
> Ideally, shouldBe test output should be self-descriptive. For example, if you change the tests to something like shouldBe("getKeyCode('.', 0x6E)",  "110"), the lines would not all look the same.

Done.

>> LayoutTests/platform/mac/fast/events/numpad-keycode-mapping.html:19
>> +    eventSender.keyDown(keyName, 0, 3);
> 
> Which one of these constants is keypad key location? Could you please define a named constant for it for clarity?

Done.
Comment 18 Alexey Proskuryakov 2012-10-23 13:32:16 PDT
Comment on attachment 170207 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:8
> +        In r57951 we switched to mapping keys by character code. This regressed the numpad keys which no longer match the Windows virtual key codes. This CL fixes this by never mapping numpad keys by character code.

This line is equally long.

> LayoutTests/platform/mac/fast/events/numpad-keycode-mapping.html:42
> +    finishJSTest()

Didn't notice this at first. This is not an async test, and shouldn't need finishJSTest.
Comment 19 Sailesh Agrawal 2012-10-23 13:42:34 PDT
Created attachment 170224 [details]
Patch for landing
Comment 20 Sailesh Agrawal 2012-10-23 13:43:10 PDT
Comment on attachment 170207 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:8
>> +        In r57951 we switched to mapping keys by character code. This regressed the numpad keys which no longer match the Windows virtual key codes. This CL fixes this by never mapping numpad keys by character code.
> 
> This line is equally long.

Done.
Oops, fixed.

>> LayoutTests/platform/mac/fast/events/numpad-keycode-mapping.html:42
>> +    finishJSTest()
> 
> Didn't notice this at first. This is not an async test, and shouldn't need finishJSTest.

Done.
Comment 21 WebKit Review Bot 2012-10-24 15:36:41 PDT
Comment on attachment 170224 [details]
Patch for landing

Clearing flags on attachment: 170224

Committed r132411: <http://trac.webkit.org/changeset/132411>
Comment 22 WebKit Review Bot 2012-10-24 15:36:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Ryosuke Niwa 2013-01-02 15:20:59 PST
Did you forget to update WebKitTestRunner (WebKit2)?
Comment 24 Ryosuke Niwa 2013-01-02 15:21:22 PST
We’re seeing failure like this one:
http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r138643%20(4408)/platform/mac/fast/events/numpad-keycode-mapping-pretty-diff.html

--- /Volumes/Data/slave/mountainlion-release-tests-wk2/build/layout-test-results/platform/mac/fast/events/numpad-keycode-mapping-expected.txt
+++ /Volumes/Data/slave/mountainlion-release-tests-wk2/build/layout-test-results/platform/mac/fast/events/numpad-keycode-mapping-actual.txt
@@ -3,24 +3,24 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS getKeyCode('.') is 0x6E
-PASS getKeyCode('*') is 0x6A
-PASS getKeyCode('+') is 0x6B
-PASS getKeyCode('clear') is 0x0C
-PASS getKeyCode('/') is 0x6F
-PASS getKeyCode('enter') is 0x0D
-PASS getKeyCode('-') is 0x6D
-PASS getKeyCode('=') is 0xBB
-PASS getKeyCode('0') is 0x60
-PASS getKeyCode('1') is 0x61
-PASS getKeyCode('2') is 0x62
-PASS getKeyCode('3') is 0x63
-PASS getKeyCode('4') is 0x64
-PASS getKeyCode('5') is 0x65
-PASS getKeyCode('6') is 0x66
-PASS getKeyCode('7') is 0x67
-PASS getKeyCode('8') is 0x68
-PASS getKeyCode('9') is 0x69
+FAIL getKeyCode('.') should be 110. Was 65.
+FAIL getKeyCode('*') should be 106. Was 65.
+FAIL getKeyCode('+') should be 107. Was 65.
+FAIL getKeyCode('clear') should be 12. Was 65.
+FAIL getKeyCode('/') should be 111. Was 65.
+FAIL getKeyCode('enter') should be 13. Was 65.
+FAIL getKeyCode('-') should be 109. Was 65.
+FAIL getKeyCode('=') should be 187. Was 65.
+FAIL getKeyCode('0') should be 96. Was 48.
+FAIL getKeyCode('1') should be 97. Was 65.
+FAIL getKeyCode('2') should be 98. Was 65.
+FAIL getKeyCode('3') should be 99. Was 65.
+FAIL getKeyCode('4') should be 100. Was 65.
+FAIL getKeyCode('5') should be 101. Was 53.
+FAIL getKeyCode('6') should be 102. Was 65.
+FAIL getKeyCode('7') should be 103. Was 55.
+FAIL getKeyCode('8') should be 104. Was 65.
+FAIL getKeyCode('9') should be 105. Was 57.
 PASS successfullyParsed is true
 
 TEST COMPLETE
Comment 25 Sailesh Agrawal 2013-01-02 15:27:16 PST
> Did you forget to update WebKitTestRunner (WebKit2)?

Hi Ryosuke. Looks like I did forget, sorry about that.

It looks like I just have to port my change from EventSendingController.mm to Tools/WebKitTestRunner/mac/EventSenderProxy.mm

Working on that right now. Thanks.
Comment 26 Ryosuke Niwa 2013-01-02 15:28:53 PST
(In reply to comment #25)
> > Did you forget to update WebKitTestRunner (WebKit2)?
> 
> Hi Ryosuke. Looks like I did forget, sorry about that.
> 
> It looks like I just have to port my change from EventSendingController.mm to Tools/WebKitTestRunner/mac/EventSenderProxy.mm
> 
> Working on that right now. Thanks.

Thanks. Note that I've filed https://bugs.webkit.org/show_bug.cgi?id=105958.