Bug 49927 – [GTK] Windowless plugins gets broken key input event (at least in Flash)

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Nicolas Dufresne]

Visiting this test site:

http://people.collabora.co.uk/~nicolas/public/flash/window-less-editor.html

with any WebKitGTK based browers shows that key event are not correctly sent to the plugin. As an example typing J result in / being received.

[Nicolas Dufresne]

Created attachment 74606 [details]
Windowless plugins gets broken key input

When converting the GdkEvent to XEvent we need to send the
hardware_keycode as XEvent::keycode. Instead we where sending
the GdkEvent::keyval.

[Martin Robinson]

In TestNetscapePlugin there are some pretty glaring NOTIMPLEMENTED lines for keyDown and keyUp events for X11 plugins. Perhaps implementing these and providing a test (or updating results) might prevent bugs like this in the future. Do you mind checking whether or not it's feasible to do that in this patch?

[Xan Lopez]

(In reply to comment #1)
> Created an attachment (id=74606) [details]
> Windowless plugins gets broken key input
> 
> When converting the GdkEvent to XEvent we need to send the
> hardware_keycode as XEvent::keycode. Instead we where sending
> the GdkEvent::keyval.

I think we might be missing something, because the original code was also sending bogus values and when we changed it to the current one we started to get correct results in the layout tests. At the very least we should have an explanation of why this is the right thing to do and a test for it.

[Xan Lopez]

Comment on attachment 74606 [details]
Windowless plugins gets broken key input

r- for lack of test.

[Nicolas Dufresne]

(In reply to comment #2)
> In TestNetscapePlugin there are some pretty glaring NOTIMPLEMENTED lines for keyDown and keyUp events for X11 plugins. Perhaps implementing these and providing a test (or updating results) might prevent bugs like this in the future. Do you mind checking whether or not it's feasible to do that in this patch?

Yeah, I'm working on this now. Just for testing, I've hardcoded the keycode to 0 and all the test passed because of the NOTIMPLEMENTED mark. It's very easy to implement correctly, but then I discovered that there is other bug in the test code (seems to be in the EventSender.cpp).

Obviously this has been changed multiple time without ever being tested ;)

[Nicolas Dufresne]

(In reply to comment #3)
> I think we might be missing something, because the original code was also sending bogus values and when we changed it to the current one we started to get correct results in the layout tests. At the very least we should have an explanation of why this is the right thing to do and a test for it.

As said, any change to this line have zero effect on the LayoutTests, you might be confusing with the effect of the other changes found in http://trac.webkit.org/changeset/66827.

But one thing is sure, you can't put a keysym into the keycode field of XEvent.
And if you look at the GTK code you'll see that hardware_keycode is the original XEvent::keycode.

[Xan Lopez]

(In reply to comment #6)
> As said, any change to this line have zero effect on the LayoutTests, you might be confusing with the effect of the other changes found in http://trac.webkit.org/changeset/66827.
> 
> But one thing is sure, you can't put a keysym into the keycode field of XEvent.
> And if you look at the GTK code you'll see that hardware_keycode is the original XEvent::keycode.

plugins/keyboard-events.html fails here with your patch in exactly the same way that used to fail before 66827.

[Xan Lopez]

(In reply to comment #7)
> (In reply to comment #6)
> > As said, any change to this line have zero effect on the LayoutTests, you might be confusing with the effect of the other changes found in http://trac.webkit.org/changeset/66827.
> > 
> > But one thing is sure, you can't put a keysym into the keycode field of XEvent.
> > And if you look at the GTK code you'll see that hardware_keycode is the original XEvent::keycode.
> 
> plugins/keyboard-events.html fails here with your patch in exactly the same way that used to fail before 66827.

If you are not seeing this I guess it might somehow be system specific (perhaps differences in layouts?), but for sure that's the only part of the patch that could make any difference wrt this. If hardware_keycode is the actual hardware key pressed and the keyval already shows at least a translation layer depending on the layout it would make sense there are differences between machines, but then IMHO the test should be ideally changed to take this into account.

[Nicolas Dufresne]

I don't know what's wrong with my build, but now I get the same test failure has Xan (while I'm pretty sure it's a false negative). Here's the explanation:

The event->keyval is exactly like the X11 keysym. One important thing to know about those keysym is that they match the ASCII code of basic keys like a, b and c.

Currently we where sending the plugin an XEvent with keycode set to the keysym. Then, in the test plugin, we where printing the XEvent keycode just like if it was a char. As the keysym value is the character we wanted to print, the test pass (false positive).

Will upload a new patch soon ...

[Nicolas Dufresne]

Created attachment 74710 [details]
Also Fix test plugin

This version of the patch also fix the test plugin not to assume that a keycode is a valid ASCII code. This should get the test to work again.

[Xan Lopez]

(In reply to comment #10)
> Created an attachment (id=74710) [details]
> Also Fix test plugin
> 
> This version of the patch also fix the test plugin not to assume that a keycode is a valid ASCII code. This should get the test to work again.

This seems sensible, but why was it working in the other ports?

[Martin Robinson]

Comment on attachment 74710 [details]
Also Fix test plugin

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

Just a couple nits.

> WebKitTools/DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp:266
> +keycode_to_char(XKeyEvent *event)

I think this should be called keyEventToChar since it actually takes an XKeyEvent and not a keycode. Also, new code should follow WebKit style (event though the rest of the file doesn't).

[Martin Robinson]

Nicolas, can you build a Qt build to verify that this test doesn't fail with your change?

[Nicolas Dufresne]

(In reply to comment #13)
> Nicolas, can you build a Qt build to verify that this test doesn't fail with your change?

Run the test and it indeed break Qt test. After investigation, they have code that runs only in the DRT that bogusly fix the test. I mention that to Andreas Kling and he told me not to care about it, that we should push the changes, he will fix it afterward.

I'll send a new patch fixing the naming error you found soon.

[Nicolas Dufresne]

Created attachment 74820 [details]
Fixed function naming

Same patch with the naming fix mentions before.

[Martin Robinson]

Comment on attachment 74820 [details]
Fixed function naming

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

Looks really good, though there are still a couple small style issues. Sorry that I was not explicit about them before.

> WebKitTools/DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp:266
> +static char
> +keyEventToChar(XKeyEvent *event)

This should be one line and the asterisk should be next to the type name here.

[Nicolas Dufresne]

Created attachment 74824 [details]
Fix function implementation style

I must say, it's very hard to notice those errors when all the surrounding code is consistently doing differently ;) Hopefully this is the one.

[Nicolas Dufresne]

Comment on attachment 74824 [details]
Fix function implementation style

Manipulation error, sorry

[Nicolas Dufresne]

Created attachment 74825 [details]
Fix remaining style issues

Hopefully this is the last one.

[Martin Robinson]

Comment on attachment 74825 [details]
Fix remaining style issues

Great! Thanks.

[WebKit Commit Bot]

Comment on attachment 74825 [details]
Fix remaining style issues

Clearing flags on attachment: 74825

Committed r72717: <http://trac.webkit.org/changeset/72717>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/72717 might have broken Qt Linux Release
The following tests are not passing:
plugins/keyboard-events.html

[Martin Robinson]

As we expected this is failing on Qt. I will skip the test and open a bug.