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

VERIFIED FIXED

Status

VERIFIED FIXED
18 years ago
14 years ago

People

(Reporter: bzbarsky, Assigned: paulkchen)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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 2

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

r=jag
(Reporter)

Updated

18 years ago
Keywords: approval, patch

Comment 3

18 years ago
Related: bug 34151
Alec, could you sr= this?

Comment 5

18 years ago
so we are certain (somebody check cvsblame?) that the ONLY reason that we focus
the window is for tryToClose()?
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

18 years ago
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).. 
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?

Keywords: approval → review
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

18 years ago
awesome! sr=alecf with that indentation fix
Created attachment 28918 [details] [diff] [review]
patch with indentation fixed
This is checked in and is fixed in 2001-04-02-16 on Linux....
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
QA Contact: sairuh → claudius
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.]
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.