Bug 40029 – Widgets get 2 notifications (lost focus/got focus) each time focus switches between 2 nodes

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Jay Civelli]

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).

Note:
This happens in Document.cpp in Document::setFocusedNode()

[Jay Civelli]

Created attachment 57605 [details]
First version of the patch

[Jay Civelli]

Created attachment 57665 [details]
Remove extra space

[Stuart Morgan]

(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.

[Jay Civelli]

Created attachment 57670 [details]
Addressing Stuart's comments.

[Jay Civelli]

@Stuart

Good point.
I tried to address the ordering problem in the new patch.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

See also: bug 33512, bug 28913, bug 12262, bug 20387, bug 18759.

[Jay Civelli]

@Alexey
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.

[Alexey Proskuryakov]

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.

[Jay Civelli]

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.

[Jay Civelli]

I found a different way to address the issue I was trying to address with this patch, so this is now obsolete.