73237 - we should not call window.focus() on all windows right before closing them on shutdown

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now, at application shutdown, we cycle through all the windows and focus
each one before we close it.  This will unminimize and try to re-render
minimized windows, which is totally unnecessary.

Attaching patch to fix this.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 2

[Peter "jag" Annema]

This was also discussed in one of the newsgroups I think. This is the solution I
came up with back then.

r=jag

Comment 3

[R.K.Aa.]

Related: bug 34151

Comment 4

[Boris Zbarsky [:bzbarsky]]

Alec, could you sr= this?

Comment 5

[Alec Flett]

so we are certain (somebody check cvsblame?) that the ONLY reason that we focus
the window is for tryToClose()?

Comment 6

[Boris Zbarsky [:bzbarsky]]

cvsblame just says that blake touched that when he landed the .value/.label thing.

The time before that was the fix to bug 8673 (change from revision 1.28 to
revision 1.29 of the relevant file), which has the checkin comment:

"8673   ShutDown() should try to close all open windows. r= hangas"

Before that we never actively focused the window, but the change from revision
1.7 to revision 1.8 includes the following commented code:

+       //      if ( topWindow.tryToClose == null )
+       //      {
+       //             topWindow.close();
+       //      }
+       //      else
+       //      {
+       //              // Make sure topMostWindow is visibile
+       //              // stop closing windows if tryToClose returns false
+       //              topWindow.focus();
+       //              if ( !topWindow.tryToClose() )
+       //                      break;
+       //      }

and the checkin comment "12893   [DOGFOOD] File --> Exit causes ender to get
loaded ???"

No mention anywhere of _why_ we're focusing before trying to close.

Does tryToClose() pop up any dialogs when it fails?  If it does, is the idea
that we need to have the window open and on top when the dialog pertaining to it
pops up?  If that is the case, it should be the responsibility of tryToClose()
to focus its window as necessary, in my opinion.

Comment 7

[Alec Flett]

I think I agree actually - leave it up to the implementors of tryToClose()...
then the above patch becomes much simpler.. we just have to fix all the
implementors (editor and mail compose I suspect).. 

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: updated patch

Comment 9

[Boris Zbarsky [:bzbarsky]]

Editor and mailcompose it was indeed.

Updated patch fixed all (both) of the places where TryToClose is defined to call
window.focus() right before popping up the confirmation dialog and removes the
call to window.focus() from the global overlay.  The focusing should only be
happening if we have modified content and are going to pop up a dialog now. jag,
care to review?

Comment 10

[Peter "jag" Annema]

Looks okay... r=jag. Indentation nit (just use tabs, sigh):

+++ mailnews/compose/resources/content/MsgComposeCommands.js    2001/03/27
02:51:42
@@ -1397,6 +1397,10 @@
        // Returns FALSE only if user cancels save action
        if (contentChanged || msgCompose.bodyModified)
        {
+        // call window.focus, since we need to pop up a dialog
+        // and therefore need to be visible (to prevent user confusion)
+        window.focus();
+        
                if (commonDialogsService)

Comment 11

[Alec Flett]

awesome! sr=alecf with that indentation fix

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: patch with indentation fixed

Comment 13

[Boris Zbarsky [:bzbarsky]]

This is checked in and is fixed in 2001-04-02-16 on Linux....

Comment 14

[sairuh (rarely reading bugmail)]

mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31.

if you think this particular bug is not fixed, please make sure of the following
before reopening:

a. retest with a *recent* trunk build.
b. query bugzilla to see if there's an existing, open bug (new, reopened,
assigned) that covers your issue.
c. if this does need to be reopened, make sure there are specific steps to
reproduce (unless already provided and up-to-date).

thanks!

[set your search string in mail to "AmbassadorKoshNaranek" to filter out these
messages.]