408398 - Command-W Should only close current Tab; closes entire viewer window

Status: VERIFIED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Matt Dudziak]

The problem is that if you have multiple tabs open in a viewer window, pressing command-W (or selecting File->Close) causes the entire mail viewer window to close. As is the case in Firefox, only the active tab should be closed.

To reproduce:
- Right click a message and select 'Open message in New Tab'. Do this for a couple of messages
- Press Command-W, expecting the frontmost tab to close
- Note that the entire message viewer (along with ALL the tabs) closes.

Comment 1

[David Ascher (:davida)]

In general, I think closing the three-pane window should be harder than it is.

Comment 2

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

bug 218999 (thundertab) is going to require this change.

Comment 3

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

going to make this blocking so it's easier to track

Comment 4

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

Attachment: patch to close the current tabs before closing the window

I'm just working in the current code (much of which is really ugly) to make this happen.  However this patch gets the job done and I wanted to make sure to share with everyone.

ctrl-w will close the current tab until there is only one tab left and then it will close the entire window.  This matches the behavior of Firefox on Windows and Linux, not sure about the Mac.

I think making it harder to close down Thunderbird is the wrong approach to the problem.  The reason (as I see it) that we want to prevent Thunderbird from too easily being shutdown is that Thunderbird is so slow to start.  It would be much more beneficial in lots of directions if Thunderbird was actually quick to start and responsive immediately.  

Of course that's a long term battle that we need to win and perhaps the short term fight is preventing people from shutting down by accident.

Comment 5

[Tony Mechelynck [:tonymec]]

Could this bug and bug 433322 be related?

Comment 6

[Przemyslaw Bialik]

I can confirm that the patch works OK.
Any chances for TB ver of Bug 455852 ? (can be default to false ;)

Comment 7

[David :Bienvenu]

reassigning to clarkbw since he's been working on this.

Comment 8

[David :Bienvenu]

Comment on attachment 345642 [details] [diff] [review]
patch to close the current tabs before closing the window

this looks OK to me, except that the commented out line should just be removed. Asking for review from Phil

Comment 9

[Phil Ringnalda (:philor)]

Comment on attachment 345642 [details] [diff] [review]
patch to close the current tabs before closing the window

Huh, I thought I commented on this on Halloween, a goblin must have eaten my tab. It would have been along the lines of "please fix a few nits and request review from me, so we can land this, just...

>-        CloseMailWindow();
>+/*        CloseMailWindow(); */
>+          CloseMailTabOrWindow();

remove, don't comment out, and don't over-indent the replacement text

>+  var tabmail = document.getElementById('tabmail')

missing semicolon

>+  if ( tabmail.tabInfo.length == 1 ) 

no spaces inside parens, and no trailing space

and it should be ready to go."

Comment 10

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

Attachment: 408398-v2.patch

Ok, here's the updated patch.

The original CloseMailWindow() function had been removed since the first patch so this is slightly different.  However I put the CloseMailTabOrWindow() in a similar location as before.  And I feel so much more hip now that I used let instead of var, woot!

Let me know if there's anything else needed.

Comment 11

[Phil Ringnalda (:philor)]

Comment on attachment 347931 [details] [diff] [review]
408398-v2.patch

Gah, I stepped right into that one, didn't I? Let me sidestep whether I'll then see another patch from jminta to re-inline it, by just making him review now :)

Comment 12

[Joey Minta]

Comment on attachment 347931 [details] [diff] [review]
408398-v2.patch

I'm pretty strongly against functions that are only called once. It'd make sense to me to have this logic just inside the switch statement (and extensions can just as easily write goDoCommand("cmd_close") as they can CloseMailTabOrWindow()). Are there any good reasons to keep this as a separate function?

Comment 13

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

> Are there any good reasons to keep this as a separate function?

None in particular.  Your plan sounds like a good one to me.  I can move the funtionality out of the function and into the switch statement unless someone else who is going to check it in wants to make that change before committing.

Comment 14

[Joey Minta]

Comment on attachment 347931 [details] [diff] [review]
408398-v2.patch

r=jminta with the logic moved to the switch.

Comment 15

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

Attachment: 408398-v3.patch

Here's an updated version.

Moved the logic of the function into the switch statement.

Carrying over the r=jminta

Comment 16

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

i'm not owning this anymore

adding checkin-needed keyword

Comment 17

[David :Bienvenu]

fix checked in.

Comment 18

[Przemyslaw Bialik]

-> VERIFIED

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081116 Lightning/1.0pre Shredder/3.0b1pre + Mac