131651 - Popup window is blurred on open if opened from a mousedown handler for a link

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Brian Ryner (not reading)]

Spun off from bug 120209, as I believe this is a different issue and looks like
it's cross-platform.

If you have an anchor element with a mousedown handler which opens and focuses a
popup window, that popup window will receive a blur as soon as it is opened. 
The reason for this is that we fire the script event handler first, then we
focus the link in our mousedown handling (see
nsGenericHTMLElement::HandleDOMEventForAnchors), which refocuses the parent
window.  This can cause the popup window to close immediately if it has an
onblur handler that closes the window.

Comment 1

[saari (gone)]

I tried the obvious approach of checking if the window was active before
focusing from nsGenericHTMLElement::HandleDOMEventForAnchors but that didn't
work. At the time the check was made via asking the focus controller if it was
active, the controller still thought it was since we didn't yet get back to the
OS event to deactivate it.

So we raced the pop up window's deactivate, and we beat it *but* the global
gLastFocusedDocument was correctly set to the new window... so we sent it a blur
from SendFocusBlur.

It looks like the focus globals are out of synch with the focus controller, and
this case gets in the gap.

Comment 2

[saari (gone)]

Attachment: Patch that fixes this. Mildly scary part in nsEventStateManager.

This fixes it. The part in nsGenericHTMLElement is right, but the part in
nsEventStateManager is to deal with this issue of the globals and focus
controllers getting out of synch until other events in the event queue are
dealt with. It is an interesting case that I handn't really considered before.
This probably needs some thought about possible places where keeping these in
synch would cause problems. Keeping them in synch also feels like a hack, and
I'd like to get rid of the globals entirely, but the focus controllers don't
have the right semantics to take over the globals' functionality right now
(probably ever), so I'm guessing the right thing will have to wait for post
1.0.

Comment 3

[saari (gone)]

taking on for mozilla 1.0 as I have a patch and it exposes a problem that could
be causing other focus issues.

Comment 4

[Kevin McCluskey (gone)]

embed triage: topembed+, very visible defect 

Comment 5

[Judson Valeski]

-> bryner. Is there any chance you can run with Saari's patch here?

Comment 6

[Peter Trudelle]

Does this occur on any top sites?

Comment 7

[Brian Ryner (not reading)]

Yeah, I'll give saari's patch some testing (I've been meaning to do this,
haven't gotten around to it).

Comment 8

[Peter Trudelle]

Navtriage still has no feel for the impact of this bug. Is Parker a top site?

Comment 9

[Samir Gehani]

Nav triage team: nsbeta1-

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 75145 [details] [diff] [review]
Patch that fixes this. Mildly scary part in nsEventStateManager. 

r=bryner.   This all seems correct to me, and I couldn't find any regressions
running with this patch.

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 75145 [details] [diff] [review]
Patch that fixes this. Mildly scary part in nsEventStateManager. 

sigh, ok.  The mildly scary part in EventStateManager has become more scary. 
This patch causes a regression where if I activate a window right as a new page
is loading, focus will get wacked and you won't be able to focus anything until
you deactivate and reactivate the window.  This is on Linux.

Comment 12

[Brian Ryner (not reading)]

Attachment: more spackle

So, in the case that was causing this regression, nextController was null.  I
added a check for this so that we don't deactivate the previous controller in
this situation.

Chris, I'm not sure which platform you were testing on where the globals were
out of sync; can you retest and make sure this doesn't cause your fix not to
work correctly?

Comment 13

[Brian Ryner (not reading)]

Comment on attachment 86986 [details] [diff] [review]
more spackle

r=saari from bryner's machine

Comment 14

[Joe Hewitt (gone)]

Comment on attachment 86986 [details] [diff] [review]
more spackle

sr=hewitt

Comment 15

[Brian Ryner (not reading)]

Checked in on the trunk.  Will let this bake for a couple of days before
checking in on the branch.

Comment 16

[Shiva Thirumazhusai]

Causing topcrash in the yesterday's Trunk builds. Look at the topcrash bug 151568. 

Comment 17

[Brian Ryner (not reading)]

Nominating for branch approval.

Comment 18

[Jaime Rodriguez, Jr.]

namachi - are you saying this resolved a topcrash, or caused one? the bug you
refer to seems to be resolved fixed.

Kevin - you marked this one as a very visible defect. is this because the URl
referenced is a top site, or that this defect is seen quite often out in the wild?

Comment 19

[Kevin McCluskey (gone)]

I don't think this is seen often on real sites. The original bug that this was
spawned from bug 120209 was causing problems on a number of sites and is now
fixed I'm not sure if the fix needs to go on the branch.

Comment 20

[scottputterman]

Adding adt1.0.1- based on comment 19.

Comment 21

[Boris Zbarsky [:bzbarsky]]

This caused bug 227741, looks like...