Closed Bug 383760 Opened 13 years ago Closed 13 years ago

Add quit confirmation dialog

Categories

(Firefox :: General, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 3 alpha6

People

(Reporter: mwu, Assigned: mwu)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 6 obsolete files)

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.
Attached patch Add quit confirmation dialog (obsolete) — Splinter Review
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)
Attachment #267749 - Flags: review?(mano)
Attachment #267750 - Flags: ui-review?(beltzner)
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.
(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!
Attached patch Add quit confirmation dialog, v2 (obsolete) — Splinter Review
Updated patch based on Simon Bünzli's comments.
Attachment #267749 - Attachment is obsolete: true
Attachment #267914 - Flags: review?(mano)
Attachment #267749 - Flags: review?(mano)
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?
(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.
Attached patch Add quit confirmation dialog, v3 (obsolete) — Splinter Review
Add a bugfix I missed last time.
Attachment #267914 - Attachment is obsolete: true
Attachment #267933 - Flags: review?(mano)
Attachment #267914 - Flags: review?(mano)
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). ;-)
(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.
(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.
(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.
Attached patch Add quit confirmation dialog, v4 (obsolete) — Splinter Review
Add additional preference check.
Attachment #267933 - Attachment is obsolete: true
Attachment #267980 - Flags: review?(mano)
Attachment #267933 - Flags: review?(mano)
Attached patch Add quit confirmation dialog, v5 (obsolete) — Splinter Review
Break out confirmation dialog opening code into another function, eliminate old createBundle call.
Attachment #267980 - Attachment is obsolete: true
Attachment #268020 - Flags: review?(mano)
Attachment #267980 - Flags: review?(mano)
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.
Attachment #268020 - Flags: review?(mano) → review+
Attached patch Add quit confirmation dialog, v6 (obsolete) — Splinter Review
All review comments should be addressed in this update. Now waiting for ui review..
Attachment #268020 - Attachment is obsolete: true
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 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.
Attachment #267750 - Flags: ui-review?(beltzner) → ui-review+
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 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
Attachment #267751 - Flags: ui-review?(beltzner) → ui-review+
(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?
Updated with beltzner's strings.
Attachment #268177 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Need a Litmus testcase for this.
Flags: in-testsuite?
Duplicate of this bug: 351774
Duplicate of this bug: 378922
Blocks: 384658
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. 



Now with browser.startup.page = 3, browser.tabs.warnOnClose doesn't do anything?
Blocks: 384907
(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 ...
(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.
Depends on: 385244
Depends on: 385581
Blocks: 385830
Blocks: 385582
Blocks: 386067
I was prompted to save my tabs when I installed a Minefield update and restarted.  Seems to me this should be automatic.
> 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
Blocks: 385853
Blocks: 386786
Blocks: 386628
Blocks: 385794
Flags: in-testsuite? → in-litmus?
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.
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. ;)
Litmus Triage: marcia will handle Litmus test case.
Duplicate of this bug: 402002
https://litmus.mozilla.org/show_test.cgi?id=5116 is one test case that covers this functionality.
Flags: in-litmus? → in-litmus+
Depends on: 427652
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.
Blocks: 439756
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
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)
No longer depends on: 445109
You need to log in before you can comment on or make changes to this bug.