Bug 35432 - [Gtk] Make DRT EventSender::keyDown to consider 'hardware_keycode' field when synthesizing an event
Summary: [Gtk] Make DRT EventSender::keyDown to consider 'hardware_keycode' field when...
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords: Gtk
Depends on:
Blocks: 18662 30587
  Show dependency treegraph
 
Reported: 2010-02-26 08:16 PST by Antonio Gomes
Modified: 2010-04-20 13:24 PDT (History)
5 users (show)

See Also:


Attachments
patch 0.1 (3.20 KB, patch)
2010-02-26 08:32 PST, Antonio Gomes
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
patch 0.2 (3.31 KB, patch)
2010-02-26 09:26 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-02-26 08:16:05 PST
When synthesizing an event in Gtk's DRT EventSender keydownCallback implementation, an invalid 'hardware_keycode' value
can cause it to be badly processed by Gtk+.

Scenarios:

1) (most of current layout tests fall in this category)
* Focused node an editor (e.g. <input type=text> , <textarea>, etc).
* EventSender::keyDown("arrowDown") is called from JS.
* webkit_web_view_key_press is called and the event is processed by WebCore.

bool webkit_web_view_key_press_event (...)
(...)
  if (frame->eventHandler()->keyEvent(keyboardEvent))
    return TRUE;
(...)


2) On the other hand, in spatial navigation tests in bug 18662 ...

* Focused node is a link (i.e. <a href="xxx">)
* EventSender::keyDown("arrowDown") is called from JS.
* webkit_web_view_key_press does not proccess the event and fallback to GTK+

bool webkit_web_view_key_press_event(...)
(...)
  /* Chain up to our parent class for binding activation */
  return GTK_WIDGET_CLASS(webkit_web_view_parent_class)->key_press_event(widget, event);


|gtk_bindings_activate_event| fails at some point because hardware_keycode is not provided.


patch coming ...
Comment 1 Antonio Gomes 2010-02-26 08:32:44 PST
Created attachment 49588 [details]
patch 0.1
Comment 2 WebKit Review Bot 2010-02-26 08:37:58 PST
Attachment 49588 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/gtk/EventSender.cpp:565:  Declaration has space between type name and * in GdkKeymapKey *keys  [whitespace/declaration] [3]
WebKitTools/DumpRenderTree/gtk/EventSender.cpp:567:  Extra space before ( in function call  [whitespace/parens] [4]
WebKitTools/DumpRenderTree/gtk/EventSender.cpp:569:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Antonio Gomes 2010-02-26 09:26:46 PST
Created attachment 49591 [details]
patch 0.2

Fixed the style issue.
Comment 4 Gustavo Noronha (kov) 2010-02-26 10:21:18 PST
Comment on attachment 49591 [details]
patch 0.2

> diff --git a/WebKitTools/DumpRenderTree/gtk/EventSender.cpp b/WebKitTools/DumpRenderTree/gtk/EventSender.cpp
> index 458e0ba..88330d2 100644
> --- a/WebKitTools/DumpRenderTree/gtk/EventSender.cpp
> +++ b/WebKitTools/DumpRenderTree/gtk/EventSender.cpp
> @@ -559,6 +559,16 @@ static JSValueRef keyDownCallback(JSContextRef context, JSObjectRef function, JS
>  
>      gboolean return_val;
>      event.key.type = GDK_KEY_PRESS;
> +
> +    // When synthesizing an event, an invalid hardware_keycode value
> +    // can cause it to be badly processed by Gtk+.
> +    GdkKeymapKey* keys;
> +    gint n_keys;
> +    if (gdk_keymap_get_entries_for_keyval(gdk_keymap_get_default(), gdkKeySym, &keys, &n_keys)) {
> +        event.key.hardware_keycode = keys[0].keycode;
> +        g_free(keys);
> +    }
> +

I suggest moving this code to before the gboolean return_val declaration, just so it is close to the rest of the general initializations (which are done for both the key press, and key release events).

This looks good to me, otherwise. Please check if we could enable a layout test with this, though. Would be good to be sure this is working before you land the spatial navigation patch. Thanks!
Comment 5 Antonio Gomes 2010-02-26 13:48:35 PST
r55309
Comment 6 Antonio Gomes 2010-04-11 20:01:00 PDT
Comment on attachment 49591 [details]
patch 0.2

Clearing flags on attachment: 49591

Committed r55309: <http://trac.webkit.org/changeset/55309>