Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Description • 14 years ago

After removing the quit warning dialog, people are losing data since they've come to depend on that dialog.

We should show the "closing multiple tabs" warning in the case of closing the last window.

Hopefully this along with adding the "restore previous session" button on the home page will make the people happy while still giving the streamlining of "I'm intentionally quitting via file|exit or the cmd/ctrl-Q" case.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 1 • 14 years ago

This is could be duped to bug 384907 but that's awfully noisy, and I'd rather just fix it then here a bunch of "finally" comments. Then we can dupe that here.

	eyal gruss
	Comment 2 • 14 years ago

is the dialog also intended to show when firefox is set to "show my windows and tabs from last time"? this would seem a bit redundant.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 3 • 14 years ago

(In reply to comment #2)
> is the dialog also intended to show when firefox is set to "show my windows and
> tabs from last time"? this would seem a bit redundant.

No. The change as I understand it is just for the last window when your session would not be restored.

	Luke Iliffe (Harlequin99)
	Comment 4 • 14 years ago

Some of the confusion arising from bug 592822 that I have seen from forum posts and bug reports is people think the UI option in the tabs pref pane to warn on closing multiple tabs is now no longer working. When it is explained that that pref (b.t.warnOnClose) is, and always was, only for secondary windows not the final window it doesn't make much sense to them.

Would a relatively simple solution be for that UI pref pane checkbox to control both browser.warnOnQuit and browser.tabs.warnOnClose? It would still maintain separate prefs for those that choose to always restore a previous session (thus allowing a prompt when closing secondary windows with tabs but no prompt on quitting), but (un)checking a single UI option would give consistent behaviour for the default setting.

Bug 628080 maybe related to this in as much as it talks about changing the default setting and possibly the behaviour of browser.tabs.warnOnClose.

Just some thoughts.

	Sheila Mooney
	Comment 5 • 14 years ago

If this is indeed a hardblocker, can we triage it out of the nom queue? Should it be BetaN or Final?

	Dão Gottwald [:dao]
	Comment 6 • 14 years ago

Please don't set hardblocker/softblocker on bugs that don't actually block.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 7 • 14 years ago

(In reply to comment #5)
> If this is indeed a hardblocker, can we triage it out of the nom queue? Should
> it be BetaN or Final?

I would say betaN.

(In reply to comment #6)
> Please don't set hardblocker/softblocker on bugs that don't actually block.

I had hear that if we nom something we should also put our thoughts on hard vs soft. Marked with [hardblocker?] to denote that.

	Dão Gottwald [:dao]
	Comment 8 • 14 years ago

(In reply to comment #7)
> (In reply to comment #5)
> > If this is indeed a hardblocker, can we triage it out of the nom queue? Should
> > it be BetaN or Final?
> 
> I would say betaN.
> 
> (In reply to comment #6)
> > Please don't set hardblocker/softblocker on bugs that don't actually block.
> 
> I had hear that if we nom something we should also put our thoughts on hard vs
> soft. Marked with [hardblocker?] to denote that.

Still shows up on https://bugzilla.mozilla.org/buglist.cgi?quicksearch=sw%3Ahardblocker
But it's not just inconvenient. There was a nice little story on slashdot linking to such a bug list. Then again, it caught even more false positives, as just mentioning "hardblocker" in a comment would put a bug on that list... http://it.slashdot.org/story/11/01/19/1320229/Firefox-4-A-Huge-Pile-of-Bugs

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 9 • 14 years ago

This is essentially just what I had done in an earlier version of my patch in bug 592822. I think it's doing what we want here but I haven't actually tested it yet.

Gavin - I know we wanted to avoid changing logic in here, but it seems pretty unavoidable.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 10 • 14 years ago

Comment on attachment 508463 [details] [diff] [review]
Patch v0.1

I think this looks mostly fine, except for the part that loses the "browser.warnOnQuit=false means we will never prompt in any case" characteristic, which I think is necessary to keep (with this patch as-is, we would start prompting in the restart case if warnOnQuit=false but warnOnRestart=true).

I would refactor the logic such that showPrompt is defined as:

var showPrompt = Services.prefs.getBoolPref("browser.warnOnQuit") && !(aQuitType == "restart" && !Services.prefs.getBoolPref("browser.warnOnRestart"));

and have that after the aQuitType=="lastwindow" check.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 11 • 13 years ago

(In reply to comment #10)
> Comment on attachment 508463 [details] [diff] [review]
> Patch v0.1
> 
> I think this looks mostly fine, except for the part that loses the
> "browser.warnOnQuit=false means we will never prompt in any case"
> characteristic, which I think is necessary to keep (with this patch as-is, we
> would start prompting in the restart case if warnOnQuit=false but
> warnOnRestart=true).

I know we talked about this before, but I still *really* hate that warnOnQuit is supposed to override in that case. We have different dialogs. I would expect warnOnRestart to warn me before restarting regardless of the value of warnOnQuit. warnOnQuit is no longer a global override with this bug anyway.

We're changing logic in there now anyway, so if we're ever going to fix it, now seems like the most appropriate time. Even though this bug is specifically about showing the close window dialog, I still think we should do it - in a new bug if we have to.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 12 • 13 years ago

First off...
I talked with Gavin on IRC. We're going to keep warnOnQuit as a global override and introduce a new pref for the quit dialog. This is going to mean switching warnOnQuit back to true and having the new pref be false. I'm going to do that work here even though it's not directly about the window closing dialog (I'll morph the bug a bit later).

Secondly...
Remember when we all thought browser-lastwindow-close-requested was notified when the last window was closing? Yea that was fun. That was my mistake. browser-lastwindow-close-requested is only notified when the last window is closed but not actually quitting (other windows open). When closing the last window quits, we just follow the normal quit-application-requested path.

So what I did here is not going to work in the case we want. I have an idea though, but so far it requires modifying globalOverlay. Patch tomorrow.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 13 • 13 years ago

The change to globalOverlay.js here sends "lastwindow" as the data with the quit-application-requested notification, which allows us to distinguish quitting via the X on the window vs file > exit/quit & cmd/ctrl+q

We then use that in nsBrowserGlue to make sure that we use a quit type of "lastwindow" to determine which dialog to show (if any). I think the comments I added in there should make it pretty clear the process we take to determine which dialog to show.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 14 • 13 years ago

Comment on attachment 509508 [details] [diff] [review]
Patch v0.2
[Checked in: See comment 16]

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>   _onQuitRequest: function BG__onQuitRequest(aCancelQuit, aQuitType) {

>+    // If the session is going to be restored, then we will not show any dialogs
>+    // regardless of other conditions. browser.warnOnQuit == false also overrides
>+    // other possibilites of the prompt showing. Otherwise these are the conditions
>+    // that will result in a dialog being shown:

We should probably list the cases we explicitly don't show *any* dialog:
- session will be restored (resume_session_once or browser.startup.page=3)
- there is only one tab open
- browser.warnOnQuit is false
- we're in private browsing mode

>+    // 1. aQuitType == "lastwindow" or "quit" and browser.showQuitWarning == true
>+    //    - The quit dialog will be shown
>+    // 2. aQuitType == "restart" && browser.warnOnRestart == true
>+    //    - The restart dialog will be shown
>+    // 3. aQuitType == "lastwindow" && browser.tabs.warnOnClose == true
>+    //    - The "closing multiple tabs" dialog will be shown

This seems to be overloading "lastwindow" - it's now sent both for "last window is closing, but we're not quitting" and also "we're quitting (via closing last window)"... I guess that's not a problem, probably worth a comment?

>+    var showPrompt = false;
>+    var mostRecentBrowserWindow;
>     try {

I think this try/catch is unneeded, can you just remove it?

>+      if (aQuitType == "lastwindow") {
>+        mostRecentBrowserWindow = Services.wm.getMostRecentWindow("navigator:browser");
>+        aCancelQuit.data = !mostRecentBrowserWindow.gBrowser.warnAboutClosingTabs(true);

Do we want to do this in private browsing mode? I think not. Should just move the PB mode check to earlier in this function, I think.

I think the logic would be clearer as:

if (aQuitType != "restart" && showQuitWarning)
  showPrompt = true;
else if (aQuitType == "restart" && warnOnRestart)
  showPrompt = true;
else if (aQuitType == "lastwindow") {
  ...
}

r=me with these addressed. We should probably get a couple of followup bugs on file:
- remove the lastwindow-close-requested stuff, since I don't think we need to support showing a dialog in that case, and it would simplify these code paths significantly.
- remove support for showing the quit dialog entirely, and simplify the shutdown path even further.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 15 • 13 years ago

That all makes sense. Thanks for the review. I know exactly how painful all of this dialog stuff is and I look forward to those followups.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 16 • 13 years ago

Pushed http://hg.mozilla.org/mozilla-central/rev/84921e24be9c with the changes requested.

I filed bug 632270 and bug 632271 for those followups.

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 18 • 13 years ago

I just lost all my tabs due to this change.  I got the single-tab page on startup, i.e. the new so-call Home page, and clicked 'restore'.  

That worked fine, but the bug about opening the session in a 'new window' instead of the current one with only one tab rose up to bite me.  When I closed the browser again I forgot, and also got no warning I was closing 'multiple' windows, thus when I closed the one with all my tabs, there remained the stupid 'home page' window.  Closed that.. and BAM lost all my tabs.

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 19 • 13 years ago

With browser.showQuitWarning set to true and
browser.warnOnQuit set to true 

I'm not getting any warning box... I'm closing the application like probably 95% of windows usere with the X in the upper right corner, if that matters.

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 20 • 13 years ago

(In reply to comment #19)
> With browser.showQuitWarning set to true and
> browser.warnOnQuit set to true 
> 
> I'm not getting any warning box... I'm closing the application like probably
> 95% of windows usere with the X in the upper right corner, if that matters.

Yes, it DOES matter - using File->Exit does produce the dialog box.  IMO, the X should do the same thing.

	tabmix.onemen
	Comment 21 • 13 years ago

the is bug in _onQuitRequest

    var quitDialogTitle = quitBundle.formatStringFromName(aQuitType + "DialogTitle", [appName], 1);

chrome://browser/locale/quitDialog.properties don't have an entry for
lastwindowDialogTitle

so when we close last window with the X aQuitType = lastwindow and this function faile to show the dialog !

	Bill Gianopoulos [:WG9s]
	Comment 22 • 13 years ago

Do you have browser.showQuitWarning set to true?

	Bill Gianopoulos [:WG9s]
	Comment 23 • 13 years ago

please read http://blog.zpao.com/post/3174360617/about-that-quit-dialog before commenting.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 24 • 13 years ago

(In reply to comment #21)
> the is bug in _onQuitRequest
> 
>     var quitDialogTitle = quitBundle.formatStringFromName(aQuitType +
> "DialogTitle", [appName], 1);
> 
> chrome://browser/locale/quitDialog.properties don't have an entry for
> lastwindowDialogTitle
> 
> so when we close last window with the X aQuitType = lastwindow and this
> function faile to show the dialog !

Well shit. Thanks for that. I had taken something out between the patch here and what I pushed because I thought that was covered. Patch in a minute.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 25 • 13 years ago

Since I messed this up, here's the patch I checked in for posterity's sake.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 26 • 13 years ago

And here's the part that should fix the problem.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 27 • 13 years ago

On IRC, Gavin said he would prefer this, so bam.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 28 • 13 years ago

Pushed the followup: http://hg.mozilla.org/mozilla-central/rev/39959be42c6f

	stephan
	Comment 30 • 13 years ago

(In reply to comment #29)
> *** Bug 632797 has been marked as a duplicate of this bug. ***

This is not the case, I reported the bug in the latest Beta Version. In the "real" release version, I am prompted for closure.  I thought I was submitting the bug report against the latest beta "4" version.  Anyone verify it has been fixed in the latest Beta Version?

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 31 • 13 years ago

(In reply to comment #30)
> (In reply to comment #29)
> > *** Bug 632797 has been marked as a duplicate of this bug. ***
> 
> This is not the case, I reported the bug in the latest Beta Version. In the
> "real" release version, I am prompted for closure.  I thought I was submitting
> the bug report against the latest beta "4" version.  Anyone verify it has been
> fixed in the latest Beta Version?

No it wasn't fixed for Beta 11, but it will be for Beta 12. That bug is a duplicate to this since we fixed the bug here (even if you don't have the fix yet, the bug still counts as fixed)

	Daniel C
	Comment 33 • 13 years ago

Here is how I see a user's thought process going when they upgrade from 3.6 to 4.0. The user has default settings apart from their homepage (I believe this to be a very common case):

1. "I have lots of tabs, but I need to close the browser and log out now. That's OK, Firefox usually asks me if I want to save my tabs."
2. Click X.
3. See "You are about to close 6 tabs. Are you sure you want to continue? [Cancel/Close Tabs]"
4. "Erm, I don't know. I usually click 'Save and Quit'. I don't want to Cancel (title says 'Confirm close') because I need to close the browser and log out now, but I don't want to Close Tabs because I want them to be there when I open up Firefox again.
5A. Click Cancel and tediously save tabs manually by bookmarking them or something.

Or,
5B. Click Close Tabs and hope for the best. "Maybe they automatically save and restore these days? I guess that could make sense."
6. Reopen Firefox. See homepage and no tabs. Assume Firefox has eaten your tabs. Swear profusely and wonder what was wrong with the "Quit/Save on Quit/Cancel" dialog.

Please tell me if you think this is an unrealistic or unlikely scenario and explain why, because I think this will occur often. How is a user who is used to the 3.6 method of saving tabs going to know that they can click "Close Tabs" safe in the knowledge that they can get their tabs back? And how to get those tabs back?

	stephan
	Comment 34 • 13 years ago

Hello:

All things considered, Firefox is still way better than IE. And I promote Firefox.

What I have a hard time understanding is why "they" can not release a sub-letter of the beta version and fix* the problem "temporarily." 

*copy the code from production version and "slip it in."  People aren't asking for a change in how this function works, just put it back in. I know as reported here - it will be "restored" in the next beta version, however I am beseeched by another problem, I don't know how I will be notified when the next beta will be out, and this beta does not offer me an opportunity to check for updates in the help menu.

Currently I work in a position where people do things and fix them later. I am sorry to report, we "Americans" are losing the ability to think and do things right the first time. The just do it mentality is good, but just do it right.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 35 • 13 years ago

We are not going to put out a beta 11.5 or anything of the sort. The fix will be in beta 12 when that goes out, hopefully in the next week. They are called betas because they are unfinished. If you're unhappy waiting, use a nightly version.

Check for updates was moved into the about dialog. I think the betas get notified of newer betas when they are released.

	stephan
	Comment 39 • 13 years ago

Well - Surprise, surprise....

I accidently discovered that if two firefox windows are opened at the SAME time; firefox prompts you to save open tabs, or quit - - - YOU must have two windows opened.

Thought I just provide this little tidbit; just in case the next beta works in reverse; multiple windows opened and Firefox just closes the window without a prompt.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 40 • 13 years ago

(In reply to comment #39)
> I accidently discovered that if two firefox windows are opened at the SAME
> time; firefox prompts you to save open tabs, or quit - - - YOU must have two
> windows opened.

That shouldn't be the case. By default we shouldn't be showing any dialogs when "quitting" (closing the last window is slightly different). Care to expand upon your situation (version, OS, value of some prefs - browser.warnOnQuit, browser.showQuitWarning, browser.tabs.warnOnClose)

	stephan
	Comment 41 • 13 years ago

Windows 7 64bit Home Premium Version is being used. I have not accessed the "settings window/tab", but have selected the option warn if closing a window with multiple tabs. I am not at the window 7 computer right now.
Will post values when I get to that computer (by the way, that computer is the  only one running the latest beta version.

	stephan
	Comment 42 • 13 years ago

Windows 7 64bit Home Premium Version is being used. I have not accessed the "settings window/tab", but have selected the option warn if closing a window with multiple tabs. I am not at the window 7 computer right now.
Will post values when I get to that computer (by the way, that computer is the  only one running the latest beta version.

	Ben
	Comment 43 • 13 years ago

This is just farcical. You're basically saying "People are crying out for the quit dialog back so I'll give them the close multiple tabs one instead"!

You're deliberately confusing users and deliberately not telling them that their session is safe and how to get it back.

The number of users not on about:home could be HUGELY greater than the initial 13% estimate of 52m - see here: https://groups.google.com/group/mozilla.dev.apps.firefox/msg/b10164f573606818?hl=en

As if that wasn't enough - and it sure as hell is - you're also planning to move to completely removing the ability to prompt on exit, even by tweaking the settings in about:config: Bug 632271

Words fail me. Twitter prompts the user before retweeting, and you refuse to do it when closing 50 tabs!

	Ben
	Comment 44 • 13 years ago

The 'You are about to close X tabs' confirmation now comes up when closing the last window in Fx4.0b12, but not if selecting File > Exit.

I understand that this is by design.  Well, the item in Options > Tabs does not say "Warn me when closing multiple tabs some of the time".

What if they were trying for File > Work Offline and mis-clicked?  What if they forgot they had something important open in a tab in another window?

Yes, it's two clicks instead of one, but this doesn't make it foolproof, and the user has ASKED to be warned when closing multiple tabs (a warning that they can easily turn off right there on the dialog if they're going for streamlined).

	Eric Appleman
	Comment 45 • 13 years ago

Using beta 12 on Linux and I have been accidentally doing CTRL+Q when I meant to CTRL+Tab.

The browser closes and I lose my tabs. It's not fun.

	Pascual Strømsnæs
	Comment 46 • 13 years ago

Pressing Ctrl+Q, or in my case the Apple key and Q, instead of Ctrl+Tab is something that happens quit frequently.

I strongly urge the developers to quit arguing about details here and think about it from the perspective of the user!
There needs to be a warning to prevent unintended termination of the application. I don't see how hard it is to understand that.

I can also confirm that on Firefox 4 on Mac OS X pressing Apple key and Q quits Firefox without any warning when having several tabs open.

	DBA
	Comment 48 • 13 years ago

I wish someone would explain to me why this interface decision was made. I have lost a lot of time and state to this silly behavior. If I want the application to warn me before quitting with multiple tabs, I don't see why I should have to go fiddlefaffing around in the about:config entries or change my home page to make that happen. Really, what is the point of having a preferences checkbox that says "warn before closing multiple tabs" if you aren't going to respect the user's selection?

	Chris Bainbridge
	Comment 49 • 9 years ago

I just opened bug #1138054 before finding this one.

Can someone confirm that Ctrl-Q is supposed to actually close all tabs without any warning?
Is that the correct behaviour?

It seems odd that there is a confirmation for the user in case they accidentally press Alt-F4 to close the window, but if they accidentally press Ctrl-Q (which is far more common, due to Ctrl-W being used to close tabs etc.) then there is no confirmation.

Perhaps it would be easiest to just get rid of the Ctrl-Q default - Chrome uses Ctrl-Shift-Q which is more sensible.