Bug 99188 – Incorrect keycodes for numpad /, -, +, .

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Viet-Trung Luu]

(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).

[Viet-Trung Luu]

I should have pointed out that Safari gets it right.

[Sailesh Agrawal]

Working on this.

[Viet-Trung Luu]

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.

[Viet-Trung Luu]

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.)

[Sailesh Agrawal]

Created attachment 168604 [details]
Patch

[Sailesh Agrawal]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Sailesh Agrawal]

(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.

[Alexey Proskuryakov]

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.

[Sailesh Agrawal]

Created attachment 169038 [details]
Patch

[Sailesh Agrawal]

Added a layout test for platform/mac. Please take another look.

[Sailesh Agrawal]

Alexey, Ping!

[Tony Chang]

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

[Alexey Proskuryakov]

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?

[Sailesh Agrawal]

Created attachment 170207 [details]
Patch

[Sailesh Agrawal]

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.

[Alexey Proskuryakov]

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.

[Sailesh Agrawal]

Created attachment 170224 [details]
Patch for landing

[Sailesh Agrawal]

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.

[WebKit Review Bot]

Comment on attachment 170224 [details]
Patch for landing

Clearing flags on attachment: 170224

Committed r132411: <http://trac.webkit.org/changeset/132411>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Did you forget to update WebKitTestRunner (WebKit2)?

[Ryosuke Niwa]

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

[Sailesh Agrawal]

> 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.

[Ryosuke Niwa]

(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.