428843 - Unchecking "warn on close multiple tabs" also disables the Quit dialog (no longer offers to save tabs when quitting)

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Recall]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008041015 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008041015 Minefield/3.0pre

Unchecking "warn on close multiple tabs" breaks the option for "save tabs on exit" It used to work a few builds ago but not sure of regression range.

If you keep the option checked, then you still get the prompt to save tabs on exit. If it is unchecked then firefox does not give the option.

Reproducible: Always

Steps to Reproduce:

1.Uncheck "warn on close multiple tabs
2.Open a few tabs
3.Exit firefox
Actual Results:  
"Do you want to save tabs" is not given as an option.

Expected Results:  
Unchecking the option should still give the option to save tabs on exit.

This is a regression.

Comment 1

[Recall]

OK seems bug 419009 has caused this, is this intentional behavior then?

Comment 2

[Roman R.]

(In reply to comment #0)
> Unchecking the option should still give the option to save tabs on exit.

Should it? I think FF should save a session if the "show my windows and tabs from last time" is checked, and it should do it without asking.

Comment 3

[Recall]

I dont like getting bugged that I am closing multiple tabs. I do however like to be able to save my tabs on exit, but not always. See bug 416696 that was fixed in February for and idea of what i mean.

Comment 4

[Roman R.]

It seems like somebody should reevaluate this whole thing, get requirements from users and redesign this area, including the pref UI, of course, to avoid confusion.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

This was an intentional change from bug 419009. See bug 428742 comment 6 for the prefs you need to change to get the old behavior.

Comment 6

[Recall]

Gavin, thanks but this is really such a confusing setup. Can we not get a preference for "prompt to save tabs on restart" checkbox under tools>options>tabs this way you can choose to always have your tabs saved, or not as in my case. Should be a quick fix?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

We're well past string freeze, we can't be adding checkboxes to the pref dialog for Firefox 3.

Comment 8

[Roman R.]

3.0.1 then?

Comment 9

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

Roman: yes, we need to revisit the entire set of prefs around quit and restart, we meant to do it for Firefox 3 but ran out of time.

However, I do agree that the way this has been fixed has ended up putting us in a pretty bad state; we fixed bug 419009 in order to resolve an annoyance (not asking before closing multiple tabs when there was only one window and browser.startup.page = 3) in which there was no dataloss, only timeloss, and the result is we've created an entirely possible case in which there can now be dataloss.

I'm gonna think about this for a bit. I still do think that there are some advantages to the fix from bug 419009, namely that the one exposed user pref implies that users will or will not be warned before closing multiple tabs. It also allows people to reset the warnOnQuit pref, but that comes at the cost of not being warned when they close a window with multiple tabs.

Comment 10

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

Spoke with mconnor about this, and we agreed that this doesn't block. The relevant advantage that swayed the decision here was that at least now there's UI for getting the "Save and Quit" dialog back (it's tied to "Warn me when closing multiple tabs") whereas before there wasn't.

We definitely want to clean this up in the future, though.

Comment 11

[Recall]

Mike, I really do not think this is the way to go. The UI is now more unusable than before and now get an extra annoyance for closing tabs. This has caused bug 428742 as well ( which has been marked wontfix! ) This seems to me a crazy decision is there no chance of backing out bug 419009?

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> Spoke with mconnor about this, and we agreed that this doesn't block. The
> relevant advantage that swayed the decision here was that at least now there's
> UI for getting the "Save and Quit" dialog back (it's tied to "Warn me when
> closing multiple tabs") whereas before there wasn't.

You think that that advantage outweighs the impact of this bug and the lack of a good workaround for bug 428742? If so, I disagree. I think that being unable to re-enable the Quit dialog is a minor issue that should be addressed by reworking preferences UI in the next release, whereas the inability to configure warnings such that you're warned if dataloss will occur (closing a window with multiple tabs), but aren't warned in the cases where your session will be restored (restarting, quitting with the "save my tabs" preference enabled) without toggling a hidden pref will be a much more annoying problem for most users. The Quit dialog is a useful feature, and tying it to the "close a window with multiple tabs" pref cripples it unnecessarily.

I'm not sure why you would mark this "wanted-firefox3", since there is no solution other than taking a different approach in bug 419009, and you've indicated that you don't think we're going to do that.

Comment 13

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

In bug 428742 claimed that setting browser.startup.page=3 should be tied directly to showing or not showing the warning when quitting, and I stand by that; we don't always successfully restore state, so a warning should be shown to avoid potential dataloss, especially in the restart case.

I've reopened bug 428742, as I think it was marked incorrectly twice, alas.

The one thing I didn't consider in my blocking decision was that the way things are now, checking "don't show me this warning again" when quitting ends up also turning off the multiple tab warning, which isn't very intuitive.

Based on that, I think we need to back out bug 419009, ensure that browser.WarnOnQuit = (browser.startup.page==3), and then actually figure out how to solve bug 419009 properly, which is to say ensure that the multiple tab warning is given irrespective of browser.startup.page/browser.WarnOnQuit.

Comment 14

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

(In reply to comment #13)
> In bug 428742 claimed that setting browser.startup.page=3 should be tied
> directly to showing or not showing the warning when quitting, and I stand by
> that; we don't always successfully restore state, so a warning should be shown
> to avoid potential dataloss, especially in the restart case.

Ick, this was half edited. Please ignore everything except:

I don't think that setting browser.startup.page=3 should effect the restart case.

Comment 15

[Simon Bünzli]

What about the following stopgap solution:
* the visible "warn me when closing..." UI controls both browser.tabs.warnOnClose _and_ browser.warnOnQuit (in the same way as the "new pages should open in..." UI controls two prefs);
* when one of both prefs is set to false, we remove the checkbox;
* toggling the checkbox toggles both prefs,
* however as long as the checkbox is untouched, the prefs can diverge, and
* removing the checkbox from the warning prompt only toggles one of both prefs.

Like this, there'd be a somewhat obvious way to disable/enable both prefs without us having to introduce new UI at this stage (at least people could be walked through without having to touch about:config)...

Comment 16

[Recall]

Simon, I like that idea if indeed it is too late to get a pref ui in. At least then by unchecking the warn on close tabs we will still keep the option for save on quit, unless "always do this" is checked for not saving. Get so close to final now is it possible to shoehorn this in before it ships?

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #15)
> What about the following stopgap solution:

I think we should do this. Can you prepare the patch?

Comment 18

[Simon Bünzli]

Attachment: fix per comment #15

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 318187 [details] [diff] [review]
fix per comment #15

>Index: browser/components/preferences/tabs.js

>+  writeWarnCloseTabs: function() {
>+    var warnOnClose = document.getElementById("warnCloseMultiple");
>+    document.getElementById("browser.warnOnQuit").value = warnOnClose.checked;
>+    return warnOnClose.value;

warnOnClose doesn't have a ".value", it's a checkbox. This works anyways because undefined maps to "default element value", though, so you can just change this to return undefined explicitly.

Comment 20

[Mike Connor [:mconnor]]

Comment on attachment 318187 [details] [diff] [review]
fix per comment #15

I don't like this.  It overloads the checkbox in an unintuitive way.  If one of the warnings is off, and you want to turn the other off in the prefwindow, you need to check and uncheck it.  That feels pretty non-deterministic to me, and I'm not willing to ship something that will sometimes look like its broken.  We should just fix this the right way in 3.1.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Attachment: alternative patch (-w)

Talked a bit with Beltzner, this is an alternative patch that addresses my issues with the current behavior.

The main changes are to never show the quit/restart prompts if the session will be restored, and to not disable the "close window with multiple tabs" warning when disabling the quit warning with session store enabled. Disabling the quit prompt without enabling session restore all the time will still disable the "close window with multiple tabs" warning, but since the user is opting in to no-warning dataloss (quitting with multiple tabs and session store disabled), the other dataloss case (closing a window with multiple tabs) shouldn't bother them too much (and they can still re-add the quit prompt in our UI since it still uses the warnOnClose pref).

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Attachment: alternative patch that works (-w)

Comment 23

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

Comment on attachment 318541 [details] [diff] [review]
alternative patch that works (-w)

uir=beltzner, thanks!

(I'll, uh, warn dria and phik)

Comment 24

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

(Still not blocking, but I'd approve the reviewed patch)

Comment 25

[Recall]

Is this patch gonna make it in?

Comment 26

[Mike Connor [:mconnor]]

Comment on attachment 318541 [details] [diff] [review]
alternative patch that works (-w)

code looks right.  I'm not completely convinced this is any better, but its probably more conservative.

If this regresses anything, it gets backed out and we just ship.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/nsBrowserGlue.js 	1.93 

Comment 28

[Soothsayer]

Following this check in, I now get the close multiple tabs prompt every time I close Firefox even when no tabs are open.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9pre) Gecko/2008050812 Minefield/3.0pre

Comment 29

[Recall]

I don't get the prompt here with no tabs open. In fact nothing has changed at all for me, the only way for me to be prompted on close to save session is to keep the annoying multiple tabs warning on. I don't even think an extension can fix this, unless it uses its own restore session.

Can we just get this reopened until a proper fix lands with a gui interface?

Comment 30

[Soothsayer]

If it helps reproduce / identify the cause, my pertinent preferences are:
browser.warnOnRestart = false
browser.tabs.warnOnClose = true

Also, overwriting my nsBrowserGlue.js with the one from yesterday's build fixes the problem.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #28)
> Following this check in, I now get the close multiple tabs prompt every time I
> close Firefox even when no tabs are open.

That's an expected change from this patch, to cover cases where you close the window accidentally and lose your session. Please file a new bug if you think we should revert it.

Comment 32

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #29)
> I don't get the prompt here with no tabs open. In fact nothing has changed at
> all for me, the only way for me to be prompted on close to save session is to
> keep the annoying multiple tabs warning on.

You're right, this patch only affected the cases where you have "Always restore my tabs" enabled. To get the behavior you describe we'd need to decouple the prefs again, which we can't do for Firefox 3. Please file a new bug to cover that case.

Comment 33

[Soothsayer]

As suggested, I have created bug 433018 requesting this patch be backed out due to undesired behaviour and inaccurate pop up message.

Comment 34

[George Carstoiu]

Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b9pre) Gecko/20110109 Firefox/4.0b9pre

Issue is still present and can cause data loss (session is lost) if "Warn me when closing multiple tabs" is unchecked.