Closed Bug 395388 Opened 14 years ago Closed 12 years ago

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

Categories

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

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: Thunderbird_Mail_DE, Assigned: joybaggins)

References

(Blocks 1 open bug)

Details

(Keywords: polish, student-project)

Attachments

(2 files, 3 obsolete files)

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.
Can you explain that a bit more? Don't see what you mean... 
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.
Flags: blocking-thunderbird3?
Aah, I see. Thx Phil!
OS: Windows XP → All
Hardware: PC → All
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.
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?
(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.

tabs in general are not ready, but we should definitely fix this.
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Whiteboard: tabs
Assignee: mscott → nobody
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Priority: -- → P2
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.
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.
Blocks: tabsmeta
Duplicate of this bug: 429782
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.
Assignee: nobody → dmose
Severity: major → minor
Keywords: polish
Priority: P2 → P3
Summary: It must not be possible to close the 3-pane-window-tab → 3-pane-window-tab shouldn't appear to be closible, since it's not
Target Milestone: --- → Thunderbird 3.0rc1
And, really, I think davida's instincts were right: we would actually ship without this; leaving blocking- and wanted+.
Assignee: dmose → nobody
(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.
Assignee: nobody → joybaggins
Status: NEW → ASSIGNED
Keywords: student-project
See bug 485268 for the "do something better, once we've stopped pretending you can close it when you can't" bug.
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?
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.
(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?
(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!
> 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.
Attached patch Patch version 1 (obsolete) — Splinter Review
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!
Attachment #371127 - Flags: ui-review?(clarkbw)
Attachment #371127 - Flags: review?(mkmelin+mozilla)
Attached image Screenshot
Component: Mail Window Front End → Toolbars and Tabs
QA Contact: front-end → toolbars-tabs
Whiteboard: tabs
Attachment #371127 - Flags: ui-review?(clarkbw) → ui-review+
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 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.
Attachment #371127 - Flags: review?(mkmelin+mozilla) → review-
Attached patch Patch version 2 (obsolete) — Splinter Review
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.
Attachment #371127 - Attachment is obsolete: true
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+.
Attachment #371686 - Flags: ui-review+
Attachment #371686 - Flags: review?(mkmelin+mozilla)
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.
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 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....)
Attachment #371686 - Flags: review?(mkmelin+mozilla)
Attached patch Patch version 3 (obsolete) — Splinter Review
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.
Attachment #371686 - Attachment is obsolete: true
Attachment #372609 - Flags: review?(mkmelin+mozilla)
Comment on attachment 372609 [details] [diff] [review]
Patch version 3

Bringing over ui-r+
Attachment #372609 - Flags: ui-review+
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;
Attached patch Patch version 4Splinter Review
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.
Attachment #372609 - Attachment is obsolete: true
Attachment #373319 - Flags: ui-review+
Attachment #373319 - Flags: review?(mkmelin+mozilla)
Attachment #372609 - Flags: review?(mkmelin+mozilla)
Comment on attachment 373319 [details] [diff] [review]
Patch version 4

Looks good, thx for the patch hendrik! r=mkmelin
Attachment #373319 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
Target Milestone: Thunderbird 3.0rc1 → Thunderbird 3.0b3
Duplicate of this bug: 488914
Checked in: http://hg.mozilla.org/comm-central/rev/51aae65ce7e0
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 472104
You need to log in before you can comment on or make changes to this bug.