Bug 36147

Summary: Give keyboard focus to PluginDocuments by default
Product: WebKit Reporter: John Abd-El-Malek <jam>
Component: New BugsAssignee: John Abd-El-Malek <jam>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, levin, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch fishd: review+

Description John Abd-El-Malek 2010-03-15 18:02:05 PDT
Give keyboard focus to PluginDocuments[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dplugins in PluginDocuments by default
Comment 1 John Abd-El-Malek 2010-03-15 18:06:30 PDT
Created attachment 50751 [details]
Patch
Comment 2 John Abd-El-Malek 2010-03-15 18:08:34 PDT
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.
Comment 3 Eric Seidel (no email) 2010-03-15 19:06:25 PDT
Filed bug 36149.
Comment 4 John Abd-El-Malek 2010-03-15 19:11:50 PDT
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.
Comment 5 John Abd-El-Malek 2010-03-16 00:00:36 PDT
Created attachment 50767 [details]
Patch
Comment 6 John Abd-El-Malek 2010-03-16 00:06:34 PDT
Created attachment 50769 [details]
Patch
Comment 7 John Abd-El-Malek 2010-03-16 00:17:00 PDT
Created attachment 50770 [details]
Patch
Comment 8 Ojan Vafai 2010-03-16 09:27:09 PDT
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.
Comment 9 John Abd-El-Malek 2010-03-16 10:09:55 PDT
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).
Comment 10 John Abd-El-Malek 2010-03-16 12:23:19 PDT
Created attachment 50823 [details]
Patch
Comment 11 WebKit Review Bot 2010-03-16 12:24:58 PDT
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.
Comment 12 Ojan Vafai 2010-03-16 12:39:19 PDT
(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.
Comment 13 John Abd-El-Malek 2010-03-16 14:26:54 PDT
Created attachment 50839 [details]
Patch
Comment 14 WebKit Review Bot 2010-03-16 14:32:06 PDT
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.
Comment 15 John Abd-El-Malek 2010-03-16 14:37:48 PDT
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.
Comment 16 Darin Fisher (:fishd, Google) 2010-03-16 15:26:56 PDT
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
Comment 17 David Levin 2010-03-16 16:44:13 PDT
Note there are several files in here that have "Added: svn:executable" which shouldn't.
Comment 18 John Abd-El-Malek 2010-03-16 17:06:30 PDT
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).
Comment 19 John Abd-El-Malek 2010-03-16 17:32:29 PDT
Committed r56096: <http://trac.webkit.org/changeset/56096>