Add quit confirmation dialog

RESOLVED FIXED in Firefox 3 alpha6

Status

()

Firefox
General
--
enhancement
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: mwu, Assigned: mwu)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 3 alpha6
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 267749 [details] [diff] [review]
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)
Attachment #267749 - Flags: review?(mano)
(Assignee)

Comment 2

10 years ago
Created attachment 267750 [details]
Screenshot of the quit confirmation dialog
Attachment #267750 - Flags: ui-review?(beltzner)
(Assignee)

Comment 3

10 years ago
Created attachment 267751 [details]
Screenshot of the quit confirmation dialog (with multiple windows)
Attachment #267751 - Flags: ui-review?(beltzner)

Comment 4

10 years ago
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.
(Assignee)

Comment 5

10 years ago
(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!
(Assignee)

Comment 6

10 years ago
Created attachment 267914 [details] [diff] [review]
Add quit confirmation dialog, v2

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 7

10 years ago
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?
(Assignee)

Comment 8

10 years ago
(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.
(Assignee)

Comment 9

10 years ago
Created attachment 267933 [details] [diff] [review]
Add quit confirmation dialog, v3

Add a bugfix I missed last time.
Attachment #267914 - Attachment is obsolete: true
Attachment #267933 - Flags: review?(mano)
Attachment #267914 - Flags: review?(mano)

Comment 10

10 years ago
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). ;-)
(Assignee)

Comment 11

10 years ago
(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

10 years ago
(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.
(Assignee)

Comment 13

10 years ago
(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.
(Assignee)

Comment 14

10 years ago
Created attachment 267980 [details] [diff] [review]
Add quit confirmation dialog, v4

Add additional preference check.
Attachment #267933 - Attachment is obsolete: true
Attachment #267980 - Flags: review?(mano)
Attachment #267933 - Flags: review?(mano)
(Assignee)

Comment 15

10 years ago
Created attachment 268020 [details] [diff] [review]
Add quit confirmation dialog, v5

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+
(Assignee)

Comment 17

10 years ago
Created attachment 268177 [details] [diff] [review]
Add quit confirmation dialog, v6

All review comments should be addressed in this update. Now waiting for ui review..
Attachment #268020 - Attachment is obsolete: true

Comment 18

10 years ago
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+

Comment 22

10 years ago
(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?
(Assignee)

Comment 23

10 years ago
Created attachment 268506 [details] [diff] [review]
Add quit confirmation dialog, v7

Updated with beltzner's strings.
Attachment #268177 - Attachment is obsolete: true
(Assignee)

Comment 24

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 25

10 years ago
Need a Litmus testcase for this.
Flags: in-testsuite?

Updated

10 years ago
Duplicate of this bug: 351774

Updated

10 years ago
Duplicate of this bug: 378922
(Assignee)

Updated

10 years ago
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. 



Comment 29

10 years ago
Now with browser.startup.page = 3, browser.tabs.warnOnClose doesn't do anything?
(Assignee)

Updated

10 years ago
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 ...

Comment 31

10 years ago
(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.

Updated

10 years ago
Depends on: 385244
Blocks: 385425

Updated

10 years ago
Depends on: 385581
(Assignee)

Updated

10 years ago
Blocks: 385830
(Assignee)

Updated

10 years ago
Blocks: 385582

Updated

10 years ago
Blocks: 386067

Comment 32

10 years ago
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
(Assignee)

Updated

10 years ago
Blocks: 386786
(Assignee)

Updated

10 years ago
Blocks: 386628
(Assignee)

Updated

10 years ago
Blocks: 385794

Updated

10 years ago
Flags: in-testsuite? → in-litmus?

Comment 34

10 years ago
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.
Blocks: 395234

Comment 35

10 years ago
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.
Depends on: 398907
Duplicate of this bug: 402002
Depends on: 402694
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

Comment 39

9 years ago
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.

Updated

9 years ago
Blocks: 439756

Comment 40

9 years ago
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

9 years ago
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)
Depends on: 445109
Depends on: 456447

Updated

8 years ago
No longer depends on: 445109
You need to log in before you can comment on or make changes to this bug.