Closed Bug 487700 Opened 14 years ago Closed 12 years ago
Alert box no longer has focus after closing popup
Regression range appears to be: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ae146611c2d7&tochange=e783d41a4e4a
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
Assignee: nobody → emaijala
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2
Priority override, can't devote time right now, sorry :(
Ok. Per smaug this is not a DOM bug, it's a win32 widget level bug, so reassigning.
Assignee: emaijala → nobody
Component: DOM: Core & HTML → Widget: Win32
QA Contact: general → win32
Neil, you reviewed Ere's patch - any chance you can take a look at this? It's a blocker without an owner right now.
Assignee: nobody → neil
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.
(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...
(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...
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.
(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.
I'll take it.
Assignee: neil → jmathies
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.
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.
Attachment #412911 - Flags: review?(emaijala)
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.
Attachment #412911 - Flags: review?(emaijala) → review-
(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.
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.
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.
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.
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 ?.
Flags: blocking1.9.2+ → blocking1.9.2?
> 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.
Attachment #371951 - Attachment is obsolete: true
...and ditto IE8.
(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.
Not a blocker for 3.6 final, but definitely something we need to fix.
blocking2.0: --- → ?
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.
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.
Jeff and I just tested - Safari and Chrome both end with focus on the alert box too.
We shouldn't block on this, since we're not even sure we can fix it.
blocking2.0: ? → -
My revised test case https://bugzilla.mozilla.org/attachment.cgi?id=413298 now seems to be working OK
(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 ?
(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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.