RESOLVED FIXED 166759
[GTK] Should support key and code properties on keyboard events
https://bugs.webkit.org/show_bug.cgi?id=166759
Summary [GTK] Should support key and code properties on keyboard events
Gustavo Noronha (kov)
Reported 2017-01-06 05:50:26 PST
[GTK] Should support key and code properties on keyboard events
Attachments
Patch (36.06 KB, patch)
2017-01-06 05:58 PST, Gustavo Noronha (kov)
cgarcia: review+
Gustavo Noronha (kov)
Comment 1 2017-01-06 05:58:36 PST
Michael Catanzaro
Comment 2 2017-01-06 10:15:23 PST
Comment on attachment 298199 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298199&action=review Awesome, thanks. :) Do you have any thoughts on not-very-related bug #136430? You probably looked over the code where we can set that GdkEventKey::send_event? Anyway, I guess Carlos will want to review this too. My $0.02: > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:395 > + memset(&utf8, 0, sizeof(utf8)); No need for memset, write char utf8[7] = { 0 }; > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:396 > + g_unichar_to_utf8(unicodeCharacter, reinterpret_cast<gchar*>(&utf8)); Don't cast from char* to gchar*, they are the same type. :P
Michael Catanzaro
Comment 3 2017-01-06 10:17:41 PST
(In reply to comment #2) > > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:396 > > + g_unichar_to_utf8(unicodeCharacter, reinterpret_cast<gchar*>(&utf8)); > > Don't cast from char* to gchar*, they are the same type. :P Sorry, I see it's a cast from char** to gchar*. Hm, why is this? Why do you need a cast there at all?
Carlos Garcia Campos
Comment 4 2017-01-07 00:53:01 PST
Comment on attachment 298199 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298199&action=review > Source/WebCore/ChangeLog:14 > + - fast/events/arrow-keys-on-body.html > + - fast/events/constructors/keyboard-event-constructor.html > + - fast/events/key-events-in-input-button.html > + - fast/events/key-events-in-input-text.html > + - fast/events/keyboardevent-code.html > + - fast/events/keyboardevent-key.html Shouldn't we unskip them? or are all these currently failing? > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:124 > + return "Backspace"; Why not ASCIILiteral here too? (and below) > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:727 > // FIXME: This is incomplete. We should change this to mirror > // more like what Firefox does, and generate these switch statements > // at build time. This FIXME applies to to the new methods too, I think we should autogenerate all these long switches. Not necessarily in this patch, though.
Gustavo Noronha (kov)
Comment 5 2017-01-09 04:03:36 PST
(In reply to comment #2) > Do you have any thoughts on not-very-related bug #136430? You probably > looked over the code where we can set that GdkEventKey::send_event? Unfortunately no idea =( > > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:395 > > + memset(&utf8, 0, sizeof(utf8)); > > No need for memset, write char utf8[7] = { 0 }; > > > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:396 > > + g_unichar_to_utf8(unicodeCharacter, reinterpret_cast<gchar*>(&utf8)); > > Don't cast from char* to gchar*, they are the same type. :P Oops, yeah, I changed that code quite a bit and it ended up very weird. Thanks!
Gustavo Noronha (kov)
Comment 6 2017-01-09 04:05:33 PST
(In reply to comment #4) > > Source/WebCore/ChangeLog:14 > > + - fast/events/arrow-keys-on-body.html > > + - fast/events/constructors/keyboard-event-constructor.html > > + - fast/events/key-events-in-input-button.html > > + - fast/events/key-events-in-input-text.html > > + - fast/events/keyboardevent-code.html > > + - fast/events/keyboardevent-key.html > > Shouldn't we unskip them? or are all these currently failing? Currently failing. I thought about skipping and unskipping on this patch but I thought it would go in quick enough =) > > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:124 > > + return "Backspace"; > > Why not ASCIILiteral here too? (and below) Ooops, my mistake. I left some of them for a search-and-replace later on and forgot to do it. > > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:727 > > // FIXME: This is incomplete. We should change this to mirror > > // more like what Firefox does, and generate these switch statements > > // at build time. > > This FIXME applies to to the new methods too, I think we should autogenerate > all these long switches. Not necessarily in this patch, though. Agreed. I'll take a look at how Firefox does it.
Gustavo Noronha (kov)
Comment 7 2017-01-09 06:16:04 PST
Note You need to log in before you can comment on or make changes to this bug.