99848 - 'Exit' disabled on quicklaunch systray context menu if browser windows are open

Status: RESOLVED WONTFIX

(Core Graveyard :: QuickLaunch (AKA turbo mode), defect)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Launch Mozilla.
2. Enable Quick Launch if it's not already enabled.
3. Right-click the Quick Launch icon in the system tray.

Result: 'Exit Mozilla' is disabled.

Expected: 'Exit Quick Launch' is enabled.  Selecting it exits Quick Launch but 
not Mozilla.

Comment 1

[Blake Ross]

-> me

Bill, what do you think about changing the current hackish way (which you 
alluded to) that windows set callback functions -- by setting a random property 
on the dom window -- to a function on nsIXULWindow, so we can just call this 
from C++ easily?

Comment 2

[Bill Law]

That might work.  We probably still need to be able to get to it from JS also so
that the code that handles File->Exit can still work (or, that code needs to
change to use the nsXULWindow scheme).

So would the nsXULWindow::xxxx implementation just call the js function (if
defined)?

We definitely need to run this by danm; adding him to cc: list.

Comment 3

[Blake Ross]

Yeah, it would just call the js function.  As for calling it from js, we can
just call that method on nsIXULWindow if we can get to an nsIXULWindow from js.
 I asked Ben who asked Dan who said that it was possible, but ugly ...

Comment 4

[Dan M]

  There's a step missing -- I don't know what hackish attribute is set in the
current implementation. Or what you need to do or why, for that matter. But if
you gotta add something to nsIXULWindow, then you gotta, one imagines. Try to
keep it from getting too ugly now.
  Dan's secret method for getting to nsIXULWindow from script (hold on...):

window.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIWebNavigation).QueryInterface(Components.interfaces.nsIDocShellTreeItem).treeOwner.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIXULWindow);

  happens to work. In real life of course you'd want to bust it up into separate
statements and insert null checks. The part where an nsIWebNavigation happens to
actually be an nsIDocShellTreeItem is especially suspect.
  Also, I notice a window.XULBrowserWindow property set on navigator browser
windows. It looks to me like it's not what the name would suggest, but if it is,
there ya go.
  Obligatory disclaimers: it's gross to ask for a XUL Window from a DOM window;
inapplicable in embedding situations; don't tell anybody you heard this from me;
unsupported (but probably won't break); look out! eek!

Comment 5

[Bill Law]

That's a good one, Dan.  Just further proof of my theory that you can get from
any object+interface to any other via some set of QIs, GIs, and getter functions.

But I don't think we should do that.

Dan, you care about this because I think you'll need this code to fix bug 14807.

But maybe we don't need to get to the .xul window from our .js that is currently
doing this.  That code could continue to call the .js functions directly.

That's a little bogus because theoretically one could have a derived
nsIXULWindow that didn't implement this new method in that way.  But I don't
think we'd claim our globalOverlay.js code (or wherever it is) is guaranteed to
work with such windows in any case.

Another alternative is to put the "exit" logic in some nsAppShellService method.
 Basically, something like QuitAfterAsking() which would be basically Quit()
with the extra call to the window before closing it.

Comment 6

[Blake Ross]

> Another alternative is to put the "exit" logic in some nsAppShellService method.
> Basically, something like QuitAfterAsking() which would be basically Quit()
> with the extra call to the window before closing it.

Yeah, this would also be okay. But in this case are you still suggesting that we
implement a method on nsIXULWindow?  Or, what would be calling on the window
that you refer to?

Comment 7

[Bill Law]

Yes, we'd still need a new nsIXULWindow method (implemented by calling the
javascript property).  nsAppShellService::QuitAfterAsking would have to call that.

The only difference is that now no JS code would have to get the nsIXULWindow
interface.

Comment 8

[Blake Ross]

Yes, but then how does the window specify what js to call?  The idea was that 
nsIXULWindow would have an onLastWindowClosing(nsIQuitObserver foo) method that 
the window could call in its onload, like it currently does window.tryToClose = 
foo.  But if we don't want to get the nsIXULWindow from js, then I don't see 
how later (in QuitAfterAsking), we'll know what to call.

Comment 9

[Blake Ross]

This all seems pointless, anyway.  Shouldn't returning false from a window's 
onclose or onunload do the right thing and prevent closing and, therefore, 
shutdown?

Comment 10

[Dan M]

ooo. Like I said, I don't follow the whole line of reasoning in this bug, so I'm
probably not completely clear. But comment 8 worries me. "Am I the last window"
state seems out of place on a window, especially in the interface. The
appshellservice already has knowledge of the open windows and knows about
last-window-closed conditions (and has overrides and other complicated malarky
that you probably don't want to be second-guessing somewhere else). As Blake
mentions in comment 9, it's a known problem that we don't respond to the
system's OK-to-close queries quite right. Is that the issue?

Comment 11

[Blake Ross]

The issue is that we'd like to enable the Exit menuitem in the native
QuickLaunch menu for its icon, but we can't, because we have no way of asking
the window if it's ready for shutdown.  This is because the way windows
currently set their callback function is by setting a property on the window,
tryToClose.   IOW, they call window.tryToClose = CanClose(); in their onload,
where CanClose() returns a bool saying whether shutdown can continue or not.  We
can easily call the window.tryToClose function from globalOverlay.js's shutdown
code because it's already in js, but accessing that from C++ would be a real pain.

(Also, I didn't mean onLastWindowClosing(), I meant onWindowClose() in comment
8. Bad turbo memories.)

Comment 12

[Bill Law]

re: comment 8

I was envisioning the nxXulWindow method looking for the "tryToClose" property
and invoking it if present.  Basically, a C++-callable means to do what is done
in the JS code that gets invoked when one does File->Exit.

That's not as clean as what you suggest, though.

re: comment 9

That sounds good.  I'm not sure what the semantics of the onclose handler are
(onunload is called after the decision has been made, so I don't think it can
block closing).  I also don't know if there is a way to trigger the onclose
handler when closing a window programmitically (other than via "window.close()"
in the window itself).

E.g., whatever happens downstream from the Close() call at
http://lxr.mozilla.org/seamonkey/source/xpfe/appshell/src/nsAppShellService.cpp#349
does not result in the onclose handler getting called, as best I can determine.  

re: comments 10/11

And note that what we want to do when the user selects "Exit" on the systray
icon is pretty much what we want to do when the user does Windows
shutdown/logoff/etc.  Once the mechanism is in place, then we can use it both
places.

Comment 13

[Blake Ross]

So, Dan, what do you think?  Is this something that belongs to you, or can you
offer insight on how to hook it up so returning false from a window's onclose
prevents window closure (and, if applicable, application shutdown)?

Comment 14

[Jesse Ruderman]

Did this bug morph while I wasn't reading my bugmail?  In comment 0, I said
"Exit Mozilla" on the Quick Launch menu should be changed to "Exit Quick Launch"
and always enabled.  It sounds like you're trying to make it "Exit Mozilla" and
always enabled.

Comment 15

[Bill Law]

Perhaps we inadvertantly jumped to a wrong conclusion about how to fix this.

Blake: Maybe a simpler fix would be to change the wording and just turn off
Quick Launch?

Comment 16

[Blake Ross]

The stuff about window close procedure should I think be fixed anyway, in the
long run. Whether we want to tackle it now (I don't think it's much work if Dan
can just give me some pointers...) is another question.  We could just remove
the exit menuitem; I think fixing this would be best.

Comment 17

[Peter Trudelle]

Nav Triage team:  marking nsbeta1+

Comment 18

[Blake Ross]

Dan, can you answer comment 13 when you get a moment?

Comment 19

[Blake Ross]

So onclose="return false;" already does prevent window closure. Who knew.

But returning false from onunload doesn't halt the quitting process.  See
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocumentViewer.cpp#1334
- we don't check for eConsumeNoDefault event status.  This is presumably because
we don't want to give webpages the ability to prevent shutdown.  So it seems
like what we want to do is check if it's a xul doc and, if so, check the event
status and halt quitting if necessary.  But how do we propagate that back to the
caller?  Is returning failure the right thing to do?

Comment 20

[Blake Ross]

Another alternative, by the way, is putting the tryToClose property on the new
nsIDOMChromeWindow.

But I still think onunload="return false;" is in line with established protocol.

Comment 21

[Peter Trudelle]

nsbeta1- per ADT triage team. ->1.2

Comment 22

[http://tinymailto.com/oliversl]

(Comments on original message)
I think this way works fine.
I you want to close al Mozilla Windows:
-> go to File->Exit
If you want to close quick launch but no other window
-> select Disable quick launch from the QL context menu or in the preference window

Tested in Mozilla 1.0 RC2

Comment 23

[Bill Mason]

Today in build 2002061908 PC/Win98 I encountered this bug for the first time. 
However, it is happening regardless of whether I have any browser windows open
or not.

I tried deleting and reinstalling it and using a new profile, but the problem
persists.

Comment 24

[http://tinymailto.com/oliversl]

I think this bug should be closed, because the reported wanted to close 
QuickLaunch when Mozilla is running, and this is perfectly posible.

To close only QuickLaunch while Mozilla is running:
1. Right click on the QuickLaunch icon on the taskbar
2. Select: Disable QuickLaunch. This will close QuickLaunch

WFM in Mozilla 1.0 and Mozilla 1.0.1RC1 running on WinXP

Comment 25

[Jesse Ruderman]

Oliver: "Disable Quick Launch" permanently disables quicklaunch, while the
"Exit" item on the quicklaunch menu just closes quicklaunch until the next time
you start Mozilla.  The bug is that "Exit" is disabled while there are Mozilla
windows open.

Comment 26

[http://tinymailto.com/oliversl]

Comment #25
Hi Jesse, understand. Could it be posible that this behavior is a "feature", not
a bug? Anyway, in 1.2.1, Mozilla works just like you explain.

Comment 27

[Matthew Kerr]

Also seen with nightly build 2003011305 on Windows XP SP1.

Comment 28

[Eric Towers]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0.2) Gecko/20021216
Win98SE

"Exit Mozilla" item is *never* active.

Also, mail windows cannot be opened after first mail window is closed. 
Terminating the Quick Launch tray item permits mail windows to be opened again
(only once).  (This, of course restarts the Quick Launch component.)  Terminated
mail windows show up on Window menu as empty, unselectable items.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0.2)

The 1.0 branch is basically in maintenance mode -- security and crash fixes
only.  This is neither...

Comment 30

[Samuel Sidler (old account; do not CC)]

Isn't this the desired behavior? To exit Mozilla completely, you'd have to close
the quicklaunch and all browser windows (or whatever other windows there may
be). Thus, you must close everything before you can completely "exit mozilla". I
think this is the desired behavior because it keeps users from accidentally
closing everything.

In any case, this bug has been quiet for a while now, please update on the
status of it.

Comment 31

[Matthias Versen [:Matti]]

Quicklaunch/Turbo Mode is no longer supported in Seamonkey 2 and Seamonkey1.X is in the maintenance mode (fixing only security bugs)