Bug 85642 – keydown and keyup events have zero keycode for some numeric pad keys under Chromium on Linux

• Component: DOM
• Status: RESOLVED FIXED

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.

Created attachment 140307 [details]
Patch

[Ryosuke Niwa]

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.

[Tony Chang]

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.

[Ken]

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.

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

Created attachment 190144 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 190144 [details]
Patch

Clearing flags on attachment: 190144

Committed r144016: <http://trac.webkit.org/changeset/144016>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 110856

[Vsevolod Vlasov]

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.

[Vsevolod Vlasov]

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

Created attachment 190318 [details]
Patch

[Peter Beverloo (cr-android ews)]

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

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

Created attachment 190591 [details]
Patch

(In reply to comment #16)
> Created an attachment (id=190591) [details]
> Patch

Can someone review & CQ this, please?

[Adam Barth]

Comment on attachment 190591 [details]
Patch

Looks great.  Thanks for the test.

[WebKit Review Bot]

Comment on attachment 190591 [details]
Patch

Clearing flags on attachment: 190591

Committed r144562: <http://trac.webkit.org/changeset/144562>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rafael Weinstein]

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

[Tony Chang]

Reverted r144562 for reason:

Caused KeyPadInsert faluires in webkit_unittests

Committed r144649: <http://trac.webkit.org/changeset/144649>

Created attachment 191287 [details]
Patch

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

Once again, CQ Powerz required. :)

[WebKit Review Bot]

Comment on attachment 191287 [details]
Patch

Clearing flags on attachment: 191287

Committed r144821: <http://trac.webkit.org/changeset/144821>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

Mass move bugs into the DOM component.