Bug 58419 - [chromium] Clicking on plugin for first time blocks focus shift
Summary: [chromium] Clicking on plugin for first time blocks focus shift
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Stuart Morgan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-13 01:48 PDT by Jeremy Moskovich
Modified: 2011-04-19 13:45 PDT (History)
7 users (show)

See Also:


Attachments
testcase (962 bytes, text/html)
2011-04-13 01:54 PDT, Jeremy Moskovich
no flags Details
fix (1.69 KB, patch)
2011-04-13 10:21 PDT, Stuart Morgan
eric: review-
Details | Formatted Diff | Diff
fix v2 (4.13 KB, patch)
2011-04-18 16:30 PDT, Stuart Morgan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Moskovich 2011-04-13 01:48:32 PDT
Chromium bug: crbug.com/77172

Description:
The attached testcase contains a plugin and a textarea embedded in an iframe.

Open the testcase in Chromium, click on the plugin, then click back in the textarea and try to type some text.

Expected result:
You should be able to type into the textarea.

Actual result:
You can't type into the textarea, even though the focus ring is draw around it.
Comment 1 Jeremy Moskovich 2011-04-13 01:54:43 PDT
Created attachment 89354 [details]
testcase
Comment 2 Stuart Morgan 2011-04-13 10:21:41 PDT
Created attachment 89397 [details]
fix

Uses FocusController to handle the focus change, just as would happen for a non-plugin element (see the debugging in the Chromium bug for more details on why).
Comment 3 Stuart Morgan 2011-04-18 08:53:57 PDT
Ping. Can one of you take a look at this? It's a small patch, and the issue it fixes is pretty nasty.
Comment 4 Eric Seidel (no email) 2011-04-18 09:07:40 PDT
Comment on attachment 89397 [details]
fix

How do we test this?
Comment 5 Eric Seidel (no email) 2011-04-18 09:08:15 PDT
If this is only reproducible in the full Chromium browser, we could/should still land a manual-test.
Comment 6 Ryosuke Niwa 2011-04-18 10:12:10 PDT
Comment on attachment 89397 [details]
fix

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

Can't we use eventSender to test this?

> Source/WebKit/chromium/ChangeLog:4
> +

You forgot bug title & bug url.
Comment 7 Stuart Morgan 2011-04-18 10:25:20 PDT
(In reply to comment #4)
> How do we test this?

(In reply to comment #6)
> Can't we use eventSender to test this?

I have very little experience with WebKit development; I'll try to figure this out myself I guess (the docs seem pretty sparse), but pointers would be a lot more helpful than rhetorical questions I don't know the answers to.

(In reply to comment #6)
> You forgot bug title & bug url.

The bug link is there. I wasn't aware the title was necessary; I'll add it. (Perhaps the prepare-ChangeLog script should update the template if people are expected to include it, since I don't see any mention of it in either the template or the "how to make a patch" documentation.)
Comment 8 Ryosuke Niwa 2011-04-18 10:29:45 PDT
(In reply to comment #7)
> I have very little experience with WebKit development; I'll try to figure this out myself I guess (the docs seem pretty sparse), but pointers would be a lot more helpful than rhetorical questions I don't know the answers to.

EventSender allows you to send arbitrary mouse events / keyboard events in layout tests.  I don't think there's any documentation about eventSender but you can see a couple of examples if you grep on LayoutTests.

> (In reply to comment #6)
> > You forgot bug title & bug url.
> 
> The bug link is there. I wasn't aware the title was necessary; I'll add it. (Perhaps the prepare-ChangeLog script should update the template if people are expected to include it, since I don't see any mention of it in either the template or the "how to make a patch" documentation.)

You're supposed to put the bug title & bug url in the consecutive lines and then have a blank line before describing what you're fixing.
Comment 9 Stuart Morgan 2011-04-18 16:30:22 PDT
Created attachment 90114 [details]
fix v2

Test case added, based on looking at some of the surrounding test--thanks for the pointers. It runs as both a layout test and an interactive test (although the two modes work differently since I can't simulate clicks in normal mode).
Comment 10 Tony Chang 2011-04-19 10:06:19 PDT
Comment on attachment 90114 [details]
fix v2

Adding cq+ since stuart isn't a committer.
Comment 11 WebKit Commit Bot 2011-04-19 11:15:36 PDT
The commit-queue encountered the following flaky tests while processing attachment 90114 [details]:

http/tests/xmlhttprequest/cross-origin-no-credential-prompt.html bug 52249 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2011-04-19 11:17:52 PDT
Comment on attachment 90114 [details]
fix v2

Clearing flags on attachment: 90114

Committed r84270: <http://trac.webkit.org/changeset/84270>
Comment 13 WebKit Commit Bot 2011-04-19 11:17:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 WebKit Commit Bot 2011-04-19 12:21:03 PDT
The commit-queue encountered the following flaky tests while processing attachment 90114 [details]:

http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: alice.liu@apple.com)
The commit-queue is continuing to process your patch.
Comment 15 WebKit Review Bot 2011-04-19 12:40:41 PDT
http://trac.webkit.org/changeset/84270 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
plugins/mouse-click-iframe-to-plugin.html
Comment 16 Stuart Morgan 2011-04-19 12:55:47 PDT
Am I missing something in the test structure? It's all run in one pass, no waiting for callbacks, so I didn't think I needed notifyDone based on looking at other cases. It ran fine for me locally on Snow Leopard.
Comment 17 Ryosuke Niwa 2011-04-19 13:36:03 PDT
I skipped the test in http://trac.webkit.org/changeset/84279 for WebKit2 but it doesn't seem to be the right fix then.  Please re-enable the test on WebKit2 when you fixed the test.
Comment 19 Tony Chang 2011-04-19 13:45:32 PDT
Updated Skipped files in http://trac.webkit.org/changeset/84287.  The bug for tracking the mac/win failure is https://bugs.webkit.org/show_bug.cgi?id=58924