Closed Bug 350299 Opened 14 years ago Closed 14 years ago

Software Update wizard no longer loses focus after clicking "Restart Now", if Firefox was not focused when the wizard appeared

Categories

(Toolkit :: Application Update, defect)

1.8 Branch
x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.8.1

People

(Reporter: Gavin, Assigned: moco)

Details

(Keywords: fixed1.8.1, regression)

Attachments

(4 files, 1 obsolete file)

I've noticed this in the past few 1.8 branch nightlies - after downloading the update and clicking "Restart Now", with multiple windows open each containing multiple tabs, it seems like nothing happens. The I moved the software update dialog away and noticed that the "Confirm close tabs" dialog had appeared behind it.

The JS console shows an error about window.setTimeout() throwing NS_ERROR_NOT_INITIALIZED, at line http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/wizard.xml&rev=1.30&mark=269#255 . That scares me a little bit.

I will try to see if I can reproduce this consistently.
Turns out this only happens when the Firefox window doesn't have focus when the software update dialog appears.
Summary: Software Update dialog no longer closes after clicking "Restart Now" → Software Update dialog no longer loses focus after clicking "Restart Now" while Firefox is minimized and/or in the background
Summary: Software Update dialog no longer loses focus after clicking "Restart Now" while Firefox is minimized and/or in the background → Software Update wizard no longer loses focus after clicking "Restart Now", if Firefox was not focused when the wizard appeared
Nominating for blocking: drivers, this is pretty bad, since the width of the software update dialog almost completely occludes the "do you want to close n tabs" dialog making it potentially impossible to see why it's not working.
Flags: blocking-firefox2?
Target Milestone: --- → Firefox 2
Flags: blocking-firefox2? → blocking-firefox2+
Tested this with 1.5.0.5, and in that build, the Software Update dialog disappears when you click "Update Now" (which is also what it does on Mac).

cc'ing seth, as he was the last dude to touch this code.
taking and investigating.
Assignee: nobody → sspitzer
here's what I think is going on here:

when the user clicks "Restart XXX Now", we call gFinishedPage.onWizardFinish()

this code notifies all the observers that the app is trying to quit, and then tries to close all the windows.

When the browser doesn't have focus, this causes the "confirm close" prompt for the windows to come up "behind" the software update dialog.

One low risk fix that I'm trying is to switch focus to the various windows before we call tryToClose().

See http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.js#1595
Status: NEW → ASSIGNED
> The JS console shows an error about window.setTimeout() throwing
> NS_ERROR_NOT_INITIALIZED, at line
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/wizard.xml&rev=1.30&mark=269#255
. That scares me a little bit.

That scares me too, but I haven't seen it yet.
this should also be affecting tbird, with the tryToClose handler for the compose window.
I thought I could reproduce (following gavin's tips) but I haven't been able to make it happen again.

I'm also not sure that focusing the window before calling win.tryToClose() is the right fix.  (looking at the code base in lxr, we use the nsIWindowMediator to enumerate all the windows in several places, and no one seems to need to focus the windows.)

but, I do see a related scenario that needs fixing that could also fix gavin's scenario.

here is the related scenario:

1)  open a window and open many tabs (make sure your "warn on close" pref is enabled)
2)  minimize the window
3)  wait for software update
4)  ...resume scenario per gavin

with the window minimized, the software update dialog does not lose focus (or move to the back) when you do "Restart Now".  (This is why I think it is related to gavin's original scenario)

the "confirm close" prompt (at least for me and my window manager on windows xp) opens up at the top left, and is "behind" in z order the software update window.

when the user clicks the "Restart Now" button, the onWizardFinished() attempts to quit and close before window.close() is finally called on the wizard window.  

when in limbo, and the "confirm close" prompts are up, the software update (if you switch to it) still has the "Restart Now" and "Later" buttons enabled.

I'm wondering, looking at the EM code, if we should be calling tryToClose() at all?  (the EM seems to check if we can quit, and then does the restart | quit.

I'll try this out, and report back.
When I was testing, the case that seemed to trigger the problem most (and that led me to file the bug) was getting the prompt with several Firefox windows open without focus, but not minimized. E.g. have two Firefox windows open with multiple tabs in each, then open Notepad in the foreground and wait for the alert to appear. Then, Alt+Tab directly to the Update wizard and click "Restart now".
my "remove call to tryToClose" has (at least) two problems.

1)  gavin points out, we want this prompt, see bug #341248 (where we decided to keep the prompt, as session restore is not perfect.)

2)  tbird, and other apps, don't have session restore yet.
Attached patch patch, comments to follow (obsolete) — Splinter Review
Attachment #237024 - Flags: review?(gavin.sharp)
I've tried a couple things, including doing the window.close() before the tryToClose(), but that led to the scary:

###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!

Here's what I've got with that patch:

1)  the change to updates.js disables the two buttons when in limbo.  From my comment #8: 

'when in limbo, and the "confirm close" prompts are up, the software update (if
you switch to it) still has the "Restart Now" and "Later" buttons enabled.'

2)  in tabbrowser.xml, in the warnAboutClosingTabs() method, if I'm about to prompt, I first call window.focus().  this will raise minimized windows and should take care of gavin's scenario where the prompt gets hidden behind the software update wizard.  it also has a side benefit of making it clear (for minimized windows) which browser window the prompt is associated with.

the other place that calls warnAboutClosingTabs() is when you do "close other tabs", but in that case, the window already has focus, so there is no harm.

I think this is the right place for the window.focus() [and not in updates.js], because we only want to raise the window if we are going to show the prompt.

Note, it appears that tbird already has a window.focus() in the tbird compose window tryToClose handler, see http://lxr.mozilla.org/mozilla1.8/source/mail/components/compose/content/MsgComposeCommands.js#2500

Also note, that the ideal fix would be more involved (and more risky).  See ben's comment in onWizardFinish(), at http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.js#1599
another place that needs the window.focus() is the update wizard itself.

for example, if you minimize the software update wizard while downloading, and then quit, we have a similar issue.

http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.js#1292
Attachment #237024 - Attachment is obsolete: true
Attachment #237075 - Flags: superreview?(mconnor)
Attachment #237075 - Flags: review?(gavin.sharp)
Attachment #237024 - Flags: review?(gavin.sharp)
Whiteboard: [fix in hand, awaiting reviews]
Comment on attachment 237075 [details] [diff] [review]
patch, includes adding focus() to the trytoclose in the update wizard.

I'm wondering if the prompt service should take care of focusing the parent window of parented alerts, but that would be changing the behavior for all callers, which certainly isn't suitable for the branch at this stage, and isn't necessarily correct for all cases (this seems to be bug 87592). I'm also kind of worried that stealing focus might be the wrong thing to do in some cases, calling focus() directly has led to accessibility issues in the past (for users of screen readers, for example), but I guess that since it only happens after an explicit user action (clicking the "restart now" button) and only before an alert appears, this should be OK.

I'm not really clear on the second scenario you describe (and therefore am not sure that the second hunk in that patch is correct), could you elaborate? Does quitting Firefox without focusing the software update dialog really cause onWizardCancel to be called? In other words, how is onWizardCancel called without the user clicking on the cancel button (in which case the window is already focused)?
Attachment #237075 - Flags: review?(gavin.sharp) → review+
Comment on attachment 237075 [details] [diff] [review]
patch, includes adding focus() to the trytoclose in the update wizard.

I think I agree about the promptservice behaviour gavin mentioned, please file a followup there, and mention that we should back out these calls if/when we change that behaviour
Attachment #237075 - Flags: superreview?(mconnor) → superreview+
gavin and mconnor:  thanks for the reviews.

> I'm not really clear on the second scenario you describe (and therefore am not
> sure that the second hunk in that patch is correct), could you elaborate? Does
> quitting Firefox without focusing the software update dialog really cause
> onWizardCancel to be called? In other words, how is onWizardCancel called
> without the user clicking on the cancel button (in which case the window is
> already focused)?

yes, this scenario can happen if you start software update over a slow link, minimize the window, and then attempt to quit the application.  onWizardCancel will be called in that scenario.  I'll go get some additional details of the calls stack to onWizardCancel in my scenario.

I'm not convinced (yet) that we want the prompt service to focus the calling window.  It seems reasonable, but I haven't wrapped my head around it.  But for the tryToClose() callers, it is what we want.  I will log a spin off bug to discuss doing that, and to include a note about removing the manually callers to window.focus() before the prompt service calls.

ok, some more info about the updates.js side of the fix and the associated scenario.

yes, we still want to focus the window.  here's why:

in the download is happening, and the software update wizard is minimized, quitting the app *will not* cause a prompt to show up because onWizardCancel() for the downloading page of the wizard doesn't have code to prompt.  

The way to get the prompt (screen shot coming) on exit is if you pause the download, minimize, then quit (or if you cancel the dialog when paused, same code path.)

see
http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.js#1285

Note, the the "quit" scenario, hitting "yes" the prompt will not stop the closing of the app.  We should pass up the result of the prompt up to onWizardCancel(), and back up to tryToClose(), so that if you hit "yes" we will not shut down the app.  I'll spin up a new bug on this issue.

Note, in the quit scenario, the wording of this prompt needs some work.  I'll log a new bug on this issue.

I'm going to check the r/sr patch into the trunk, and then seek approval for the branch.
did this make it into trunk yet?
Whiteboard: [fix in hand, awaiting reviews] → [has patch][checkin needed]
> did this make it into trunk yet?

sorry for the delay, I just landed it.  

Here are the scenarios that I re-tested on the trunk that this patch addresses:

1)  when a browser window is minimized (or "behind" in the z-order), we will raise the window if we attempt to confirm the close of multiple tabs on quit.  (adding window.focus() fixes this.)

2)  when the software update window is minimized (or "behind" in the z-order), we will raise the wizard if we attempt confirm a paused download on quit. [as noted earlier, the wording of this prompt is good for cancel, but not quitting the app.  spin of bugs coming...]

3)  In the software update wizard after downloading an update and upon clicking "Restart Now", we disable the "Restart Now" and "Later" buttons so that while the user is dealing with various "try to close handlers", the user can't switch back to the software update wizard and click the "Restart Now" and "Later" buttons.

I'll go log the 3 or 4 spin off bugs previously mentioned in this bug (non of which block ff2) and then seek approval for the branch.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][checkin needed]
Comment on attachment 237075 [details] [diff] [review]
patch, includes adding focus() to the trytoclose in the update wizard.

seeking approval
Attachment #237075 - Flags: approval1.8.1?
Comment on attachment 237075 [details] [diff] [review]
patch, includes adding focus() to the trytoclose in the update wizard.

a=beltzner on behalf of 181drivers
Attachment #237075 - Flags: approval1.8.1? → approval1.8.1+
fix landed on the branch.

gavin or adam:  if you guys know of any "when I quit the app, the 'confirm close' prompt is at position 0,0" or "when I quit the app, the 'confirm close' prompt does not appear modal to the browser window" bugs, this fix would fix those issues.
Keywords: fixed1.8.1
In this case, the "Confirm Close" dialog was slightly visible. However, clicking on it would not bring it forward even though it has focus.
Even after loading Photoshop, the dialog remains on top of all other windows. Notice that the hidden Photoshop dialog appears to have focus.
Comment on attachment 239371 [details]
"Update Downloaded" dialog covering "Confirm Close" dialog

This and the other "Update Downloaded" image were created after applying an update from the 2.0b nightly channel.


FF windows open:
* one browser window with multiple open tabs.
* I do not believe FF had focus, but I was away from my desk when SoftUpdt fired

Clicked [Update & Restart Bon Echo now].

... here are the last two entries from updates.xml ...
<update type="minor" name="Bon Echo 2.0" version="2.0" extensionVersion="2.0" serviceURL="https://aus2.mozilla.org/update/2/Firefox/2.0/2006091905/WINNT_x86-msvc/en-US/nightly/Windows_NT%205.1/update.xml" installDate="1158773232165" buildID="2006092004" isCompleteUpdate="false" channel="nightly">
<patch type="complete"
URL="http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2006-09-20-04-mozilla1.8/firefox-2.0.en-US.win32.complete.mar" size="7379034" selected="false" state="undefined"/>

<patch type="partial"
URL="http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2006-09-19-05-mozilla1.8/firefox-2.0.en-US.win32.partial.2006091905-2006092004.mar" size="7434" selected="true" state="succeeded"/>
</update>

<update type="minor" name="Bon Echo 2.0" version="2.0" extensionVersion="2.0" 
serviceURL="https://aus2.mozilla.org/update/2/Firefox/2.0/2006091804/WINNT_x86-msvc/en-US/nightly/Windows_NT%205.1/update.xml" installDate="1158680072403" 
buildID="2006091905" isCompleteUpdate="false" channel="nightly">
<patch type="complete"
URL="http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2006-09-19-05-mozilla1.8/firefox-2.0.en-US.win32.complete.mar"
size="7378593" selected="true" state="succeeded"/>
</update>
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.