383760 - Add quit confirmation dialog

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Michael Wu [:mwu]]

There should be a confirmation dialog on quit if the user has more than one webpage open. This also avoids having a confirmation dialog for every window that has tabs.

Comment 1

[Michael Wu [:mwu]]

Attachment: Add quit confirmation dialog

This adds a quit confirmation dialog to firefox, which opens when the user tries to quit the firefox with 2 or more pages open. It also allows the user to save the session for next time. I will file a followup bug to add the appropriate pref checkbox to turn this on/off since I don't currently see an obvious place to place it. (but if there's a good place right now, I'll put it in)

Comment 2

[Michael Wu [:mwu]]

Attachment: Screenshot of the quit confirmation dialog

Comment 3

[Michael Wu [:mwu]]

Attachment: Screenshot of the quit confirmation dialog (with multiple windows)

Comment 4

[Simon Bünzli]

Comment on attachment 267749 [details] [diff] [review]
Add quit confirmation dialog

>+  // Get the current window position/size.
>+  var x = window.screenX;

>+  // Store these into the window attributes (for persistence).
>+  var win = document.getElementById( "main-window" );
>+  win.setAttribute( "x", x );

While you're moving that code around, please simplify it to

win.setAttribute("x", window.screenX);

>+        if (windowcount < 2 && pagecount < 2)
>+          break;

So we don't care about preserving a single tab? Especially at OS shutdown users might lose an forgotten window with a forum post or something.

BTW: Not resetting _saveSession before that point might lead to slightly inconsitent behavior if you have several tabs open, tell Firefox to resume the session when quitting, an extension afterwars cancels the quit-request. Then you close all tabs but one and try again... now the session will be silently resumed.

Final nit: checking for pagecount < 2 alone should be sufficient (with two windows you'll have at least two tabs open).

>+saveTitle=&Save session and quit

What about "Preserve session..." or "Remember session..." instead? Opposed to session saving extensions, users can't actively reopen/restore a session - it just automatically happens at the next startup and afterwards the preserved data is discarded/overwritten.

Comment 5

[Michael Wu [:mwu]]

(In reply to comment #4)
> (From update of attachment 267749 [details] [diff] [review])
> >+  // Get the current window position/size.
> >+  var x = window.screenX;
> 
> >+  // Store these into the window attributes (for persistence).
> >+  var win = document.getElementById( "main-window" );
> >+  win.setAttribute( "x", x );
> 
> While you're moving that code around, please simplify it to
> 
> win.setAttribute("x", window.screenX);
> 
Ok.

> >+        if (windowcount < 2 && pagecount < 2)
> >+          break;
> 
> So we don't care about preserving a single tab? Especially at OS shutdown users
> might lose an forgotten window with a forum post or something.
> 
I suppose, but the current window closing code only cares about having more than one tab, and it seems like always warning about closing the last window will get annoying. However, if we were to actually count the number of pages that have modified forms.. that could be useful as a follow up to this. (other bugs have been filed about adding that)

> BTW: Not resetting _saveSession before that point might lead to slightly
> inconsitent behavior if you have several tabs open, tell Firefox to resume the
> session when quitting, an extension afterwars cancels the quit-request. Then
> you close all tabs but one and try again... now the session will be silently
> resumed.
> 
True. I'll fix this.

> Final nit: checking for pagecount < 2 alone should be sufficient (with two
> windows you'll have at least two tabs open).
> 
Sure. (The patch used to count tabs and windows, which needs this logic)

> >+saveTitle=&Save session and quit
> 
> What about "Preserve session..." or "Remember session..." instead? Opposed to
> session saving extensions, users can't actively reopen/restore a session - it
> just automatically happens at the next startup and afterwards the preserved
> data is discarded/overwritten.
> 
I'll leave it to beltzner to make the right decision here. :)

Thanks!

Comment 6

[Michael Wu [:mwu]]

Attachment: Add quit confirmation dialog, v2

Updated patch based on Simon Bünzli's comments.

Comment 7

[Simon Bünzli]

Comment on attachment 267914 [details] [diff] [review]
Add quit confirmation dialog, v2

>+        if (pagecount < 2)
>+          break;

You seem to have forgotten to reset _saveSession before that line... ;-)

>+          if (prefService.getIntPref("browser.startup.page") == 3)

|| prefService.getBoolPref("browser.sessionstore.resume_session_once")

BTW: Shouldn't it rather be prefBranch instead of prefService?

Comment 8

[Michael Wu [:mwu]]

(In reply to comment #7)
> (From update of attachment 267914 [details] [diff] [review])
> >+        if (pagecount < 2)
> >+          break;
> 
> You seem to have forgotten to reset _saveSession before that line... ;-)
> 
> >+          if (prefService.getIntPref("browser.startup.page") == 3)
> 
I misunderstood the problem and fixed an unrelated one. Opps. Fixed now. Thanks.

> || prefService.getBoolPref("browser.sessionstore.resume_session_once")
> 
> BTW: Shouldn't it rather be prefBranch instead of prefService?
> 
The rest of the code in this file uses prefService. Just being consistent here.

Comment 9

[Michael Wu [:mwu]]

Attachment: Add quit confirmation dialog, v3

Add a bugfix I missed last time.

Comment 10

[Simon Bünzli]

Comment on attachment 267933 [details] [diff] [review]
Add quit confirmation dialog, v3

I so don't want to bug you, but it really doesn't make sense _not_ to check prefService.getBoolPref("browser.sessionstore.resume_from_crash") at the following line:

>+          if (prefService.getIntPref("browser.startup.page") == 3)

OTOH there was an argument about ignoring those prefs altogether and always ask (since we can't resume _all_ of the current session - just most of it; see bug 348471) - that's beltzner's call though.

>+          var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                      .getService(Components.interfaces.nsIPrefBranch);

You're really handling a prefBranch here (which doesn't even inherit from nsIPrefService). ;-)

Comment 11

[Michael Wu [:mwu]]

(In reply to comment #10)
> (From update of attachment 267933 [details] [diff] [review])
> I so don't want to bug you, but it really doesn't make sense _not_ to check
> prefService.getBoolPref("browser.sessionstore.resume_from_crash") at the
> following line:
> 
Sorry, I don't follow why that is necessary or what that achieves. Can you elaborate?

> >+          var prefService = Components.classes["@mozilla.org/preferences-service;1"]
> >+                                      .getService(Components.interfaces.nsIPrefBranch);
> 
> You're really handling a prefBranch here (which doesn't even inherit from
> nsIPrefService). ;-)
> 
I'll file a follow up bug to clean up the use of preferences in this file. I'd rather be consistent in this file for now.

Comment 12

[Simon Bünzli]

(In reply to comment #11)
> Sorry, I don't follow why that is necessary or what that achieves.

My bad... it's browser.sessionstore.resume_session_once and not .resume_from_crash I meant (cf. comment #7). That pref is set when the user has somehow told an extension to resume the session just the very next time - which is pretty much the same as the user telling Firefox to always do that.

Comment 13

[Michael Wu [:mwu]]

(In reply to comment #12)
> (In reply to comment #11)
> > Sorry, I don't follow why that is necessary or what that achieves.
> 
> My bad... it's browser.sessionstore.resume_session_once and not
> .resume_from_crash I meant (cf. comment #7). That pref is set when the user has
> somehow told an extension to resume the session just the very next time - which
> is pretty much the same as the user telling Firefox to always do that.
> 
Ah, ok. That makes sense. I'll add a check for that.

Comment 14

[Michael Wu [:mwu]]

Attachment: Add quit confirmation dialog, v4

Add additional preference check.

Comment 15

[Michael Wu [:mwu]]

Attachment: Add quit confirmation dialog, v5

Break out confirmation dialog opening code into another function, eliminate old createBundle call.

Comment 16

[Asaf Romano (gone)]

Comment on attachment 268020 [details] [diff] [review]
Add quit confirmation dialog, v5

It took me some time to figure out the way you're eliminating the tabs-warning in the quit case only. I would appreciate some comments in browser.js explaining the different code paths.

> 
>+  // Store current window position/size into the window attributes 
>+  // for persistence.
>+  var win = document.getElementById("main-window");

While you're at it, use document.documentElement instead.

>+      switch (buttonChoice) {
>+      case 0:
>+        if (neverAsk.value)
>+          prefService.setBoolPref("browser.warnOnQuit", false);
>+        break;
>+      case 1:
>+        aCancelQuit.QueryInterface(Components.interfaces.nsISupportsPRBool);
>+        aCancelQuit.data = true;
>+        break;
>+      case 2:
>+        if (neverAsk.value)
>+          prefService.setIntPref("browser.startup.page", 3);

Please note here the UI quirk/bug we discussed.

>@@ -1,4 +1,4 @@
>-function closeWindow(aClose)
>+function closeWindow(aClose, aPromptFunction)
> {
>   var windowCount = 0;
>    var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
>@@ -14,9 +14,13 @@ function closeWindow(aClose)
> # Closing the last window doesn't quit the application on OS X.
> #ifndef XP_MACOSX
>   // If we're down to the last window and someone tries to shut down, check to make sure we can!
>-  if (windowCount == 1 && !canQuitApplication())
>+  if (windowCount == 1 && !canQuitApplication()) {
>     return false;
>+  } else
> #endif

nit: no else after return, please avoid single line braces as well.

>+  if (windowCount != 1 && aPromptFunction && !aPromptFunction()) {

Make that |typeof(aPromptFunction) == "function"|.

r=mano otherwise.

Comment 17

[Michael Wu [:mwu]]

Attachment: Add quit confirmation dialog, v6

All review comments should be addressed in this update. Now waiting for ui review..

Comment 18

[David Balažic]

This might not be the place, but I would like to bring up a related issue :
When more than one window is open and the user closes it (by clicking the close button in the window title bar), he gets a dialog "Confirm close n tabs" but no indication and way to save the tabs. This is bad especially when the user forgets that there are other FF windows, so he closes the current one, confident that its content wil be restored on next start. But of course it isn't.
In case when session restore is active FF should either inform the user that these tabs will be not saved or offer a way to save them.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 267750 [details]
Screenshot of the quit confirmation dialog

Sorry to block so long. :(

This gets uir+ with the following changes:

title = "Quit Minefield"

string = "Do you want Minefield to save your tabs for the next time it starts?"

( Cancel )
( Save and quit )
( Quit )

This should land, but will likely need some further refinement. For example, I anticipate that people will wonder why we're only offering to save the session in cases where there are > 1 tabs open. I also wonder if we might not want to simply always save state and (as Shaver and others suggested in d-a-f last year) ask if they want to restore state on startup.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 267751 [details]
Screenshot of the quit confirmation dialog (with multiple windows)

This gets uir+ with the following changes:

title = "Quit Minefield"

string = "Do you want Minefield to save your tabs and windows for the next time it starts?"

( Cancel )
( Save and quit )
( Quit )

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 267751 [details]
Screenshot of the quit confirmation dialog (with multiple windows)

For reference, that previous discussion of this design is here:

http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/19a91346ede30028/e3e02974fb9f4d05?lnk=gst&q=close+N+tabs+in+M+windows&rnum=1#e3e02974fb9f4d05

Comment 22

[Simon Bünzli]

(In reply to comment #19)
> I also wonder if we might not want to simply always save state
> and ask if they want to restore state on startup.

Privacy issues...? Furthermore: how are you supposed to know what pages you had open when you last closed the browser?

Comment 23

[Michael Wu [:mwu]]

Attachment: Add quit confirmation dialog, v7

Updated with beltzner's strings.

Comment 24

[Michael Wu [:mwu]]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.183; previous revision: 1.182
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.798; previous revision: 1.797
done
Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.30; previous revision: 1.29
done
RCS file: /cvsroot/mozilla/browser/locales/en-US/chrome/browser/quitDialog.properties,v
done
Checking in browser/locales/en-US/chrome/browser/quitDialog.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/quitDialog.properties,v  <--  quitDialog.properties
initial revision: 1.1
done
Checking in browser/locales/jar.mn;
/cvsroot/mozilla/browser/locales/jar.mn,v  <--  jar.mn
new revision: 1.66; previous revision: 1.65
done
Checking in toolkit/content/globalOverlay.js;
/cvsroot/mozilla/toolkit/content/globalOverlay.js,v  <--  globalOverlay.js
new revision: 1.31; previous revision: 1.30
done

Comment 25

[Adam Guthrie]

Need a Litmus testcase for this.

Comment 28

[Jim Jeffery not reading bug-mail 1/2/11]

For whatever reason this patch seems to have broken TabMix Plus.
Tab settings in the extension are not working, and web-pages display very wide with no horiz scroll being applied.  
There are no errors in the JS Console 

All builds after the 1140 build on today's date are having the same issue with TMP.  The problem has also been reported to the TabMix Plus guys. 

Comment 29

[[:Aleksej]]

Now with browser.startup.page = 3, browser.tabs.warnOnClose doesn't do anything?

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #29)
> Now with browser.startup.page = 3, browser.tabs.warnOnClose doesn't do
> anything?

Yeah, I didn't catch that in my initial read-through of this bug, either. That seems strange to me for a couple of reasons:

 1. SessionRestore isn't 100% perfect, and some state (such as bugzilla comments that didn't commit properly, or whatever) can be lost.

 2. Now there's two ways to not get asked; first is to change browser.startup.page = 3, second is browser.tabs.warnOnClose = false.

I'm trying to figure out if I just got *so* used to a confirm on close that I find it awkward to live without it, or if this is something we need to fix.

Let's see how more people feel about it for now ...

Comment 31

[[:Aleksej]]

(In reply to comment #30)
> I'm trying to figure out if I just got *so* used to a confirm on close that I
> find it awkward to live without it, or if this is something we need to fix.

Firefox also takes some time to load, especially Minefield now -- I think it takes as long to load Minefield without addons as 2.0.0.4 with 45 of them here.

Comment 32

[:deanis74]

I was prompted to save my tabs when I installed a Minefield update and restarted.  Seems to me this should be automatic.

Comment 33

[(not reading, please use [e-mail] instead)]

> I was prompted to save my tabs when I installed a Minefield update and
> restarted.  Seems to me this should be automatic.

dean, see bug #385853

Comment 34

[BugMagnet]

see:
https://bugzilla.mozilla.org/show_bug.cgi?id=391229

An enhanced warning may help, but enhanced saving and reloading would be better.

as is:
"You are about to close 32 tabs.  Are you sure you want to continue?"

without enhanced session saving, suggest something like:

"You are about to close 32 tabs.  There are other existing windows, popups and or processes.  Closing this window now will cause loss of data for these 37 tabs and they WILL NOT RESTORE on restart. Are you sure you want to continue?"

enhancing what is saved on [x] may negate the need for such a warning.

Comment 35

[Valdis Kletnieks]

Tuning in a bit late here... I personally wish that warnOnQuit functioned the way it did in The Elder Times, when it would prompt even if you only had one frame open in one window.

I've gotten burnt too many times where I have 3 tabs open, want to nuke the last 2, and I hit control-W one time too many and the whole thing evaporates.

Alternately, any way to prevent control-W from killing the window if it's the only tab?

(The *really* fun variant - when the Download Manager window is still open-but-iconfied someplace on the screen, and you get into a nice "it's already running" **** contest when you try to re-launch it... Argh. ;)

Comment 36

[Marcia Knous [:marcia]]

Litmus Triage: marcia will handle Litmus test case.

Comment 38

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=5116 is one test case that covers this functionality.

Comment 39

[Jacob]

oh, 
https://bugzilla.mozilla.org/show_bug.cgi?id=439756 
is duplicate of this bug here? In my reported bug, i see not warning anymore.

Comment 40

[Doug Kramer]

When I press Command-q (Mac), Firefox always quits and loses data, even though in about:config, browser.warnOnQuit is true.  It should display a warning dialog box.  Is there a bug request against this?

Firefox 3.0

Comment 41

[Ed Lee :Mardak]

If you go to your preferences (cmd-,) -> main tab -> startup -> when firefox starts...

Switch it to "show my home page" (from "show my windows and tabs from last time")

It'll make it show the quit confirmation dialog (even though warnOnQuit is true and it doesn't warn right now)