487700 - Alert box no longer has focus after closing popup

Status: RESOLVED WORKSFORME

(Core :: Widget: Win32, defect, P2)

Description

[Duncan Loveday]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090409 Minefield/3.6a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090409 Minefield/3.6a1pre

The javascript code below results in an alert box that does not have focus. As in you can't dismiss it by pressing enter, you have to click on it. This is not the case in 3.0.8 or the 1.9.1 tip.

    var w=window.open("about:blank", "popup1", "top=20");
    w.close();
    alert("Hi");


Reproducible: Always

Steps to Reproduce:
1. Open the attached test case, press "Go"
2. Try to dismiss the alert by pressing enter
3.
Actual Results:  
In step 2 nothing happens. You have to click on the alert to dismiss it.

Expected Results:  
The alert should be dismissed by pressing enter in step 2 as is the case in Fx 3.0.8 and the 1.9.1 builds.

Comment 1

[Duncan Loveday]

Attachment: Test case

Comment 2

[Ria Klaassen (not reading all bugmail)]

Regression range appears to be:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ae146611c2d7&tochange=e783d41a4e4a

Comment 3

[Johnny Stenback (:jst)]

Ere, per Ria's regression range this points to your fix for bug 313403. Can you have a look at this for 1.9.2?

Comment 4

[Johnny Stenback (:jst)]

Ere, ping?

Comment 5

[Ere Maijala (slow)]

Priority override, can't devote time right now, sorry :(

Comment 6

[Johnny Stenback (:jst)]

Ok. Per smaug this is not a DOM bug, it's a win32 widget level bug, so reassigning.

Comment 7

[Joe Drew (not getting mail)]

Neil, you reviewed Ere's patch - any chance you can take a look at this? It's a blocker without an owner right now.

Comment 8

[[e-mail]]

This bug has two parts.

The first part is that because this is a web page closing the popup, it happens asynchronously, so the popup really closes during the modal loop, which is after the original window is disabled but before the alert window has been made visible. Windows therefore activates some other window instead.

The second part is that, since bug 313403, nsWindow doesn't focus windows as aggressively as it used to which means that the alert doesn't reclaim focus.

Comment 9

[[e-mail]]

(In reply to comment #8)
> The second part is that, since bug 313403, nsWindow doesn't focus windows as
> aggressively as it used to which means that the alert doesn't reclaim focus.
And it needs to do that because otherwise the alert gets a WM_ACTIVATE message even when it fails to become the active window...

Comment 10

[[e-mail]]

(In reply to comment #9)
> (In reply to comment #8)
> > The second part is that, since bug 313403, nsWindow doesn't focus windows as
> > aggressively as it used to which means that the alert doesn't reclaim focus.
> And it needs to do that because otherwise the alert gets a WM_ACTIVATE message
> even when it fails to become the active window...
And then it doesn't get another WM_ACTIVATE message when it does...

Comment 11

[Joe Drew (not getting mail)]

Neil, will you be able to work on this before the 1.9.2 freeze on Wednesday? If not, I can see if I can find someone with spare cycles.

Comment 12

[[e-mail]]

(In reply to comment #11)
> Neil, will you be able to work on this before the 1.9.2 freeze on Wednesday? If
> not, I can see if I can find someone with spare cycles.
I'm sorry, I'd completely forgotten about this. I'll try and help as much as I can but I don't think I'm up to writing a patch for this.

(In reply to comment #6)
> Per smaug this is not a DOM bug, it's a win32 widget level bug, so reassigning.
From my point of view, widget is doing everything DOM/appshell is asking of it.

Comment 13

[Jim Mathies [:jimm]]

I'll take it.

Comment 14

[Jim Mathies [:jimm]]

This is the code that causes this. Ere, can you offer up some reasoning behind adding this in the patch for bug 313403? Looks like comment 13 relates?

1.146 +#ifndef WINCE
1.147 +          if (mWindowType == eWindowType_dialog && !CanTakeFocus())
1.148 +            flags |= SWP_NOACTIVATE;
1.149 +#endif

CanTakeFocus goes through a set of checks to figure out what it should return:

1) start enumerating every top level window on the desktop
2) for each window enumerated, test for a visible mozilla window. if found, stop enumerating.
3) at the end of enumeration, if a visible mozilla window was *not* found, return TRUE from CanTakeFocus().
4) if a visible mozilla window *was* found, check to see if there is no current foreground window, if not return TRUE from CanTakeFocus().
5) if there is a foreground window, check to see if it's a mozilla window. if it is, return TRUE, otherwise return FALSE.

In this case, we return false in step 5, causing SWP_NOACTIVATE to be added. I'm wondering if we can remove this little chunk of code, in testing without it I didn't see any dialog focus anomalies.

Comment 15

[Jim Mathies [:jimm]]

Attachment: various alert test cases

Comment 16

[Jim Mathies [:jimm]]

Attachment: dialog focus patch v.1

This addresses the problem. One of the things I noticed from testing using the test cases is that our flash functionality when the main window is minimized changed between 3.5, 3.6, and 3.7. None of the versions really does a perfect job, so a follow up bug is probably needed.

Comment 17

[Ere Maijala (slow)]

Comment on attachment 412911 [details] [diff] [review]
dialog focus patch v.1

The idea is to avoid stealing focus from other apps. Where is the focus actually in this case? I would have assumed it would stay in a Mozilla window, but if CanTakeFocus returns false, that appears not to be the case. 

One scenario that really sucks without this check is opening e.g. Word documents from Firefox. When Word has opened the document, Firefox would steal the focus and I've witnessed quite a few times this leaving people wondering what happened.

Comment 18

[Jim Mathies [:jimm]]

(In reply to comment #17)
> (From update of attachment 412911 [details] [diff] [review])
> The idea is to avoid stealing focus from other apps. Where is the focus
> actually in this case? I would have assumed it would stay in a Mozilla window,
> but if CanTakeFocus returns false, that appears not to be the case. 

The focus is weird. It's on the last window that had foreground status before the main fx window came to the foreground. This window gains foreground status for a brief instant (same time CanTakeFocus is called), then we steal it back when the dialog shows up. This is reproducible on 3.5, 3.6, and trunk. I've been trying to track that down, but haven't come up with anything yet. Thought it might be due to trim_on_minimize code, but it doesn't.

Comment 19

[Jim Mathies [:jimm]]

In the following script code:

var w=window.open("about:blank", "popup1", "top=20");
w.close();
alert();

We create the dialog before the popup window is torn down. The dialog is present but hidden. Windows refuses to switch back to the parent browser with the hidden dialog sitting there, so it activates some other window. We then tear down the popup, and call Show on the dialog, but due to the failure in CanTakeFocus, the dialog isn't activated. (CanTakeFocus fails because the parent browser is no longer the foreground window.)

This isn't a widget bug. We should create the dialog and show it immediately before the popup is torn down, or in line with the script, tear down the popup and then create and show the dialog.

Comment 20

[Jim Mathies [:jimm]]

Attachment: sequence

Comment 21

[Jim Mathies [:jimm]]

3.5 steps to see the problem:

1) open "various alert test cases" attachment.
2) open an explorer window
3) in explorer, highlight something (a folder or a file)
4) place explorer on the desktop so that the highlighted item is unobstructed by 3.5's window
5) click on explorer window, click on Fx window
6) run the second test case "open window, close window, open dialog"
7) keep an eye on the highlighted item in explorer

results: 

you should see the explorer window gain the foreground briefly, then loose it when the dialog in fx pops up. The highlighted item helps spot this since it changes color when the window gains the foreground.

Comment 22

[Jim Mathies [:jimm]]

After speaking with smaug, creating the dialog as hidden and then tearing down the popup is expected, since the popup tear down happens async. So the options here are: 

1) land the patch I posted making dialogs much more assertive (and potentially annoying) on windows.
2) roll together an uber-hack in widget that tests for hidden dialogs during window tear down which would then trigger different dialog Show() functionality.
3) mark as wont-fix, and live with this.
4) back out bug 313403, which improved our handling of GetAttention.

Ere voted thumbs down on #1. I personally don't like #2 as there's no way of knowing what side effects it will have and generally, we're trying to remove hacks from widget rather than add them. #3 is a possibility, this is a corner case that site authors can avoid, and #4 isn't a very good option since it improves widget support and removed a bunch of ugly timer code.

Comment 23

[Joe Drew (not getting mail)]

I personally am OK with wontfixing this, mostly because it's an edge case. But it will suck to have webpages that used to work stop working.

It'd be interesting to see a survey of other browsers to see what they do in these cases.

Regardless, this probably shouldn't block. Changing back to ?.

Comment 24

[Duncan Loveday]

Attachment: Revised test case

> It'd be interesting to see a survey of other browsers to see what they do in
> these cases.

Opera 10.01 won't close a window with about:blank in it, so attaching a slightly changed test case using window.open("",... instead of window.open("about:blank",...

Opera 10.01 and IE7 and Firefox 3.5.x all end with focus on the dialogue in this test case.

Comment 25

[Duncan Loveday]

...and ditto IE8.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #22)
> 4) back out bug 313403, which improved our handling of GetAttention.
4.1) ...and fix that bug in some other way this doesn't regress this bug.

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Not a blocker for 3.6 final, but definitely something we need to fix.

Comment 28

[Jim Mathies [:jimm]]

It's important to note here that this alert dialog focus issue is just a symptom of the main problem detailed in comment 19 and exposed by the steps in comment 21. The patch in bug 313403 didn't cause this.

Comment 29

[Jim Mathies [:jimm]]

smaug, curious where the code that creates/displays the alert dialog is in the source? I'd like to do some more debugging and it would be helpful to see where that happens.

Comment 30

[Joe Drew (not getting mail)]

Jeff and I just tested - Safari and Chrome both end with focus on the alert box too.

Comment 31

[Joe Drew (not getting mail)]

We shouldn't block on this, since we're not even sure we can fix it.

Comment 32

[Duncan Loveday]

My revised test case https://bugzilla.mozilla.org/attachment.cgi?id=413298 now seems to be working OK

Comment 33

[Duncan Loveday]

(In reply to comment #32)
> My revised test case https://bugzilla.mozilla.org/attachment.cgi?id=413298 now
> seems to be working OK

I should have been clearer: The test cases I originally posted now work in the Fx4 trunk. Does that mean we can close this as WFM or are there still outstanding issues that need further work ?

Comment 34

[Jim Mathies [:jimm]]

(In reply to comment #33)
> (In reply to comment #32)
> > My revised test case https://bugzilla.mozilla.org/attachment.cgi?id=413298 now
> > seems to be working OK
> 
> I should have been clearer: The test cases I originally posted now work in
> the Fx4 trunk. Does that mean we can close this as WFM or are there still
> outstanding issues that need further work ?

Yes, our new alert mechanism has solved this for us.