Bug 12675 - REGRESSION: onBlur doesn't fire when focus switches between subframes
Summary: REGRESSION: onBlur doesn't fire when focus switches between subframes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-02-06 23:37 PST by Maciej Stachowiak
Modified: 2007-07-05 13:16 PDT (History)
4 users (show)

See Also:


Attachments
blur.html (47 bytes, text/html)
2007-02-20 16:31 PST, Maciej Stachowiak
no flags Details
blurFrame.html (121 bytes, text/html)
2007-02-20 16:32 PST, Maciej Stachowiak
no flags Details
Use FocusController::setFocusedNode in the EventHandler (6.98 KB, patch)
2007-02-27 17:41 PST, Dex Deacon
adele: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maciej Stachowiak 2007-02-06 23:37:50 PST
2006-12-10 15:22:08 Adam Roben:
* SUMMARY
We don't dispatch blur events when switching between subframes.

* STEPS TO REPRODUCE
1. Save blur.html and blurFrame.html to the same folder
2. Open blurFrame.html in ToT Safari
3. Type in the text field on the left
4. Type in the text field on the right

* RESULTS
No blur event is fired. If the event is fired, an alert dialog will pop up.

2006-12-12 19:37:07 Adam Roben:
Assigning to Geoff since he's working on focus issues.

2006-12-18 13:48:08 Stephanie Lewis:
Safari BRB Reviewed

2007-01-15 14:10:29 Alice Liu:
Safari blocker reviewed

<rdar://problem/4874825>
Comment 1 Maciej Stachowiak 2007-02-20 16:31:22 PST
Created attachment 13282 [details]
blur.html
Comment 2 Maciej Stachowiak 2007-02-20 16:32:33 PST
Created attachment 13283 [details]
blurFrame.html
Comment 3 Dex Deacon 2007-02-27 14:18:18 PST
I don't have permission to assign bugs, but I am looking into this.
Comment 4 Dex Deacon 2007-02-27 17:41:29 PST
Created attachment 13415 [details]
Use FocusController::setFocusedNode in the EventHandler

I had to modify an existing layout test (fast/frames/iframe-window-focus.html) to pass under this change.  I think it's a valid modification though.  The test was blurring and then re-focusing a text node after entering some text.  It then entered more text, assuming the cursor position was preserved.

This worked before because the node never actually received the blur event.  With my change, however, the cursor position is reset when the node is blurred.
Comment 5 Maciej Stachowiak 2007-02-27 19:00:08 PST
I thought we had code to intentionally restore caret position when focusing a text field via script. I could be remembering wrong though. I think Adele knows the most about this.
Comment 6 David Kilzer (:ddkilzer) 2007-02-27 20:17:47 PST
Adding Adele to CC list per Comment #5.

Comment 7 Adele Peterson 2007-02-27 22:20:24 PST
Comment on attachment 13415 [details]
Use FocusController::setFocusedNode in the EventHandler

This seems fine to me.

Its weird though, I applied the patch in WebCore, and iframe-window-focus.html still passes for me.  Are you sure that change is needed?
Comment 8 Adele Peterson 2007-02-27 22:48:38 PST
Comment on attachment 13415 [details]
Use FocusController::setFocusedNode in the EventHandler

Just going to r- this until we figure out the layout test issue.
Comment 9 Adele Peterson 2007-02-28 12:12:33 PST
Something was wrong with my setup before.  I'm not sure what, but now I can see the layout test difference.

Here's the thing, this does expose a behavior problem with rich text editing in GMail.

If you start typing, and then change the font color (using one of their buttons), the cursor gets set back to the beginning.

This is sort of a separate problem, but the symptom is pretty bad.  I'm tempted to make this bug blocking the new bug...that someone should file... about restoring a previous selection for editable elements.
Comment 10 Dex Deacon 2007-02-28 13:59:31 PST
In Document::setFocusedNode(), the call to clearSelectionIfNeeded() is what resets the cursor position.  It doesn't appear to be stored anywhere, so it would seem to be that case that unfocusing a node would always reset the cursor position.  Maybe the solution involves storing the selection (or at least part of it) somewhere before we clear it.
Comment 11 Adele Peterson 2007-02-28 16:27:12 PST
I think we can probably fix this by changing FocusController::setFocusedNode.  I think its clearing out the oldDocument's focusNode too aggressively.  I'm testing a change for that in my tree.

If that fixes this frame focus problem, we should be able to proceed with the original fix.
Comment 12 Adele Peterson 2007-02-28 16:27:40 PST
I meant, frame caret problem.
Comment 13 Adele Peterson 2007-02-28 18:03:22 PST
I think I was wrong in my last comment.  Since you need to clear the focusedNode to get blur events, we can't really get rid of any of the setFocusedNode(0) calls.

Dex, your idea about refining clearSelectionIfNeeded sounds good.  Ideally, I think we'll want to be able to save and restore selection for individual elements, but doing this at the frame level might be a good first step.

I might be talking myself in circles here.


Comment 14 Adele Peterson 2007-07-05 13:11:46 PDT
This is fixed in TOT.  Probably by 21467.
Comment 15 Adele Peterson 2007-07-05 13:16:20 PDT
I narrowed the range down to 21448 - 21494, and 21467 is my best guess