Bug 36147 – Give keyboard focus to PluginDocuments by default

• Component: New Bugs
• Status: RESOLVED FIXED

[John Abd-El-Malek]

Give keyboard focus to PluginDocuments[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dplugins in PluginDocuments by default

[John Abd-El-Malek]

Created attachment 50751 [details]
Patch

[John Abd-El-Malek]

This is needed for Pepper plugins.

I've spent two days trying to write a test for this with no luck.  Trying to call "eventSender.keyDown()" from a plugin in a window returned from window.open always sends the event to the window opener.

[Eric Seidel (no email)]

Filed bug 36149.

[John Abd-El-Malek]

actually, I found the bug in chromium's test_shell.  I have a fix (in chrome's tree).  I'll try to run the test case on mac.

[John Abd-El-Malek]

Created attachment 50767 [details]
Patch

[John Abd-El-Malek]

Created attachment 50769 [details]
Patch

[John Abd-El-Malek]

Created attachment 50770 [details]
Patch

[Ojan Vafai]

Why all the additions to the skipped lists? Generally, we avoid adding to the skipped lists unless really necessary. If you just need expected results for the other platforms, then you can commit this and then grab the results off the bots. That said, this test looks platform-agnostic to me.

[John Abd-El-Malek]

Ojan: the layout test plugin's NPP_HandleEvent is not implemented on the other platforms (i.e. printing to the console what key was sent to it via eventSender).  I followed what was done for the keyboard-events test (https://bugs.webkit.org/show_bug.cgi?id=34936).

[John Abd-El-Malek]

Created attachment 50823 [details]
Patch

[WebKit Review Bot]

Attachment 50823 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:114:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:114:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ojan Vafai]

(In reply to comment #9)
> Ojan: the layout test plugin's NPP_HandleEvent is not implemented on the other
> platforms (i.e. printing to the console what key was sent to it via
> eventSender).  I followed what was done for the keyboard-events test
> (https://bugs.webkit.org/show_bug.cgi?id=34936).

I see. That seems fine to me then.

[John Abd-El-Malek]

Created attachment 50839 [details]
Patch

[WebKit Review Bot]

Attachment 50839 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:114:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Abd-El-Malek]

OK this is ready for review.

I tried to make this work on Mac, but eventSender seems to always send the
event to the main window.  In this test a new window is opened up and hence the
event doesn't get to the plugin.  I don't know Objective-C so I'm unable to go
further.  I did update the layout plugin though.

[Darin Fisher (:fishd, Google)]

Comment on attachment 50839 [details]
Patch

> Index: WebCore/page/EventHandler.cpp
> ===================================================================
> --- WebCore/page/EventHandler.cpp	(revision 56027)
> +++ WebCore/page/EventHandler.cpp	(working copy)
> @@ -2057,6 +2057,8 @@ static Node* eventTargetNodeForDocument(
>      if (!doc)
>          return 0;
>      Node* node = doc->focusedNode();
> +    if (!node && doc->isPluginDocument())
> +        node = doc->body()->firstChild();

nit: it might be nice to create a helper function on PluginDocument
that can be used to fetch the embed node.  that way this code here
doesn't need to know the internal DOM structure of the plugin document.

R=me

[David Levin]

Note there are several files in here that have "Added: svn:executable" which shouldn't.

[John Abd-El-Malek]

Darin: good suggestion, done

David: thanks for pointing it out.  I didn't add them, so I wonder if webkit-patch did?  I just took them out (except for the pl file).

[John Abd-El-Malek]

Committed r56096: <http://trac.webkit.org/changeset/56096>