Bug 221456 – [GTK] event.ctrlKey and other are false in keydown event

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

Created attachment 419376 [details]
Test case

Check the attached example and type things like Ctrl or Alt. The event.ctrlKey or event.altKey will be false.

This works fine on Mac and other browsers.

[Manuel Rego Casasnovas]

Created attachment 419387 [details]
Patch

[Adrian Perez]

Comment on attachment 419387 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419387&action=review

> Source/WebKit/Shared/gtk/WebEventFactory.cpp:70
> +    if (keyboardEvent) {

When ”keyboardEvent” is true, the event type will always be “GDK_KEY_PRESS” or
“GDK_KEY_RELEASE”; so instead of adding this ”keyboardEvent” flag to the function
signature, I think we can directly check for “GDK_KEY_PRESS” instead:

  if (gdk_event_get_event_type(const_cast<GdkEvent*>(event)) == GDK_KEY_PRESS) {
      guint keyval;
      gdk_event_get_keyval(event, &keyval);
      switch (keyval) {
          // ...
      }
  }

  return modifiers;

…or is there something else I am missing here?

> Source/WebKit/Shared/gtk/WebEventFactory.cpp:340
> +

Please remove this stray empty line before landing.

[Manuel Rego Casasnovas]

Created attachment 419402 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 419387 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419387&action=review

>> Source/WebKit/Shared/gtk/WebEventFactory.cpp:70
>> +    if (keyboardEvent) {
> 
> When ”keyboardEvent” is true, the event type will always be “GDK_KEY_PRESS” or
> “GDK_KEY_RELEASE”; so instead of adding this ”keyboardEvent” flag to the function
> signature, I think we can directly check for “GDK_KEY_PRESS” instead:
> 
>   if (gdk_event_get_event_type(const_cast<GdkEvent*>(event)) == GDK_KEY_PRESS) {
>       guint keyval;
>       gdk_event_get_keyval(event, &keyval);
>       switch (keyval) {
>           // ...
>       }
>   }
> 
>   return modifiers;
> 
> …or is there something else I am missing here?

True thing, the first version of this method was not getting the event.

I'll upload a new version doing that next week. Thanks for the review.

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Manuel Rego Casasnovas]

Created attachment 419560 [details]
Patch

[Manuel Rego Casasnovas]

Created attachment 419561 [details]
Patch

[Manuel Rego Casasnovas]

Applied the suggested changes and added a test. PTAL, thanks!

[Adrian Perez]

Comment on attachment 419561 [details]
Patch

Very nice that the patch comes now with a layout test, thanks for that!

[EWS]

Committed r272489: <https://commits.webkit.org/r272489>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419561 [details].