| Summary: | keydown and keyup events have zero keycode for some numeric pad keys under Chromium on Linux | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | wez | ||||||||||||
| Component: | DOM | Assignee: | wez | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | dcheng, dglazkov, japhet, ken.demarest, k.y.sergey, levin, ojan, peter+ews, rafaelw, rniwa, tony, vsevik, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Linux | ||||||||||||||
| Bug Depends on: | 110856 | ||||||||||||||
| Bug Blocks: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
wez
2012-05-04 10:47:55 PDT
Created attachment 140307 [details]
Patch
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 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. 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. Comment on attachment 190144 [details] Patch Clearing flags on attachment: 190144 Committed r144016: <http://trac.webkit.org/changeset/144016> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 110856 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.
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
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? Comment on attachment 190591 [details]
Patch
Looks great. Thanks for the test.
Comment on attachment 190591 [details] Patch Clearing flags on attachment: 190591 Committed r144562: <http://trac.webkit.org/changeset/144562> All reviewed patches have been landed. Closing bug. 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 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. :) Comment on attachment 191287 [details] Patch Clearing flags on attachment: 191287 Committed r144821: <http://trac.webkit.org/changeset/144821> All reviewed patches have been landed. Closing bug. Mass move bugs into the DOM component. |