Bug 31692 - window.onblur() calling window.focus leaves focus in 2 places
Summary: window.onblur() calling window.focus leaves focus in 2 places
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Victor Wang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-19 15:15 PST by Jay Campan
Modified: 2010-02-05 10:44 PST (History)
3 users (show)

See Also:


Attachments
Proposed Patch (9.71 KB, patch)
2010-01-19 13:37 PST, Victor Wang
darin: review+
Details | Formatted Diff | Diff
More test cases added (14.90 KB, patch)
2010-01-27 09:41 PST, Victor Wang
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jay Campan 2009-11-19 15:15:04 PST
Open the attached file, iframe.html
- Click the textfield at the top of the page
- Click the textfield in the iframe Google page.

Expected:
- the focus ends up back in the top textfield (as the JavaScript code on window.onblur calls window.focus())

Actual:
- Both textfields are showing a blinking cursor.

Notes:
- Also note that extra blur/focus events are fired.
- Some investigation on what happens:

EventHandler::handleMousePressEvent(const PlatformMouseEvent& mouseEvent) dispatches the event to 
the IFrame node which causes the IFrame to get focus. As part of the windown.onblur, the JS 
restores the focus to the main frame.

But after that, it calls handleMousePressEvent(const MouseEventWithHitTestResults& event) 
(different signature) which itself calls focusDocumentView() which focus again the IFrame 
triggering another cycle of onblur/onfocus, and leaving the focus controller apparently confused 
(the 2 frames are shown as having focus).
Comment 1 Victor Wang 2010-01-19 13:36:45 PST
As Jay pointed out above, the problem is caused by the focused frame in FocusController is messed up if window.onblur calls window.focus. When user clicks iframe to switch focus from main frame to iframe, FocusController::setFocusedFrame fires onblur event, which calls window.focus and then calls setFocusedFrame again to switch back. This messes up the old focused frame and new focused frame and leaves the FocusController confused. As a result, controlls in both main frame and iframe look like get focused.
Comment 2 Victor Wang 2010-01-19 13:37:39 PST
Created attachment 46946 [details]
Proposed Patch
Comment 3 Darin Adler 2010-01-19 15:35:57 PST
What if window.onfocus calls window.focus on a different window?
Comment 4 Victor Wang 2010-01-19 17:53:42 PST
Hi Darin,

Good point.

This patch only affects focusing when switching focused frame, so I tested calling iframe window focus from window.onfocus. Because of the new flag, setFocusedFrame returns when FocusController is in the middle of changing focused frame, therefore, in this case with the patch, the iframe window focus request is ignored.

The code prior the patch calls dispatchWindowEvent(win1.focusEvent) inside another dispatchWindowEvent(win2.focusEvent), the focus event is dispatched at almost the last step of setFocusedFrame, so it changes the focus back to iframe. In this case, webkit changes focus to one frame when it is in the middle of setting focus to another one, I might miss something even though the FocusController status looks fine to me.

I test this on IE and Firefox:
IE looks like ignore the iframe window focus call and behaves the same as webkit with my patch.
FireFox messes up the focus status and leaves two fields both focused.

Personally I think we should add this flag so changing focused frame request is ignored if FocusController is middle of processing another request. Same for onblur calling focus and onfocus calling focus. What do you think?
Comment 5 Darin Adler 2010-01-25 09:49:02 PST
(In reply to comment #4)
> Personally I think we should add this flag so changing focused frame request is
> ignored if FocusController is middle of processing another request. Same for
> onblur calling focus and onfocus calling focus. What do you think?

Seems OK. The real point is having enough test cases; how we implement is not as important as what the observed behavior is. I worry that there will be insufficient test coverage. Lets think of more edge and common cases and make tests that cover a lot of them.
Comment 6 Victor Wang 2010-01-27 09:41:49 PST
Created attachment 47545 [details]
More test cases added

Added more cases to test: Normally changing frame focus working as expected and FocusController only allows one changing focus request at a time.
Comment 7 Victor Wang 2010-01-29 17:25:46 PST
Manually committed r54082: http://trac.webkit.org/changeset/54082