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
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>
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>
Mass move bugs into the DOM component.