Bug 40029 - Widgets get 2 notifications (lost focus/got focus) each time focus switches between 2 nodes
Summary: Widgets get 2 notifications (lost focus/got focus) each time focus switches b...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2010-06-01 17:15 PDT by Jay Civelli
Modified: 2010-08-11 11:06 PDT (History)
2 users (show)

See Also:

First version of the patch (5.87 KB, patch)
2010-06-01 17:20 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Remove extra space (5.59 KB, patch)
2010-06-02 10:25 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Addressing Stuart's comments. (6.06 KB, patch)
2010-06-02 11:18 PDT, Jay Civelli
ap: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jay Civelli 2010-06-01 17:15:36 PDT
Each time the focus moves between 2 nodes in the same widget, the widget gets a setFocus(false) and then setFocus(true) calls.
It should not get any notification that the focus changed in that case (note it still gets the focusedNodeChanged notification on the Page Chrome).

This happens in Document.cpp in Document::setFocusedNode()
Comment 1 Jay Civelli 2010-06-01 17:20:09 PDT
Created attachment 57605 [details]
First version of the patch
Comment 2 Jay Civelli 2010-06-02 10:25:15 PDT
Created attachment 57665 [details]
Remove extra space
Comment 3 Stuart Morgan 2010-06-02 10:37:54 PDT
(drive-by, since I'm not actually a reviewer)

Isn't this introducing ordering problems? There are a lot of focus-related calls that happen in the main |if (newFocusedNode)| block, which used to happen after the setFocus(false) (which seems logical), but would now happen before it (which doesn't).

Granted the setFocus(false) call is new, and this may well not break anything that currently exists, but it seems like it could lead to weird and hard-to-find problems later if people do start using it.
Comment 4 Jay Civelli 2010-06-02 11:18:15 PDT
Created attachment 57670 [details]
Addressing Stuart's comments.
Comment 5 Jay Civelli 2010-06-02 11:19:30 PDT

Good point.
I tried to address the ordering problem in the new patch.
Comment 6 Alexey Proskuryakov 2010-06-02 13:29:45 PDT
Comment on attachment 57670 [details]
Addressing Stuart's comments.

Is this a problem for other WebKit-based browsers?

This bug doesn't describe when this can happen - I suspect it's with subframes, but that's only a guess.

Please add a regression test, or an explanation of why one can't be added.

r- for lack of regression test.
Comment 7 Alexey Proskuryakov 2010-06-02 13:32:07 PDT
See also: bug 33512, bug 28913, bug 12262, bug 20387, bug 18759.
Comment 8 Jay Civelli 2010-06-07 16:04:58 PDT
The problem is that the embedder code will get the spurious focus notifications, not the page. For that reason I don't think I can provide a layout test for this.

I see that in Chromium, the Widget::setFocus() implementation gets called with false and then true every time the focus moves from one node to the next.
Comment 9 Alexey Proskuryakov 2010-06-08 10:07:27 PDT
DumpRenderTree can dump many sorts of callbacks already. I suspect that layoutTestController.dumpEditingCallbacks() may do what you need - but if it doesn't, the way to proceed is to add a new logging delegate implementation.

Does your fix affect any of the bugs I mentioned in comment 7?

You didn't answer whether this affects other WebKit ports. If this is a Chromium-only problem, it's of course surprising that you needed to change common code to fix it. Someone will have to explain why that's necessary. You can just say that you don't know, and then it will be reviewer's job, but that will make the patch more difficult to review.
Comment 10 Jay Civelli 2010-06-09 15:57:11 PDT
I don't know of a way to surface focus events on the Widget to the  LayoutTestController (since these calls implementation are port specific) . dumpEditingCallbacks does not do it.

Since Widget::setFocus() is implemented by each port (in WebCore/platform), this change does affect all ports.

I tested all the bugs you reported in comment 7, this change does not fix any of them. It just prevents the embedder port from getting extra focus notifications.
Comment 11 Jay Civelli 2010-08-11 11:06:42 PDT
I found a different way to address the issue I was trying to address with this patch, so this is now obsolete.