WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Gustavo Noronha (kov)
Comment 1
2017-01-06 05:58:36 PST
Created
attachment 298199
[details]
Patch
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
Committed
r210504
: <
http://trac.webkit.org/changeset/210504
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug