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
: WebKit
HTML Events
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
:
: 110856
:
  Show dependency treegraph
 
Reported: 2012-05-04 10:47 PST by
Modified: 2013-03-05 14:46 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-04 10:47:55 PST
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 From 2012-05-04 13:35:42 PST -------
Created an attachment (id=140307) [details]
Patch
------- Comment #2 From 2012-05-10 00:46:15 PST -------
(From update of attachment 140307 [details])
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 From 2012-05-10 11:02:48 PST -------
(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.
------- Comment #4 From 2012-09-06 11:35:41 PST -------
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 From 2012-09-06 11:46:23 PST -------
(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 From 2013-02-25 15:56:04 PST -------
Created an attachment (id=190144) [details]
Patch
------- Comment #7 From 2013-02-25 15:57:00 PST -------
(In reply to comment #3)
> (From update of attachment 140307 [details] [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 From 2013-02-26 00:34:23 PST -------
(From update of attachment 190144 [details])
Clearing flags on attachment: 190144

Committed r144016: <http://trac.webkit.org/changeset/144016>
------- Comment #9 From 2013-02-26 00:34:27 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #10 From 2013-02-26 00:58:45 PST -------
Re-opened since this is blocked by bug 110856
------- Comment #11 From 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 From 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 From 2013-02-26 10:00:35 PST -------
Created an attachment (id=190318) [details]
Patch
------- Comment #14 From 2013-02-27 02:21:39 PST -------
(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
------- Comment #15 From 2013-02-27 13:12:11 PST -------
(In reply to comment #14)
> (From update of attachment 190318 [details] [details])
> Attachment 190318 [details] [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 From 2013-02-27 13:25:49 PST -------
Created an attachment (id=190591) [details]
Patch
------- Comment #17 From 2013-03-01 19:44:04 PST -------
(In reply to comment #16)
> Created an attachment (id=190591) [details] [details]
> Patch

Can someone review & CQ this, please?
------- Comment #18 From 2013-03-02 23:34:29 PST -------
(From update of attachment 190591 [details])
Looks great.  Thanks for the test.
------- Comment #19 From 2013-03-03 00:48:05 PST -------
(From update of attachment 190591 [details])
Clearing flags on attachment: 190591

Committed r144562: <http://trac.webkit.org/changeset/144562>
------- Comment #20 From 2013-03-03 00:48:10 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #21 From 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 From 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 From 2013-03-04 12:16:50 PST -------
Created an attachment (id=191287) [details]
Patch
------- Comment #24 From 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 From 2013-03-05 14:13:29 PST -------
Once again, CQ Powerz required. :)
------- Comment #26 From 2013-03-05 14:46:03 PST -------
(From update of attachment 191287 [details])
Clearing flags on attachment: 191287

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