395388 - 3-pane-window-tab shouldn't appear to be closible, since it's not

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect, P3)

Description

[Alex Ihrig [:Thunderbird_Mail_DE]]

In Thunderbird trunk builds (2007-09-06) it's possible to close the 3-pane-window-tab. Closing the 3-pane-window-tab leads to be unable to open any other message / folder / tab. So users are forced to quit and restart the application.

IMHO it must not be possible to close the 3-pane-window-tab, or we would need a solution (menu item) to reopen the 3-pane-window / 3-pane-window-tab.

Comment 1

[Magnus Melin [:mkmelin]]

Can you explain that a bit more? Don't see what you mean... 

Comment 2

[Phil Ringnalda (:philor)]

STR:
1. Right click a message in the thread pane, open in tab
2. Close the 3pane tab
3. Try to figure out how to get it back

OS X would *almost* be set, since it has a menu item to find-or-open a 3pane, except that it thinks the shell of a 3pane containing only a single message is a usable 3pane.

Comment 3

[Magnus Melin [:mkmelin]]

Aah, I see. Thx Phil!

Comment 4

[Alex Ihrig [:Thunderbird_Mail_DE]]

Are there ideas how to solve this bug?

IMHO we should disable the context menu item "Close Tab" and hide the red cross in the 3-pane-window-tab.

Furthermore a context menu item "Close all other tabs" (like in Firefox) would be usefull. Oh, and there the 3-pane-window must not be closed, too.

Comment 5

[WildcatRay]

Ctrl+W (in the Windows OS version as when closing a tab in Firefox) will close Thunderbird 2.0.0.14 with no user prompt to confirm whether or not the user wants to exit completely. Is this the same as this bug and thus make this bug not just Trunk, but Thunderbird 2 as well?

Comment 6

[WildcatRay]

(In reply to comment #5)
> Ctrl+W (in the Windows OS version as when closing a tab in Firefox) will close
> Thunderbird 2.0.0.14 with no user prompt to confirm whether or not the user
> wants to exit completely. Is this the same as this bug and thus make this bug
> not just Trunk, but Thunderbird 2 as well?
> 

Please disregard my comments.

Comment 7

[David Ascher (:davida)]

tabs in general are not ready, but we should definitely fix this.

Comment 8

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

Yes, we need to remove the close button on the initial tab.  We can't have people closing the initial tab or there's no reasonable way to get it back.  Our current future tab thinking requires this protection as well.

Comment 9

[Phil Ringnalda (:philor)]

Sort of fixed now, since you can't close the first tab, but it's a little harsh. Instead of not being able to close the first tab, you ought to not be able to close a folder tab if it's the only folder tab open, so you can have | Msg | Msg | 3pane | if that's what floats your boat, and an uncloseable tab shouldn't have an active close button with a tooltip and an enabled context menu. Dunno if we want to do those things here or in a new bug or two, though.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

I think the minimal amount of work that needs to happen to get to something shippable is that we remove the close button and close context menu.  More than that can be spun off to other bugs.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

And, really, I think davida's instincts were right: we would actually ship without this; leaving blocking- and wanted+.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #11)
> I think the minimal amount of work that needs to happen to get to something
> shippable is that we remove the close button and close context menu.  More than
> that can be spun off to other bugs.

Assigning to Hendrik for a possible patch for removing the close button and the close context menu.

Comment 14

[Phil Ringnalda (:philor)]

See bug 485268 for the "do something better, once we've stopped pretending you can close it when you can't" bug.

Comment 15

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

Hendrik was just asking how to fix this, and I was looking into it, and I think it would probably make things more ugly in general to have a way to have tabs that can't be closed.  I don't think we have an actual use case where we want to allow tabs to be created that the user can't close, so this would just add additional cruft.

I propose we just move on to the "do something better" bug that philor points out, on bug 485268.  My refactoring work on bug 474701 should make whatever the UI decision is hopefully feasible.  Does that make this bug invalid or wontfix or something?

Comment 16

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

There is a really easy CSS patch for this that I seem to have lost.  We should try removing the close button from the first tab.  

tabmail.css -
tabs:first-child close-button { display: none; }

I think there are some other additional things we can do to change the display of the first tab, but lets make incremental efforts.

Comment 17

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #16)
> There is a really easy CSS patch for this that I seem to have lost.  We should
> try removing the close button from the first tab.  

Bryan, Hendrik's in his end-game (less than 2 weeks left) for his Mozilla project as part of a school module - perhaps you'd like to outline exactly what should be accomplished as part of this bugfix?

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #17)
> (In reply to comment #16)
> > There is a really easy CSS patch for this that I seem to have lost.  We should
> > try removing the close button from the first tab.  
> 
> Bryan, Hendrik's in his end-game (less than 2 weeks left) for his Mozilla
> project as part of a school module - perhaps you'd like to outline exactly what
> should be accomplished as part of this bugfix?

Bryan and I had a short discussion on this on IRC, and here's how this bug should move forward wrt. incremental changes. Other upheavals will then take place later, and in other bugs.


1 - Hide the close button on the first tab.
2 - Disable the context menu "Close tab" option for the first tab.
3 - That's all!

Comment 19

[Pete Riley]

> 1 - Hide the close button on the first tab.
> 2 - Disable the context menu "Close tab" option for the first tab.
> 3 - That's all!

As Ray said in comment 5, you might also want to disallow Ctrl-W when there's only one tab because it closes Thunderbird instead of the tab.  That seems like unexpected and unwanted behavior IMO.

Comment 20

[hendrik]

Attachment: Patch version 1

Hi all, first patch:
* Removed the context menu for the first tab
* Removed the toolbar button from the first tab
* Disabled Ctrl-W when there is only one tab

Please feel free to comment. Thanks!

Comment 21

[hendrik]

Attachment: Screenshot

Comment 22

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

Comment on attachment 371127 [details] [diff] [review]
Patch version 1

Sorry, I should have responded but I didn't see comment 19 earlier.  The close behavior has been made configurable as it is a bit contentious, obviously the reason Pete pointed it out.
http://mxr.mozilla.org/comm-central/source/mail/base/content/mail3PaneWindowCommands.js#499

Lets leave that part alone so there can still be the preference.

Other than that it looks good to me so I'll plus this assuming the change mentioned is made.

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 371127 [details] [diff] [review]
Patch version 1

Please no tabs, only use spaced everywhere. (Two space indentation.)

>         {
>-          if (pref.getBoolPref("mail.tabs.closeWindowWithLastTab"))
>-            window.close();

As Bryan noted, this should stay. (Ctrl+W and such...)

>+          <xul:menupopup anonid="tabContextMenu"
>+		onpopupshowing="
>+			var tabmail = this.parentNode.parentNode.parentNode;
>+			numTabs = tabmail.tabContainer.childNodes.length;
>+			for (iTab = 0; iTab &lt; numTabs; iTab++)
>+				if (tabmail.tabContainer.childNodes[iTab] == document.popupNode)
>+					break;
>+			tab = tabmail.tabInfo[iTab];
>+			if (!tab.canClose) return false;">

This is not the way to do it. Should probably be an xbl method in the tabmail.xml file

Something like <method name="_onTabContextShowing"> maybe?

nubTabs and iTabs needs to get delcared (with "let" or "var"), otherwise the'd get global variables.

Put the return of if (!tab.canClose) return false; onto another line.

>+.tabmail-tabs > .tabmail-tab:first-child > .tab-close-button {
>+  visibility: collapse;
>+} 

Remove the trailing space on the last of these three lines.

Comment 24

[hendrik]

Attachment: Patch version 2

Hi,

*Removed the disabling of Ctrl-W when there is only one tab, as mentioned in comment 22
*Added extra xbl method 'onTabContextShowing' in tabmail.xml file
*Removed extra spaces and corrected bad indentation

Please feel free to comment. Thank you.

Comment 25

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 371686 [details] [diff] [review]
Patch version 2

Hendrik would like to request review, setting review flag for him...

Bringing over ui-r+.

Comment 26

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

Please add documentation for the onTabContextShowing method.  The tabmail.xml binding has a large documentation block for the tabmail binding where it can be added.

I would suggest changing onTabContextShowing to include the word menu somewhere, ex: "onTabContextMenuShowing".

The 'if' that belongs to the 'for' loop is not indented (relative to the 'for') but should be.  I would also suggest adding braces around the 'if' as well.

Comment 27

[Phil Ringnalda (:philor)]

You could save yourself some time checking through every tab, and make the parameter name more true, and fix bug 472104 for me, all at once, by checking whether aTabNode is actually a tab node, and returning false if it's not.

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 371686 [details] [diff] [review]
Patch version 2

Thx henrik, this is a looking quite good, just a few tweaks to go.

Please fix comment 26, also comment 27 if you want. 
(As for bracing, our style rule is something like: "braces shouldn't be added unnecessary to on-row statements, but if one of the if-else clauses has braces, they all should".)

>+            let showContextMenu = true;
>+            let iTab, numTabs = this.tabContainer.childNodes.length;

Not too crazy about declaring this way... And you don't really need the numTabs anyway, just inline it.

>+            for (iTab = 0; iTab < numTabs; iTab++)
>+            if (this.tabContainer.childNodes[iTab] == aTabNode)
>+              break;
>+            let tab = this.tabInfo[iTab];
>+            if (!tab.canClose)
>+              showContextMenu = false;

And i assume here when you find the tab you can just "return tab.canClose;" inside the loop, so then it'd also become  for(let iTab = 0....)

Comment 29

[hendrik]

Attachment: Patch version 3

Hi,

*Changed method name to 'onTabContextMenuShowing'
*Added documentation as discussed with Andrew
*Added checking for bug 472104 as mentioned in comment 27
*Inline the variables as suggested in comment 28

Thanks.

Comment 30

[hendrik]

Comment on attachment 372609 [details] [diff] [review]
Patch version 3

Bringing over ui-r+

Comment 31

[Magnus Melin [:mkmelin]]

Comment on attachment 372609 [details] [diff] [review]
Patch version 3

Looks good, just some small nits:

Please put the documentation of the method on top of the file with the other method documentations.

>+        <body>
>+          <![CDATA[
>+            // To determine whether the 'Close Tab' context menu should be
>+            //  visible according to the tab node that was being clicked on.
>+            //  If the tab node is not closible, the context menu 

"closable", no?
Also please just one space after the //s

>+
>+            // fix for bug #472104

We usually don't put bug references in for minor things like that. You can either remove it, or put a comment for where it comes into play. (Use a proper senctence: capitalize first char and use and ending dot if you do.)

>+            if (aTabNode.localName != "tab")
>+              return false;

Comment 32

[hendrik]

Attachment: Patch version 4

Hi,

*Removed the reference for bug 472104
*Only one space after the double slashes ('//')
*Corrected closible to closable
*Documentation for the method remains at the same location as discussed with Magnus

Thanks.

Comment 33

[Magnus Melin [:mkmelin]]

Comment on attachment 373319 [details] [diff] [review]
Patch version 4

Looks good, thx for the patch hendrik! r=mkmelin

Comment 35

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/51aae65ce7e0