527434 - Trying to exit Thunderbird with multiple open tabs needs a confirmation

Status: RESOLVED WONTFIX

(Thunderbird :: Toolbars and Tabs, enhancement)

Description

[kurt rosenfeld]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)
Build Identifier: Shredder

The new layout opens email windows but still has an X top right which used to close the current email window in previous versions.
I have repeatedly clicked this to close the current email but have closed shredder instead.

Reproducible: Always

Steps to Reproduce:
1.Open an eamil (or delete an email which opens the next one)
2.Click the top right x to close the email
3.this closes shredder instead
Actual Results:  
Shredder closes

Expected Results:  
A Query asking if you really want tgo close shredder

It would be OK if the current email closes.
Shredder can be closed late with an Icon 'Quit  Shredder'

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

If there is more than 1 tab open we should prompt if the person meant to close the whole window.  We could likely offer some tab management in the dialog as well. i.e. "Close all tabs"

This would be good to try for the 3.1 release.

Comment 3

[Mark Banner (:standard8)]

Trying to improve title for searching...

Comment 5

[Peter B]

(In reply to comment #2)
> If there is more than 1 tab open we should prompt if the person meant to close
> the whole window.  We could likely offer some tab management in the dialog as
> well. i.e. "Close all tabs"
> This would be good to try for the 3.1 release.

Yes I agree. Too many times I have clicked on the X and closed Thunderbird when I only intended to close the current tab.

FireFox and MS Internet Explorer have two different approaches but neither of them lets you close the browser without a warning.

TB automatically saves the open tabs and re-opens them the next time you activate TB so perhaps the MS Internet Explorer approach which asks if you want to close only the current tab or the complete program is best. 

I have no particular preference for one or the other approach.

Comment 6

[David :Bienvenu]

I'm not convinced everyone is talking about the same issue. Sounds to me like the reporter wants to make closing the stand-alone message window ask if we want to shut down the app, if the window was the only window left open, i.e., confirm shutdown of the app by closing the last window. I'd minus that, since I'm unclear on how this is different from 2.0. We could hijack this bug for accidental closing of the window instead of a tab, or more accurately, continue the hijacking. I'd still minus that, but Bryan can override me.

Comment 7

[Leo Geoffrion]

I can't speak for others, but I find very annoying the fact that when I click the Windows X icon -- what I've always done to close an individual message -- it exits the entire application without any confirmation.   I'm then forced to reconnect to my various mailboxes and re-enter send passwords.   

Firefox is much better in this area.  It asks for confirmation -- at least when more than one tab exists.   I can then cancel and do the correct command.

At the very least, please be consistent across the Mozilla family of applications.

Comment 8

[David :Bienvenu]

I don't think the stand-alone message window has tabs, does it?

Comment 9

[Peter B]

(In reply to comment #7)
> I can't speak for others, but I find very annoying the fact that when I click
> the Windows X icon -- what I've always done to close an individual message --
> it exits the entire application without any confirmation.   

I agree 100%. I hate it when an application of any type closes without warning. It is particularly easy to do when you have opened a message in a tab and you have TB in a full screen display.

> Firefox is much better in this area.  It asks for confirmation -- at least when
> more than one tab exists.   I can then cancel and do the correct command.
> At the very least, please be consistent across the Mozilla family of
> applications.

So does MS Internet explorer but in a different way.

Surely it is very easy to add a confirmation dialog on exit.

It is true that if Open Message in a new window is set in options this problem can be avoided but that looses the versatility of having tabs. 

An interesting side note: it seems there is no way to edit a new message or draft in a tab. It always opens a new window.

Comment 10

[Bryan Clark (DevTools PM) [:clarkbw]]

I think Boriss' blog had some interesting points related to this, http://jboriss.wordpress.com/2010/02/25/towards-a-cleaner-firefox-startup/

Comment 11

[Peter B]

Interesting yes, but more related to startup than exiting - which really was the whole point of this thread.

A simple dialog with "Exit" or "Close Current Tab" buttons is all that is really required. If the dialog is closed no action takes place. That is to say the command is canceled.

Comment 12

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: current firefox multiple tabs closing warning

Doing a bit of research into what other systems do.  Here's what Firefox shows when you're about to close a window with multiple tabs in it.

Comment 13

[Peter B]

Attachment: Firefox Italian Close Dialog

This is the Close Dialog of the Italian version of Firefox on Windows
Mozilla/5.0 (Windows; U; Windows NT 5.1; it; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.30729)

Translation:
Save the open tabs for the next session?
(Checkbox) Do not advise again in the future
(Buttons) Save and Exit - Close - Cancel

The use of two words (Esci (Exit) and Chiudi(Close) to close is a bit annomolous as one could easily think Chiudi simply meant to close the dialog. It would have been better if Esci(Exit) was used on both buttons. That is however an issue for the Italian Firefox developement team.

If Thunderbird is to use a similar dialog I would therefore prefer to see, in the English version;
(Buttons) Save and Exit - Exit - Cancel

Comment 14

[Peter B]

Hi Folks

Is somebody going to address this issue?

It should not be too difficult as the current tabs are saved when TB is closed. Surely it is just a matter of creating a dialog prior to the code that saves the current tabs. 

I know this is an issue that does not get much attention but it does need fixing.

Comment 17

[Peter B]

Hello folks

Thsi bug does not seem to be one that would be impossible to address, after all Firefox handles the problem quite well. Is it possible to upgrade the status to make it a must have for tb 3.2? In this way it may get the attention of someone with the necessary skills to fix it.

Comment 18

[Mark Banner (:standard8)]

(In reply to comment #17)
> Is it possible to upgrade the status to make it a must have for tb 3.2?

It is already marked in multiple ways as a priority. The only flag I am clearing is one for where we wouldn't be accepted anyway - an existing release.

Comment 19

[rossi]

so, do we want to clone the firefox behavior?
i am willing to take this bug, just tell me if that's what we want.
firefox has a tabs category in prefs where you can change this setting, do we need that too or just have it as a hidden pref (also thunderbird doesn't have a tabs category right now, so that's another decision where to put that config...)?

Comment 20

[Peter B]

(In reply to comment #19)
> so, do we want to clone the firefox behavior?
> i am willing to take this bug, just tell me if that's what we want.
> firefox has a tabs category in prefs where you can change this setting, do we
> need that too or just have it as a hidden pref (also thunderbird doesn't have a
> tabs category right now, so that's another decision where to put that
> config...)?

In bocca al lupo Rossi. Nice to see someone prepared to take this on.
Actually there is one tab setting in Options. I have the Italian version installed not the English so I am not certain of the actual words used in the English version. In options under Advanced Settings, on the second tab, Reading and Screen, the is an option for "Open messages in:" a new tab, a new window, the same window.(Some one with the English version of TB installed may like to correct this)
TB already saves the current open tabs if it is closed with any tabs open. In my opinion it is only necessary to have a pop-up which asks if you want
to close only the current open tab or the complete program (MS Internet Explorer approach).  Buttons could be: Save and Exit - Close current tab - Cancel. "Save and Exit" would be current behaviour, "Close current tab" would close only the tab currently open (the same behaviour as clicking X on the tab), Cancel would simply cancel the pop-up and return to the open window.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #19)
> so, do we want to clone the firefox behavior?
> i am willing to take this bug, just tell me if that's what we want.

As a first step, cloning the firefox behaviour seems like a good move.  We can see how that works out, and consider changing the behaviour after that.

Thank you for taking this bug, Rossi.  I look forward to reviewing your patch!  :)

Comment 22

[rossi]

Attachment: Adds prompt if user wants to close multiple tabs, option in preferences and default pref.

a few things:
1) As I couldn't think of any better place, i just added a group "Miscellaneous" to the general tab in preferences to change the warning pref, just tell me when i should put it somewhere else. (also if you think that the pref should have a different name or the event listener should be added in messenger.xul onclose rather than via addEventListener.)
2) I copy & pasted most of the actual warning function from firefox code, so i don't know if i need to put in some credit?
3) This needs l10n, don't know how to deal with that (can i put something into bugzilla?)
4) In Firefox only "visible" (not ones in currently hidden tab groups) tabs and tabs that aren't app tabs are counted. This function has to be modified in the same way, if thunderbird will ever have something like app tabs or invisible tabs...
5) I'm new to mercurial, so i hope that the patch format is ok and apologize if it's not =)

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 526217 [details] [diff] [review]
Adds prompt if user wants to close multiple tabs, option in preferences and default pref.

This should go for UX review first, and I think Blake's probably good for that anyway.

Comment 24

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 526217 [details] [diff] [review]
Adds prompt if user wants to close multiple tabs, option in preferences and default pref.

Review of attachment 526217 [details] [diff] [review]:

(In reply to comment #22)
> Created attachment 526217 [details] [diff] [review]
> Adds prompt if user wants to close multiple tabs, option in preferences and
> default pref.
> a few things:
> 1) As I couldn't think of any better place, i just added a group
> "Miscellaneous" to the general tab in preferences to change the warning pref,
> just tell me when i should put it somewhere else.

That section seems really big.  What about putting it under a new "Tabs" section (peer to Formatting and Tags) in the Display screen?

> (also if you think that the
> pref should have a different name or the event listener should be added in
> messenger.xul onclose rather than via addEventListener.)

Well, SeaMonkey does it like this:
http://mxr.mozilla.org/comm-central/source/suite/mailnews/msgMail3PaneWindow.js#720
so not doing it in the xul seems okay to me.  On the other hand, we're not doing what they do…
And we don't quite seem to be doing it the way Firefox does.  So perhaps we should be more like SeaMonkey in this case.  What do you think?

> 2) I copy & pasted most of the actual warning function from firefox code, so i
> don't know if i need to put in some credit?

Hmm, I don't know either.  I suspect the people credited will still be credited.

> 3) This needs l10n, don't know how to deal with that (can i put something into
> bugzilla?)

As I understand it, the mere fact that you added strings will cause them to show up in the l10n dashboard, and so the localizers will (hopefully) pick it up.

> 4) In Firefox only "visible" (not ones in currently hidden tab groups) tabs and
> tabs that aren't app tabs are counted. This function has to be modified in the
> same way, if thunderbird will ever have something like app tabs or invisible
> tabs...

Heh.  That would be foreshadowing.  :)

> 5) I'm new to mercurial, so i hope that the patch format is ok and apologize if
> it's not =)

Seems fine to me.  What did you do to create it?

Other notes:

There is a slightly weird interaction on the Mac, where closing the window won't quit the app, but once I remembered that it seemed to work.

But, you don't ask for confirmation if I use the Quit menu option, which I would have expected you to.  But then, neither does Firefox…  So I'm conflicted.

Having said all that, I guess I'm pretty happy with it.  If you fix the first thing I mentioned, you've got a ui-r=me.  :)

Thanks,
Blake.

Comment 25

[Mark Banner (:standard8)]

(In reply to comment #24)
> There is a slightly weird interaction on the Mac, where closing the window
> won't quit the app, but once I remembered that it seemed to work.
> 
> But, you don't ask for confirmation if I use the Quit menu option, which I
> would have expected you to.  But then, neither does Firefox…  So I'm
> conflicted.

I suspect Firefox doesn't because if you're quitting the app, we assume that's what you meant to do. If you did quit by accident, then you can restart and do a session restore.

Comment 26

[rossi]

Attachment: New patch as discussed with bwinton on irc, adds a tabs category to preferences

i did not test this on mac and linux... (but i'm confident it's good there too. =) )

Comment 27

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 532559 [details] [diff] [review]
New patch as discussed with bwinton on irc, adds a tabs category to preferences

Review of attachment 532559 [details] [diff] [review]:
-----------------------------------------------------------------

Thunderbird seems to hang at shutdown, so I didn't check that you set the pref correctly if I un-check the checkbox then.

But, assuming that works correctly, I like this.  ui-r=me.

Comment 28

[rossi]

Attachment: New patch as discussed with bwinton on irc, adds a tabs category to preferences

just a little change (not-used javascript was included and the name of the dtd in the xul was displayDTD which i changed to tabsDTD (copy&paste leftover))
bwinton says ui-review still stands.

Comment 29

[David :Bienvenu]

Comment on attachment 533437 [details] [diff] [review]
New patch as discussed with bwinton on irc, adds a tabs category to preferences

I think from IRC, you meant for bwinton to review this...

Comment 30

[David :Bienvenu]

Comment on attachment 533437 [details] [diff] [review]
New patch as discussed with bwinton on irc, adds a tabs category to preferences

ah, right, asuth

Comment 31

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 533437 [details] [diff] [review]
New patch as discussed with bwinton on irc, adds a tabs category to preferences

This looks very good!  The only thing that immediately jumps out is you did not modify tabs.xul's comment block enough; namely, Scott MacGregor does not need to be name-checked nor the year 2005 ;)

In order to move on to proper review, a mozmill test is required to verify the functionality works and to prevent regressions.  Please see:
https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing

Ideally, the test would:
1) Open another window so if the test fails Thunderbird does not automatically quit because there are no windows left open.  This could be another 3-pane window or the any other top-level window.  We seem to use the addressbook sometimes; it may be simpler because there is no need to break symmetry with a different window type.

2) Make sure the window you are going to try and close has at least two tabs.

3) Try and close it and use our modal dialog support to say "no, don't close", and verify that it did not close.

4) Try and close it and use our modal dialog support to say "yes, do close" and verify that it did close.

5) Restore window state so that a single 3-pane window is open with a single tab.  If you are using test-folder-display-helpers.js, I believe it will automatically inject such logic into an automatically generated cleanup helper for your file...

Comment 32

[Ludovic Hirlimann [:Usul]]

Rossi are you still working on this ?

Comment 33

[rossi]

i'd love to, but time isn't on my side. this became a little bigger than i thought with the mozmill test... anyway if anyone wants to take over, feel free to do so. if noone does, i'm still willing to work on it, but i had a rough time and still have two or three weeks where i will not have time to do it. i don't know if the patch still works on current code but if it does, the only thing missing is the mozmill test and the wrong comment block mentioned in comment 31.

Comment 34

[Thomas D. (:thomas8)]

I'm entirely not sure why this qualifies for [UXprio]. 
Please reconsider and remove [UXprio] here and add it to something more important and more helpful. Hundreds of things would be more pressing and helpful than this.
I also have doubts if bug 541121 and bug 587324 are duplicates of this.
Maybe it has been morphed and hijacked too often.

Comment 35

[Leho Kraav (:macmaN @lkraav)]

What state is this currently in? Ever since tabs came long, I've been using them in a similar pattern to Firefox i.e. actually really using them. Which makes accidental window closes with 10+ special-ordered tabs hurt quite a bit more.

Comment 36

[Wayne Mery (:wsmwk)]

per bug 1549215