Bug 85642 - keydown and keyup events have zero keycode for some numeric pad keys under Chromium on Linux
: keydown and keyup events have zero keycode for some numeric pad keys under Ch...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML Events
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To: wez
:
Depends on: 110856
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-04 10:47 PDT by wez
Modified: 2013-03-05 14:46 PST (History)
13 users (show)

See Also:


Attachments
Patch (1.59 KB, patch)
2012-05-04 13:35 PDT, wez
no flags Details | Formatted Diff | Diff
Patch (5.96 KB, patch)
2013-02-25 15:56 PST, wez
no flags Details | Formatted Diff | Diff
Patch (6.00 KB, patch)
2013-02-26 10:00 PST, wez
no flags Details | Formatted Diff | Diff
Patch (5.92 KB, patch)
2013-02-27 13:25 PST, wez
no flags Details | Formatted Diff | Diff
Patch (5.99 KB, patch)
2013-03-04 12:16 PST, wez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description wez 2012-05-04 10:47:55 PDT
The keys marked "5", "0"/"Ins" and "."/"Del" generate keydown and keyup events with zero keycode values under Chromium on Linux.  These keys have valid keycodes under Chromium on Windows.

See crbug.com/124755 in the Chromium tracker.
Comment 1 wez 2012-05-04 13:35:42 PDT
Created attachment 140307 [details]
Patch
Comment 2 Ryosuke Niwa 2012-05-10 00:46:15 PDT
Comment on attachment 140307 [details]
Patch

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

Can we add a unit test for this?

> Source/WebCore/platform/chromium/KeyCodeConversionGtk.cpp:92
> +    case GDK_KP_Begin:
> +        return VKEY_CLEAR; // (12) CLEAR key

I don't get this. How is that GDK_KP_Begin maps to VKEY_CLEAR?
Various search on google tells me that GDK_KP_Begin is VKEY_NUMPAD5 when numlock is on and undefined otherwise.
Comment 3 Tony Chang 2012-05-10 11:02:48 PDT
Comment on attachment 140307 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This patch adds mappings for GDK_KP_Begin, GDK_KP_Insert and GDK_KP_Delete.  There are no keycode conversion routine tests, so no tests are updated.

You should be able to add a gtest to webkit_unit_tests for this.  windowsKeyCodeForKeyEvent is just a function that you can call directly.
Comment 4 Ken 2012-09-06 11:35:41 PDT
Wez's patch is valid and fixes a serious problem on Linux.

Even if one has a problem with the "case GDK_KP_Begin" the other two entries are undeniably needed.  Every windows keyboard has these buttons, and they are used by people everywhere.

case GDK_KP_Insert:
    return VKEY_INSERT; // (45) INS key
case GDK_KP_Delete:
    return VKEY_DELETE; // (46) DEL key

I'm a bit of a noob here, but if somebody can point me to instructions on how to be a good citizen here, I'll make the fix myself.
Comment 5 wez 2012-09-06 11:46:23 PDT
(In reply to comment #4)
> Wez's patch is valid and fixes a serious problem on Linux.
> 
> Even if one has a problem with the "case GDK_KP_Begin" the other two entries are undeniably needed.  Every windows keyboard has these buttons, and they are used by people everywhere.
> 
> case GDK_KP_Insert:
>     return VKEY_INSERT; // (45) INS key
> case GDK_KP_Delete:
>     return VKEY_DELETE; // (46) DEL key
> 
> I'm a bit of a noob here, but if somebody can point me to instructions on how to be a good citizen here, I'll make the fix myself.

The hold up here is that I've not got around to adding unit-tests for windowsKeyCodeForEvent(), which currently has no tests.
Comment 6 wez 2013-02-25 15:56:04 PST
Created attachment 190144 [details]
Patch
Comment 7 wez 2013-02-25 15:57:00 PST
(In reply to comment #3)
> (From update of attachment 140307 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140307&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        This patch adds mappings for GDK_KP_Begin, GDK_KP_Insert and GDK_KP_Delete.  There are no keycode conversion routine tests, so no tests are updated.
> 
> You should be able to add a gtest to webkit_unit_tests for this.  windowsKeyCodeForKeyEvent is just a function that you can call directly.

I've added a (Linux-only) test under Source/WebKit/chromium/tests.
Comment 8 WebKit Review Bot 2013-02-26 00:34:23 PST
Comment on attachment 190144 [details]
Patch

Clearing flags on attachment: 190144

Committed r144016: <http://trac.webkit.org/changeset/144016>
Comment 9 WebKit Review Bot 2013-02-26 00:34:27 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 WebKit Review Bot 2013-02-26 00:58:45 PST
Re-opened since this is blocked by bug 110856
Comment 11 Vsevolod Vlasov 2013-02-26 01:01:12 PST
Breaks compilation on chromium mac:

ce/WebKit/chromium/tests/KeyCodeConversionTest.cpp:36:10: fatalerror: 'gdk/gdkkeysyms.h' file not found [1]
 #include <gdk/gdkkeysyms.h>
          ^
1 error generated.
Comment 12 Vsevolod Vlasov 2013-02-26 01:02:47 PST
And on chromium os as well
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Builder/builds/7367/steps/compile/logs/stdio

third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp:36:28:warning: gdk/gdkkeysyms.h: No such file or directory
third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp: In member function 'virtual void<unnamed>::KeyCodeConversionTest_KeyPadClear_Test::TestBody()':
third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp:46:error: 'GDK_KP_Begin' was not declared in this scope
third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp: In member function 'virtual void<unnamed>::KeyCodeConversionTest_KeyPadInsert_Test::TestBody()':
third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp:51:error: 'GDK_Insert' was not declared in this scope
third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp:51:error: template argument 1 is invalid
third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp:51:error: 'GDK_KP_Begin' was not declared in this scope
third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp: In member function 'virtual void<unnamed>::KeyCodeConversionTest_KeyPadDelete_Test::TestBody()':
third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp:57:error: 'GDK_Delete' was not declared in this scope
third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp:57:error: template argument 1 is invalid
third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp:57:error: 'GDK_KP_Delete' was not declared in this scope
third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp: In member function 'virtual void<unnamed>::KeyCodeConversionTest_AltGr_Test::TestBody()':
third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp:63:error: 'GDK_Alt_R' was not declared in this scope
third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp:63:error: template argument 1 is invalid
third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.cpp:63:error: 'GDK_ISO_Level3_Shift' was not declared in this scope
make: *** [out/Release/obj.target/webkit_unit_tests/third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTest.o] Error 1
Comment 13 wez 2013-02-26 10:00:35 PST
Created attachment 190318 [details]
Patch
Comment 14 Peter Beverloo (cr-android ews) 2013-02-27 02:21:39 PST
Comment on attachment 190318 [details]
Patch

Attachment 190318 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/16789118
Comment 15 wez 2013-02-27 13:12:11 PST
(In reply to comment #14)
> (From update of attachment 190318 [details])
> Attachment 190318 [details] did not pass cr-android-ews (chromium-android):
> Output: http://webkit-commit-queue.appspot.com/results/16789118

Gah, my bad - didn't realise that *Gtk* files aren't automatically omitted on non-GTK.  Will fix and re-upload.  Apologies for the noise...
Comment 16 wez 2013-02-27 13:25:49 PST
Created attachment 190591 [details]
Patch
Comment 17 wez 2013-03-01 19:44:04 PST
(In reply to comment #16)
> Created an attachment (id=190591) [details]
> Patch

Can someone review & CQ this, please?
Comment 18 Adam Barth 2013-03-02 23:34:29 PST
Comment on attachment 190591 [details]
Patch

Looks great.  Thanks for the test.
Comment 19 WebKit Review Bot 2013-03-03 00:48:05 PST
Comment on attachment 190591 [details]
Patch

Clearing flags on attachment: 190591

Committed r144562: <http://trac.webkit.org/changeset/144562>
Comment 20 WebKit Review Bot 2013-03-03 00:48:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Rafael Weinstein 2013-03-04 10:49:51 PST
This appears to be causing KeyPadInsert failures in webkit_unittests. e.g.

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/6820

log output:

(view as text)
KeyCodeConversionTest.KeyPadInsert: 
../../third_party/WebKit/Source/WebKit/chromium/tests/KeyCodeConversionTestGtk.cpp:51: Failure
Value of: windowsKeyCodeForKeyEvent(0xff9d)
Actual: 12
Expected: windowsKeyCodeForKeyEvent(0xff63)
Which is: 45
Comment 22 Tony Chang 2013-03-04 11:06:22 PST
Reverted r144562 for reason:

Caused KeyPadInsert faluires in webkit_unittests

Committed r144649: <http://trac.webkit.org/changeset/144649>
Comment 23 wez 2013-03-04 12:16:50 PST
Created attachment 191287 [details]
Patch
Comment 24 wez 2013-03-04 12:19:31 PST
Apologies for the fail caused by the previous patch-set, which had a copy/paste typo I must have introduced just before uploading. It seems that EWS doesn't run the new test, so it wasn't caught. :(
Comment 25 wez 2013-03-05 14:13:29 PST
Once again, CQ Powerz required. :)
Comment 26 WebKit Review Bot 2013-03-05 14:46:03 PST
Comment on attachment 191287 [details]
Patch

Clearing flags on attachment: 191287

Committed r144821: <http://trac.webkit.org/changeset/144821>
Comment 27 WebKit Review Bot 2013-03-05 14:46:08 PST
All reviewed patches have been landed.  Closing bug.