Last Comment Bug 644169 - (tb-tabsontop) Move tab selector above the mail toolbar, and give each tab its own toolbar (tabs on top for Thunderbird)
(tb-tabsontop)
: Move tab selector above the mail toolbar, and give each tab its own toolbar (...
Status: RESOLVED FIXED
[gs]
: dev-doc-needed
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: Trunk
: x86 All
: -- enhancement with 6 votes (vote)
: Thunderbird 11.0
Assigned To: Mike Conley (:mconley)
:
:
Mentors:
http://getsatisfaction.com/mozilla_me...
: 702797 (view as bug list)
Depends on: 717261 721405 725507 735622 456169 687836 711508 712395 712560 712624 712938 713008 713058 713061 713071 713079 713919 715565 717264 719008 724324 728309 728964 732803 738857
Blocks: tabsmeta 508776 661742 705381 709572 710831 710838 710867 712159 712283 712285 712322 724213
  Show dependency treegraph
 
Reported: 2011-03-23 08:51 PDT by [:Aureliano Buendía]
Modified: 2012-03-26 02:32 PDT (History)
36 users (show)
mconley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Thunderbird problem (12.49 KB, image/png)
2011-05-01 17:41 PDT, Marco Biscaro
no flags Details
Tabs on top with global menu on FF (25.13 KB, image/png)
2011-05-01 17:42 PDT, Marco Biscaro
no flags Details
Thunderbird 3.3 with globalmenu-extension (23.73 KB, image/png)
2011-05-02 06:20 PDT, Mike Conley (:mconley)
no flags Details
WIP Patch 1 (41.88 KB, patch)
2011-10-11 08:32 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Tabs on top - the 3pane (111.23 KB, image/png)
2011-10-11 08:43 PDT, Mike Conley (:mconley)
no flags Details
Tabs on top - search - Ubuntu (67.92 KB, image/png)
2011-10-11 08:44 PDT, Mike Conley (:mconley)
no flags Details
WIP Patch 2 (41.85 KB, patch)
2011-10-11 11:24 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Tabs on top - the 3pane - better styling - Ubuntu (127.50 KB, image/png)
2011-10-13 12:21 PDT, Mike Conley (:mconley)
no flags Details
WIP Patch 3 (43.28 KB, patch)
2011-10-13 12:25 PDT, Mike Conley (:mconley)
jonathan.protzenko: feedback+
Details | Diff | Splinter Review
Tabs on Top - 3 pane - Windows 7 (117.64 KB, image/png)
2011-10-13 13:00 PDT, Mike Conley (:mconley)
no flags Details
Tabs on Top - 3 pane - OSX 10.6.8 (86.20 KB, image/png)
2011-10-13 14:01 PDT, Mike Conley (:mconley)
no flags Details
Small CSS patch to get rid of the gap under Linux (583 bytes, patch)
2011-10-14 10:09 PDT, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
Patch for Windows Aero visual enhancement (2.72 KB, patch)
2011-10-14 13:54 PDT, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
Tabs on Top with CSS patch under Win7 (55.79 KB, image/png)
2011-10-14 13:59 PDT, Richard Marti (:Paenglab)
no flags Details
Mac enhancement CSS patch (5.20 KB, patch)
2011-10-16 01:26 PDT, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
Mac with CSS patch applied (44.95 KB, image/png)
2011-10-16 01:27 PDT, Richard Marti (:Paenglab)
no flags Details
WIP Patch 4 (54.33 KB, patch)
2011-10-18 12:10 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Windows with menubar hidden (39.11 KB, image/png)
2011-10-20 08:54 PDT, Mike Conley (:mconley)
no flags Details
WIP Patch 5 (77.85 KB, patch)
2011-10-20 11:35 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
WIP Patch 6 (69.54 KB, patch)
2011-10-21 08:25 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Theme patch (13.64 KB, patch)
2011-10-22 13:58 PDT, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
The Tabs on top under XP (29.38 KB, image/png)
2011-10-22 14:03 PDT, Richard Marti (:Paenglab)
no flags Details
Mockup under XP with border between mail toolbar and content (35.82 KB, image/png)
2011-10-22 14:09 PDT, Richard Marti (:Paenglab)
no flags Details
Windows 7 Aero - Menubar off (80.17 KB, image/png)
2011-10-24 06:57 PDT, Mike Conley (:mconley)
no flags Details
Windows 7 Aero - Menubar on (106.02 KB, image/png)
2011-10-24 06:57 PDT, Mike Conley (:mconley)
no flags Details
Windows 7 Aero - Persona (204.98 KB, image/png)
2011-10-24 06:58 PDT, Mike Conley (:mconley)
no flags Details
Windows 7 Aero - Addons Manager - Menubar on (297.37 KB, image/png)
2011-10-24 07:03 PDT, Mike Conley (:mconley)
no flags Details
Windows 7 Aero - Content Tab - Menubar On (192.46 KB, image/png)
2011-10-24 07:04 PDT, Mike Conley (:mconley)
no flags Details
Patch v1 (78.55 KB, patch)
2011-10-24 07:40 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Mac CSS patch (2.21 KB, patch)
2011-10-24 14:17 PDT, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
Patch v2 (82.54 KB, patch)
2011-10-26 09:08 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Patch v3 (87.72 KB, patch)
2011-10-26 13:16 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Patch solving three issues under Aero (1.01 KB, patch)
2011-10-27 14:01 PDT, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
Use the actual Fx tabs on aero (7.96 KB, patch)
2011-10-30 13:38 PDT, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
Patch v4 (94.27 KB, patch)
2011-10-31 08:40 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
remove the align="start" (2.57 KB, patch)
2011-11-07 11:19 PST, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
Patch v5 (125.92 KB, patch)
2011-11-22 10:50 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Patch v6 (126.28 KB, patch)
2011-11-22 11:50 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
tiny update (99.74 KB, patch)
2011-11-24 09:07 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
tiny bitrot fix (99.06 KB, patch)
2011-11-25 07:57 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
trying again (99.59 KB, patch)
2011-11-25 08:23 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
Fixes some errors in the last patch (99.94 KB, patch)
2011-11-25 08:57 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
Patch v8 (99.76 KB, patch)
2011-11-28 13:58 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
style fixes for Win7 (1.24 KB, patch)
2011-11-29 09:21 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
Style fixes for Linux and Win7 (2.48 KB, patch)
2011-11-30 07:48 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
1px separator between tab selector and content in contentTabs (6.55 KB, patch)
2011-11-30 14:03 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
1px toolbar in content tabs, now invisible in about:addons, and in Linux (8.41 KB, patch)
2011-12-01 13:52 PST, Mike Conley (:mconley)
squibblyflabbetydoo: feedback+
Details | Diff | Splinter Review
1px toolbar seperator in content tabs on all platforms, invisible in about:addons (9.73 KB, patch)
2011-12-02 07:41 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Patch v9 (108.33 KB, patch)
2011-12-05 08:31 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Windows XP additional patch (15.41 KB, patch)
2011-12-06 12:42 PST, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
Patch v9 (minor update) (108.48 KB, patch)
2011-12-06 17:40 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
Windows XP additional patch (16.98 KB, patch)
2011-12-07 10:29 PST, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
Patch v10 (155.35 KB, patch)
2011-12-07 13:02 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
pinstripe tab (7.33 KB, patch)
2011-12-08 07:01 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
slight update to previous patch (8.72 KB, patch)
2011-12-08 08:43 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
pinstripe tabs v3 (9.66 KB, patch)
2011-12-09 05:28 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
pinstripe tabs v3 (9.55 KB, patch)
2011-12-10 11:52 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
pinstripe tabs v4 (101.90 KB, patch)
2011-12-11 14:00 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
Patch v11 (121.30 KB, patch)
2011-12-12 07:10 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
pinstripe tabs v4(ish) (9.73 KB, text/plain)
2011-12-12 08:41 PST, Andreas Nilsson (:andreasn)
no flags Details
Patch v12 (125.65 KB, patch)
2011-12-12 09:59 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
patch v13 (wip) (125.51 KB, patch)
2011-12-13 07:25 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
patch v13 (wip) (125.55 KB, patch)
2011-12-13 08:28 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
patch v13 (126.96 KB, patch)
2011-12-13 12:38 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
Patch v13 - now with more context (160.31 KB, patch)
2011-12-14 08:29 PST, Mike Conley (:mconley)
sid.bugzilla: review-
bwinton: ui‑review+
Details | Diff | Splinter Review
Patch v14 (138.35 KB, patch)
2011-12-15 13:45 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Patch v14 (138.35 KB, patch)
2011-12-15 14:03 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Patch v15 (137.06 KB, patch)
2011-12-16 09:46 PST, Mike Conley (:mconley)
sid.bugzilla: review-
Details | Diff | Splinter Review
WIP Patch 16 (137.63 KB, patch)
2011-12-17 08:54 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
WIP Patch 17 (138.35 KB, patch)
2011-12-17 12:01 PST, Mike Conley (:mconley)
bwinton: ui‑review-
Details | Diff | Splinter Review
WIP Patch 18 (136.88 KB, patch)
2011-12-18 13:14 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
WIP patch 19 (104.43 KB, patch)
2011-12-19 08:15 PST, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
Patch v19 (137.65 KB, patch)
2011-12-19 08:49 PST, Mike Conley (:mconley)
sid.bugzilla: review-
Details | Diff | Splinter Review
Patch v20 (139.28 KB, patch)
2011-12-20 07:05 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Patch v21 (139.92 KB, patch)
2011-12-20 07:45 PST, Mike Conley (:mconley)
sid.bugzilla: review+
standard8: superreview+
Details | Diff | Splinter Review
Patch for accidental undelete command reassignment (1.11 KB, patch)
2011-12-21 07:51 PST, Mike Conley (:mconley)
squibblyflabbetydoo: review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review
Border between tab and email body (19.83 KB, image/png)
2012-01-26 04:01 PST, Mihovil Stanic [:Mikeyy - L10n HR]
no flags Details

Description [:Aureliano Buendía] 2011-03-23 08:51:32 PDT
Implements "Tabs on top" feature (for me is to have opacity of toolbar buttons on Aero Theme (like in FF 4.0)).
Comment 1 Marco Biscaro 2011-05-01 17:40:47 PDT
Also affects Ubuntu 11.04, when using globalmenu extension.

In this case, the menu is renderend on panel, but the progress indicator still using that space (see screenshot). Using tabs on top (like FF does) the problem will be solved (see the another screenshot).
Comment 2 Marco Biscaro 2011-05-01 17:41:45 PDT
Created attachment 529399 [details]
Thunderbird problem
Comment 3 Marco Biscaro 2011-05-01 17:42:27 PDT
Created attachment 529400 [details]
Tabs on top with global menu on FF
Comment 4 Mark Banner (:standard8, afk until Dec) 2011-05-01 23:58:18 PDT
(In reply to comment #1)
> Also affects Ubuntu 11.04, when using globalmenu extension.
> 
> In this case, the menu is renderend on panel, but the progress indicator still
> using that space (see screenshot). Using tabs on top (like FF does) the problem
> will be solved (see the another screenshot).

Sorry, but I believe we don't support that extension here afaik. You need to file that issue in around here: https://launchpad.net/globalmenu-extension

This bug is about implementing tabs-on-top in Thunderbird, and not issues with specific extensions.
Comment 5 Mike Conley (:mconley) 2011-05-02 06:20:39 PDT
Created attachment 529457 [details]
Thunderbird 3.3 with globalmenu-extension

Marco:

Hey - thanks for your feedback!

In regards to tabs-on-top: it's definitely on our TODO list.

And regarding globalmenu-extension:  the progress indicator takes up that space in TB 3.1.9, but not in TB 3.3 (see attached screenshot).  So, by the time tabs-on-top comes around, this shouldn't be a problem.

Cheers,

-Mike
Comment 6 Marco Biscaro 2011-05-02 06:54:54 PDT
@Mark: yes, I understand that here is not the place to report bugs against extensions, but I commented that implementing tabs on top is a way to solve this.

Moreover, the affected platform is 'Windows 7' and the original description talks about Aero theme. I'm just highlighting that also affects other platforms (despite I know that almost every feature is available in any OS). Linux users want tabs on top too! :)

Anyway, thanks for the direction.

@Mike: I'm using Thunderbird/3.3a4pre and I'm affected by the commented bug. Should I report a bug against globalmenu-extension?
Comment 7 Mike Conley (:mconley) 2011-05-02 07:07:30 PDT
Marco:

> I'm using Thunderbird/3.3a4pre and I'm affected by the commented bug.
> Should I report a bug against globalmenu-extension?

Oh - yes, you should report your bug here:

https://bugs.launchpad.net/globalmenu-extension

Please provide your screenshot, and the text contents of the Thunderbird About dialog (should look something like this:  Mozilla/5.0 (X11; Linux i686; rv:2.0b13pre) Gecko/20110314 Thunderbird/3.3a3)
Comment 8 Jim Porter (:squib) 2011-09-03 17:48:08 PDT
Confirming, since bwinton and I have discussed this and agree it's a good thing to do.
Comment 9 Charles 2011-09-16 11:19:08 PDT
Thanks for confirming this, Jim - until this bug is fixed, the Personal Titlebar extension developer cannot support Thunderbird with his most excellent extension that allows me to place all of my toolbar buttons on the menu bar, then when I hide the menubar, they all show up in the Window Titlebar (which iumnsho has always been a huge waste of space).
Comment 10 Charles 2011-09-16 11:21:40 PDT
Also, why is this marked as a Windows 7 only bug? The Personal Titlebar extension allows me to do this in Firefox on Windows XP...
Comment 11 Mike Conley (:mconley) 2011-10-11 08:32:18 PDT
Created attachment 566228 [details] [diff] [review]
WIP Patch 1

My first efforts at this.  This is my first serious foray into XBL, so I might be missing out on some best practices here.  Hopefully I'll fix that after some review.

This patch depends on the patch in bug 687836.
Comment 12 Mike Conley (:mconley) 2011-10-11 08:43:45 PDT
Created attachment 566234 [details]
Tabs on top - the 3pane
Comment 13 Mike Conley (:mconley) 2011-10-11 08:44:22 PDT
Created attachment 566235 [details]
Tabs on top - search - Ubuntu
Comment 14 Charles 2011-10-11 09:21:48 PDT
Awesome, thanks for working on this Mike!

I'm happy to provide testing when a test build is available (can't build one myself  I'm sorry to say)...
Comment 15 Mike Conley (:mconley) 2011-10-11 11:24:06 PDT
Created attachment 566275 [details] [diff] [review]
WIP Patch 2

Alright, this patch is starting to come together.

I'm a little unsure about how my changes might impact other portions of TB that I haven't thought about...my relative lack of experience with XBL worries me a bit.  So, feedback requested.  :)

-Mike
Comment 16 Mike Conley (:mconley) 2011-10-13 12:21:57 PDT
Created attachment 566907 [details]
Tabs on top - the 3pane - better styling - Ubuntu
Comment 17 Mike Conley (:mconley) 2011-10-13 12:25:45 PDT
Created attachment 566909 [details] [diff] [review]
WIP Patch 3

A small change here - now the mail toolbar is given the "inline-toolbar" class, as opposed to "toolbar-primary", which makes the toolbar the same colour as the selected tab, tab panel, etc.

Note the dark gap between the tab and the tabpanel.  At this point, unsure of where that's coming from - might need some help from andreasn to nail that down.

Also, we'll probably want some kind of visual separation between the mail toolbar and the thread/tree panes.
Comment 18 Mike Conley (:mconley) 2011-10-13 12:26:51 PDT
Hey Andreas - do you know where that little dark gap between the tab and the tabpanel is likely coming from?
Comment 19 Mike Conley (:mconley) 2011-10-13 13:00:04 PDT
Created attachment 566912 [details]
Tabs on Top - 3 pane - Windows 7
Comment 20 Mike Conley (:mconley) 2011-10-13 14:01:30 PDT
Created attachment 566932 [details]
Tabs on Top - 3 pane - OSX 10.6.8

Yikes - the toolbar looks a bit funky on OSX.  Also, it defaulted to icons-and-text, as opposed to icons-beside-text.  Not sure why yet.
Comment 21 Jim Porter (:squib) 2011-10-13 17:37:06 PDT
I'd recommend not using inline-toolbar for the mail toolbar. I made that style to refer to toolbars that are on the same line as other stuff (e.g. the header toolbar and the attachment toolbar).
Comment 22 Richard Marti (:Paenglab) 2011-10-14 10:09:11 PDT
Created attachment 567126 [details] [diff] [review]
Small CSS patch to get rid of the gap under Linux

This patch is only to get rid of the dark gap between the tab and the tabpanel under Linux. Windows needs more tweaking.
Comment 23 Richard Marti (:Paenglab) 2011-10-14 13:54:05 PDT
Created attachment 567175 [details] [diff] [review]
Patch for Windows Aero visual enhancement

This patch makes tabs on top looking better. No gaps, transparency behind tabs and toolbar like FF. It's not perfect and can improved but as a base for further work it should be okay.

I gave tabcontainer align="end" to position the tabs on bottom. I tested it also under Linux and it should break nothing.
Comment 24 Richard Marti (:Paenglab) 2011-10-14 13:59:34 PDT
Created attachment 567177 [details]
Tabs on Top with CSS patch under Win7
Comment 25 Richard Marti (:Paenglab) 2011-10-16 01:26:25 PDT
Created attachment 567322 [details] [diff] [review]
Mac enhancement CSS patch

This is a rough patch to make it look better also under Mac.

The XP theme isn't ready because I have now no XP PC at hand (tweaked under Win7 it works but I have to check it under real XP).
For all themes: Actually the personas are only applying to the tab bar.
Comment 26 Richard Marti (:Paenglab) 2011-10-16 01:27:47 PDT
Created attachment 567323 [details]
Mac with CSS patch applied
Comment 27 Jonathan Protzenko [:protz] 2011-10-16 06:54:40 PDT
Comment on attachment 566909 [details] [diff] [review]
WIP Patch 3

This look very good! I think that's a much-needed improvement, and maybe we'll finally have composition and address book management in a new tab (although I'd rather see these two improved first, but I heard some guy named mconley is working on the former).

Here's some comments, first of all code-wise.
- Looks like the patch has bitrotten, I had to comment out two lines in mail/base/content/quickFilterBar.js to make Thunderbird work (and I'm still getting errors in the error console). Thinking about it, that's probably the addition of tabmail-buttons-thunderbird-private.
- You changed some elements around but AFAICT you kept the same IDs. This sounds perfectly right. Is the change going to be transparent for addon developers? If not, we should make sure we communicate as loudly as we can about this.
- I'm being curious, but why do you need to specially handle the tooltip?

Appearance-wise:
- what about personas? the personas do not apply to the mail toolbar, and that feels weird compared to the previous state of things,
- that might have been mentioned already, but on linux there's a 1px solid gray line that's kinda ugly, plus some extra space that shouldn't (imho) be there between the bottom of the tabs and the top of the toolbar (using a persona),
- I just can't find out how to get rid of the qfb (might be my rebasing the patch not properly)
- the first tab displays no title when we startup Thunderbird, which is imho weird, might want to fix this here while we're at it, or do that in a followup bug :-)
- open message in a new window seems to be b0rken.

Thanks for tackling this, I guess it was not easy as it seems :)

Cheers,

jonathan
Comment 28 Mike Conley (:mconley) 2011-10-18 12:10:07 PDT
Created attachment 567830 [details] [diff] [review]
WIP Patch 4

This patch fixes a few bugs (the new message window bug, restore default set bug) and integrates Paenglab's splendid CSS patches.
Comment 29 Mike Conley (:mconley) 2011-10-18 12:22:01 PDT
(In reply to Jim Porter (:squib) from comment #21)
> I'd recommend not using inline-toolbar for the mail toolbar. I made that
> style to refer to toolbars that are on the same line as other stuff (e.g.
> the header toolbar and the attachment toolbar).

Thanks - I've dropped it in favour of a custom style.

(In reply to Jonathan Protzenko [:protz] from comment #27)
> Comment on attachment 566909 [details] [diff] [review] [diff] [details] [review]
> WIP Patch 3
> 
> This look very good! I think that's a much-needed improvement, and maybe
> we'll finally have composition and address book management in a new tab
> (although I'd rather see these two improved first, but I heard some guy
> named mconley is working on the former).

He's doing his darndest.  :D

> 
> Here's some comments, first of all code-wise.
> - Looks like the patch has bitrotten, I had to comment out two lines in
> mail/base/content/quickFilterBar.js to make Thunderbird work (and I'm still
> getting errors in the error console). Thinking about it, that's probably the
> addition of tabmail-buttons-thunderbird-private.

Thanks, fixed.

> - You changed some elements around but AFAICT you kept the same IDs. This
> sounds perfectly right. Is the change going to be transparent for addon
> developers? If not, we should make sure we communicate as loudly as we can
> about this.

So while I've kept some ID's, I've also changed the meaning of some ID's.  Like, the mail-toolbar is now within the tabpanels, and it's what has the "Get Mail", "Write" buttons, etc.  The navigation-toolbar has the menubar and the tabs switcher.  Before, the menubar, tabs switcher, and the "Get Mail" toolbar were all under the "mail-toolbar" ID.

> - I'm being curious, but why do you need to specially handle the tooltip?

A lot of that code is carry-over from tabbrowser.xul.  I don't 100% understand the reasoning - I just know that the tooltips did not work before I put it in and hooked it up.

> 
> Appearance-wise:
> - what about personas? the personas do not apply to the mail toolbar, and
> that feels weird compared to the previous state of things,

Fixed!  :)  Although now it doesn't apply to the menubar.  I'm getting help from Andreas on that.

> - that might have been mentioned already, but on linux there's a 1px solid
> gray line that's kinda ugly, plus some extra space that shouldn't (imho) be
> there between the bottom of the tabs and the top of the toolbar (using a
> persona),

Paenglab kindly patched that one.

> - I just can't find out how to get rid of the qfb (might be my rebasing the
> patch not properly)

Hrm - try again with this version.

> - the first tab displays no title when we startup Thunderbird, which is imho
> weird, might want to fix this here while we're at it, or do that in a
> followup bug :-)

Is that fixed in this version?  I'm not seeing that bug.

> - open message in a new window seems to be b0rken.
> 

Thanks, fixed.

> Thanks for tackling this, I guess it was not easy as it seems :)
> 

My pleasure.  I look forward to this being completed!
Comment 30 Mike Conley (:mconley) 2011-10-20 08:54:45 PDT
Created attachment 568407 [details]
Windows with menubar hidden

Here's the patch running on Windows 7 with the menubar hidden.
Comment 31 Mike Conley (:mconley) 2011-10-20 11:35:25 PDT
Created attachment 568451 [details] [diff] [review]
WIP Patch 5

This patch puts back the context item option for displaying the menu bar, and also puts the tab drop indicator below the tabs on Windows.
Comment 32 Mike Conley (:mconley) 2011-10-20 11:40:16 PDT
So a few people have been asking "why in the world are we putting the menubar beneath the tab selector on Windows?"

Answer:  This appears to be how Windows native apps are going as well.  If you look at Internet Explorer 9, the menubar appears below the tabs.  Likely this is because the IE team realized, like we did, that menubars really look *horrible* on glass.  Just horrible.  And hard to read.

So this is our solution to that problem.  It reduces glass, and will look more native on Windows.
Comment 33 Charles 2011-10-20 11:43:06 PDT
Yes, but this will be *optional*, correct (just as it is in Firefox)?
Comment 34 Blake Winton (:bwinton) (:☕️) 2011-10-20 11:51:46 PDT
I'm not sure what you're asking about here, Charles, so I'll try to give a few answers.

The hiding of the menu bar will be optional.
The tabs-on-top won't be, because it's too hard to maintain the divergent code paths.
The menu bar below the tabs on Windows hasn't been decided yet, but I suspect it won't be because it looks terrible on Glass, and because it's hard to maintain, and because Windows Explorer and IE both put it under the Glass area.

(I think it makes sense to leave them where they are on XP/non-Aero.  Mike, could you tell me how hard that would be?)

Thanks,
Blake.
Comment 35 Mike Conley (:mconley) 2011-10-20 12:23:24 PDT
Blake:

I don't think it'd be too hard - I think we'd use a similar technique to the one used by Firefox to put the nav bar above and below the tabs - so basically some CSS magic.

-Mike
Comment 36 Mike Conley (:mconley) 2011-10-21 08:25:37 PDT
Created attachment 568655 [details] [diff] [review]
WIP Patch 6

This patch fixes the following:

-Tabs now have their context menu's back
-tabmail Mozmill tests now pass

I've just noticed a theming issue on gnomestripe - in the compose window, the menutext is really hard to read (since it's applying WindowText to the dark menu background).

This all depends on what the GNOME people want wrt the menu above or below the tab selector.  If it's below, then what we'll need is some CSS magic to paint the menubar a certain way if it's below the TabsToolbar, and another way otherwise.  Is this possible?
Comment 37 Mike Conley (:mconley) 2011-10-21 08:28:41 PDT
I should also mention that I'm more or less happy with the functionality at this point - it looks like we're at feature parity with the old tab selector.

Once the theming work is finished, I think this patch is ready for review / polish.
Comment 38 Charles 2011-10-21 09:50:55 PDT
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #34)
> I'm not sure what you're asking about here, Charles, so I'll try to give a
> few answers.
> 
> The hiding of the menu bar will be optional.
> The tabs-on-top won't be, because it's too hard to maintain the divergent
> code paths.

Well, with Firefox, tabs-on-top is optional... that is what I was asking about.

But the more I think about it, I really don't think it is a problem in Thunderbird (in Firefox, I use Treestyle Tab with tabs on the left side), because I don't like or use tabs in Thunderbird anyway (except for the calendar). The most important thing for me is getting support for the tabs-on-top functionality, because the author of one of my favorite extensions for Firefox (Personal Titlebar) says he can't add support for Thunderbird until Thunderbird supports tabs-on-top.

I disable aero on Windows anyway, and will be using Personal Titlebar (which automatically adds anything/everything added to the menubar to the window titlebar when the menubar is hidden) once it supports Thunderbird.
Comment 39 Richard Marti (:Paenglab) 2011-10-22 13:58:13 PDT
Created attachment 568908 [details] [diff] [review]
Theme patch

This theme patch is for applying on top of WIP Patch 6 for all systems.

- Under Linux the issue with the menu bar in AB and Compozer is correct now.
- The Mac inactive mail-toolbox is now also correct.
- Personas is now applying correctly to the mail-toolbox on all systems with
  personas text color on buttons.
- The tab text is now also using the personas text color on all systems except
  XP because the tabs aren't transparent and light text would it make unreadable.

If you want the menu bar above the tabs then you can use this:

#TabsToolbar {
  -moz-box-ordinal-group: 20;
}

#mail-toolbar-menubar2 {
  -moz-box-ordinal-group: 10;
}
Comment 40 Richard Marti (:Paenglab) 2011-10-22 14:03:47 PDT
Created attachment 568909 [details]
The Tabs on top under XP

Maybe a border between mail toolbar and content could look good. I let Blake decide if needed.
Comment 41 Richard Marti (:Paenglab) 2011-10-22 14:09:57 PDT
Created attachment 568911 [details]
Mockup under XP with border between mail toolbar and content
Comment 42 Mike Conley (:mconley) 2011-10-24 06:57:04 PDT
Created attachment 569049 [details]
Windows 7 Aero - Menubar off

Here's a screenshot of ToT on Windows 7 Aero, with the menubar off.
Comment 43 Mike Conley (:mconley) 2011-10-24 06:57:54 PDT
Created attachment 569050 [details]
Windows 7 Aero - Menubar on

And here we are with the menubar on.
Comment 44 Mike Conley (:mconley) 2011-10-24 06:58:45 PDT
Created attachment 569051 [details]
Windows 7 Aero - Persona

And just for good measure, here's Windows 7 Aero, menubar off, with a Persona.
Comment 45 Mike Conley (:mconley) 2011-10-24 07:03:49 PDT
Created attachment 569055 [details]
Windows 7 Aero - Addons Manager - Menubar on
Comment 46 Mike Conley (:mconley) 2011-10-24 07:04:33 PDT
Created attachment 569057 [details]
Windows 7 Aero - Content Tab - Menubar On
Comment 47 Mike Conley (:mconley) 2011-10-24 07:40:45 PDT
Created attachment 569062 [details] [diff] [review]
Patch v1

Thanks for the awesome theme patch, Paenglab!  You rock!

I think this thing is almost ready for some ui and code review.  Going to run it through the paces on try first though:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=30700f48344c
Comment 48 Mike Conley (:mconley) 2011-10-24 08:26:56 PDT
Try server is still a little ill, so the Mozmill test results don't look too good.  However, I just ran the tests locally on my Linux box, and they all passed.  Woot!  I'll try Windows and OSX next...
Comment 49 Richard Marti (:Paenglab) 2011-10-24 14:17:42 PDT
Created attachment 569181 [details] [diff] [review]
Mac CSS patch

CSS patch Mike asked for Mac. I should hide the border when the mail-bar is collapsed. Also a quick-n-dirty patch to give more contrast to the tabs with personas. This needs more love but should work better than before.
Comment 50 Mike Conley (:mconley) 2011-10-26 09:08:12 PDT
Created attachment 569694 [details] [diff] [review]
Patch v2

Integrates Paenglab's latest OSX patch, and adds the following features:

1)  Right clicking on the empty space of the tab bar produces the toolbar context menu
2)  When customizing toolbars, dragging items onto the tab bar redirects them to the toolbar to the right of the tab bar.
Comment 51 Mike Conley (:mconley) 2011-10-26 13:16:46 PDT
Created attachment 569769 [details] [diff] [review]
Patch v3

This patch fixes some style issues with Personas, and also adds some Mozmill tests to ensure that dragging toolbar items onto the tab bar redirects to the tab toolbar appropriately.
Comment 52 Richard Marti (:Paenglab) 2011-10-27 14:01:49 PDT
Created attachment 570082 [details] [diff] [review]
Patch solving three issues under Aero

This patch solves three issues mconley found under the Aero theme with Personas.
- no left/right padding on the tabbar
- tabs "bleeding"
- tabs have white bottom border
Comment 53 Richard Marti (:Paenglab) 2011-10-30 13:38:36 PDT
Created attachment 570567 [details] [diff] [review]
Use the actual Fx tabs on aero

This additional patch uses the actual Fx tabs under Aero. This patch includes also the fixes from previous patch (Patch solving three issues under Aero)
Comment 54 Mike Conley (:mconley) 2011-10-31 08:40:50 PDT
Created attachment 570720 [details] [diff] [review]
Patch v4

Integrating Paenglab's fixes, and now the menubar is customizable.
Comment 55 Mike Conley (:mconley) 2011-11-03 06:50:12 PDT
Comment on attachment 570720 [details] [diff] [review]
Patch v4

I think I'm ready for my first round of reviews.

If you're interested in trying this thing out (especially if doing ui-review), there are try builds available here:

OSX / Linux:

https://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/mconley@mozilla.com-7ec3af623df4/

For Windows:

https://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/mconley@mozilla.com-aed95c93e9c1/

All feedback welcome!  Thanks,

-Mike
Comment 56 Richard Marti (:Paenglab) 2011-11-03 13:18:31 PDT
The buttons on the menubar don't have a 'Icons beside Text' mode also when chosen. The navigation-toolbox never get's a labelalign="end" like the mail-toolbox.
Comment 57 Jim Porter (:squib) 2011-11-05 02:15:31 PDT
Comment on attachment 570720 [details] [diff] [review]
Patch v4

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

This is just a quick pass; I haven't looked at everything yet.

One thing I thought we were doing though was to have the menubar sit on top of the tabs on Linux. It doesn't look like that for me.

::: mail/base/content/mailCore.js
@@ +290,4 @@
>  
>    var firstMenuItem = popup.firstChild;
>  
> +  toolboxIds.forEach(function(toolboxId) {

I think "for (let toolboxId in Iterator(toolboxIds)) {" is more commonly-used in Mozilla code, but I doubt this matters a whole lot.

::: mail/base/content/mailWindowOverlay.xul
@@ +945,3 @@
>    <!-- Menu -->
>    <toolbar type="menubar" id="mail-toolbar-menubar2" class="chromeclass-menubar" customizable="true"
> +           toolboxid="mail-toolbox"

Should we allow the mail toolbar buttons on the menubar? This could be strange when you're on a non-mail tab (not that it's any worse than it is with tabs-on-bottom). At some point, I expect we'll have tab-specific toolbars for non-mail tabs, which might have unexpected behavior in light of this. I'm ok with leaving this until then, though.

::: mail/base/content/messenger.xul
@@ +273,5 @@
> +<tooltip id="tabmail-tabs-tooltip" onpopupshowing="document.getElementById('tabmail').createTooltip(event);"/>
> +
> +  <toolbox id="navigation-toolbox" class="toolbox-top">
> +
> +    <toolbar id="TabsToolbar" class="toolbar-primary">

I'd go with "tabs-toolbar" here, since that's the case style used everywhere else.

::: mail/themes/gnomestripe/mail/quickFilterBar.css
@@ +37,4 @@
>  }
>  
>  #qfb-show-filter-bar > .toolbarbutton-icon {
> +  padding: 1px 3px 2px;

I'm not sure we want this, since none of the other toolbar buttons have this...

@@ +72,5 @@
>  
>  /* :::: Filter Buttons :::: */
> +#quick-filter-bar {
> +  -moz-appearance: toolbar;
> +}

Not sure we want this either, since on my theme, it looks a bit obnoxious.
Comment 58 Wayne Mery (:wsmwk, NI for questions) 2011-11-07 11:17:54 PST
I've requested feedback about the try builds in http://getsatisfaction.com/mozilla_messaging/tags/bug_644169
Comment 59 Richard Marti (:Paenglab) 2011-11-07 11:19:08 PST
Created attachment 572541 [details] [diff] [review]
remove the align="start"

I've found with the Tabs-on-top TB under Win7 when a separator is in the toolbar the buttons are growing from 24px to 28px. This is because of http://mxr.mozilla.org/comm-central/source/mail/themes/qute/mail/messageHeader-aero.css#372. This is needed because the attachment-view-toolbar has a align="start". I tried it without the align and I saw no difference and the separator rules aren't needed anymore.

Squib asked me to add the patch here.
Comment 60 Paul [pwd] 2011-11-09 05:14:08 PST
I gave the try build a whirl and really liked it. The menu bar seems to be off-colour, though that'll be negated when Bug 650170 lands. The buttons feel a lot cleaner when set against the tab rather than against the glass. The quick filter button for example looks quite poor. Can we not do what Firefox does with its Aero buttons?
Comment 61 Blake Winton (:bwinton) (:☕️) 2011-11-09 07:41:41 PST
(In reply to Paul [sabret00the] from comment #60)
> The quick filter button for example looks quite poor. Can we not do what
> Firefox does with its Aero buttons?

I'ld be up for that.  Did you want to try creating a patch that changes the theme?

Thanks,
Blake.
Comment 62 Philipp Kewisch [:Fallen] 2011-11-13 04:04:59 PST
I'd like to hear your thoughts on how the menubar should react within other tabs. The menus obviously contain a lot of mail-specific items and extensions tend to add some entries there too.

With the menubar inside the tab, I see room for the expectation that the items are specific to the current tab. On the other hand, changing menus to hide items based on the tab would confuse such users that are used to menus (or have a global menu like on mac).
Comment 63 Mike Conley (:mconley) 2011-11-14 14:40:09 PST
Philipp:

Blake might correct me on some of this, since it's really his call, but here's what I think we've decided:

1)  On Linux, the menubar will go above the tabs.
2)  Once we get data from Test Pilot about menu item usage, we hope to develop a similar "Thunderbird" button menu, like the one in Firefox.  For OSX, this might involve some menu reorganization, too.
3)  I don't believe menus will be switching content from tab to tab, though they might enable / disable from tab to tab.

Do I have that right, Blake?

-Mike
Comment 64 Paul [pwd] 2011-11-15 09:57:10 PST
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #61)
> (In reply to Paul [sabret00the] from comment #60)
> > The quick filter button for example looks quite poor. Can we not do what
> > Firefox does with its Aero buttons?
> 
> I'ld be up for that.  Did you want to try creating a patch that changes the
> theme?
> 
> Thanks,
> Blake.

I tried to write a patch but was thwarted by the hurdle. :o(
Comment 65 Blake Winton (:bwinton) (:☕️) 2011-11-15 09:59:51 PST
If you can find me on IRC, let me know what problems you were running into, and I'll try to give you a hand over.  ;)
Comment 66 rsx11m 2011-11-15 20:40:20 PST
Some feedback on the Windows try-server builds:

 - with mail.tabs.autoHide set true, the quick-filter toggle flushes to the left,
   that's a bit odd as it's on the right end when at least one tab is shown;

 - I can customize the main toolbar and the menu toolbar, but not switch the tab
   bar back below the main toolbar for "tabs below toolbar";

 - when dragging the buttons from the main toolbar to the menu bar to emulate the
   previous point, they retain "icons above text" if that was the setting for the
   main toolbar even if switched to "icons beside text".
Comment 67 Andreas Nilsson (:andreasn) 2011-11-16 06:49:59 PST
(In reply to rsx11m from comment #66)
>  - I can customize the main toolbar and the menu toolbar, but not switch the
> tab bar back below the main toolbar for "tabs below toolbar";

This would add additional complexity and just behave oddly with the specific toolbars for the Compose in tab (bug 449299) and Address book in tab (bug 457270) features.
Comment 68 Andreas Nilsson (:andreasn) 2011-11-16 06:53:58 PST
*** Bug 702797 has been marked as a duplicate of this bug. ***
Comment 69 rsx11m 2011-11-16 07:34:23 PST
While I see where comment #67 is coming from, Firefox provides an option to place the tabs below the toolbars (bug 544815), thus the Thunderbird implementation wouldn't be in parity with Firefox any more. Users finding that option in Firefox will also likely complain that it doesn't exist in Thunderbird (and not everybody will use composition and/or address book in tabs).
Comment 70 Charles 2011-11-16 07:57:57 PST
I agree with rsx11m...

Every effort should be made to keep things in parity with Firefox unless there is a very good reason not to do so.
Comment 71 pqwoerituytrueiwoq 2011-11-16 08:34:31 PST
Comments 9 through 12 of bug 702797 are relevant to this bug's comments
Comment 72 Mike Conley (:mconley) 2011-11-22 10:50:44 PST
Created attachment 576206 [details] [diff] [review]
Patch v5

This patch addresses some of squibs review comments, integrates Paenglab's patch, fixes the problem where toolbarbuttons in the menubar cannot have text go beside the icon, and allows the entire tab toolbar to be collapsed when mail.tabs.autoHide is true.
Comment 73 Mike Conley (:mconley) 2011-11-22 10:56:19 PST
> One thing I thought we were doing though was to have the menubar sit on top
> of the tabs on Linux. It doesn't look like that for me.

Oh, I forgot to mention - this latest patch also puts the menubar back above the tab selector on Linux.

> I think "for (let toolboxId in Iterator(toolboxIds)) {" is more
> commonly-used in Mozilla code, but I doubt this matters a whole lot.

I think I ran into a kind of weird JS closure bug here, or maybe I just went about things wrong - when I used the for / Iterator loop, the toolboxId passed to the menu item command event handler would be "mail-toolbar" instead of "navigation-toolbar" when the command event was fired - even though "navigation-toolbar" should have been written, as it was the value of toolbarId when the event callback was created.  :/

Using the forEach seems to sidestep that.  Any idea where I may have gone wrong there?

> Should we allow the mail toolbar buttons on the menubar? This could be
> strange when you're on a non-mail tab (not that it's any worse than it is
> with tabs-on-bottom). At some point, I expect we'll have tab-specific
> toolbars for non-mail tabs, which might have unexpected behavior in light of
> this. I'm ok with leaving this until then, though.

andreasn, bwinton and I were talking about this - we might want tab-specific buttons to be disabled when on a different tab.  We might punt until later though, when we put compose / address book into tabs.

> I'd go with "tabs-toolbar" here, since that's the case style used everywhere
> else.

Good idea.  Done.

> I'm not sure we want this, since none of the other toolbar buttons have
> this...

Removed.

> >  /* :::: Filter Buttons :::: */
> > +#quick-filter-bar {
> > +  -moz-appearance: toolbar;
> > +}
> 
> Not sure we want this either, since on my theme, it looks a bit obnoxious.

Removed.
Comment 74 Mike Conley (:mconley) 2011-11-22 10:57:59 PST
Thanks for the feedback, rsx11m!

(In reply to rsx11m from comment #66)
> Some feedback on the Windows try-server builds:
> 
>  - with mail.tabs.autoHide set true, the quick-filter toggle flushes to the
> left,
>    that's a bit odd as it's on the right end when at least one tab is shown;

Good catch, fixed.
  
>  - when dragging the buttons from the main toolbar to the menu bar to
> emulate the
>    previous point, they retain "icons above text" if that was the setting
> for the
>    main toolbar even if switched to "icons beside text".

Another good catch - fixed in my latest patch.
Comment 75 Mike Conley (:mconley) 2011-11-22 11:50:29 PST
Created attachment 576218 [details] [diff] [review]
Patch v6

Fixes a small Persona theme-ing bug that caused the tabs to overlap the mail toolbar by 1 pixel.
Comment 76 Andreas Nilsson (:andreasn) 2011-11-24 09:07:59 PST
Created attachment 576783 [details] [diff] [review]
tiny update

Same as v6, but adds a line between the toolbar and the panes.
Comment 77 Andreas Nilsson (:andreasn) 2011-11-24 09:08:45 PST
above also takes care of a bitrot
Comment 78 Andreas Nilsson (:andreasn) 2011-11-25 07:57:30 PST
Created attachment 576938 [details] [diff] [review]
tiny bitrot fix

Lets see if the size goes bananas on this one as well. Only fixes a bitrot this time around.
Comment 79 Andreas Nilsson (:andreasn) 2011-11-25 08:23:01 PST
Created attachment 576945 [details] [diff] [review]
trying again

Fixes bitrot and adds a line under the toolbar. Size of patch probably shrinks this time too, no idea why...
Comment 80 Andreas Nilsson (:andreasn) 2011-11-25 08:57:29 PST
Created attachment 576951 [details] [diff] [review]
Fixes some errors in the last patch

Same as before, but takes care of some issues that came up in the last patch I posted (toolbar in message header got a line below it as well)
Comment 81 Richard Marti (:Paenglab) 2011-11-25 11:52:08 PST
(In reply to Andreas Nilsson (:andreasn) from comment #79)
> Created attachment 576945 [details] [diff] [review] [diff] [details] [review]
> trying again
> 
> Fixes bitrot and adds a line under the toolbar. Size of patch probably
> shrinks this time too, no idea why...

I would say this is because mconley uses 8 lines before and after the diff and you are using 3 lines.
Comment 82 Mike Conley (:mconley) 2011-11-28 06:45:07 PST
Paenglab:

I think you're right on the money there - thanks for assuaging my fears. :)

-Mike
Comment 83 Mike Conley (:mconley) 2011-11-28 13:58:44 PST
Created attachment 577364 [details] [diff] [review]
Patch v8

This patch fixes the look of the menu in Linux, now that it's back above the tab selector.
Comment 84 Mike Conley (:mconley) 2011-11-28 14:00:48 PST
I've got a tabs-on-top build with this patch building on try right now - installers should be available in a few hours at:  http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/mconley@mozilla.com-1b6eedda6ea5
Comment 85 Andreas Nilsson (:andreasn) 2011-11-29 09:21:36 PST
Created attachment 577648 [details] [diff] [review]
style fixes for Win7

This small patch takes care of tab appearance on Win7 so the color change between the tab and the toolbar looks smooth.
Comment 86 Joe Sabash [:JoeS1] 2011-11-29 16:33:01 PST
(In reply to Mike Conley (:mconley) from comment #84)
> I've got a tabs-on-top build with this patch building on try right now -
> installers should be available in a few hours at: 
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/
> mconley@mozilla.com-1b6eedda6ea5

Used the try-build for an hour or so..no problems Win2k, and Winxp.
I think anyone you uses tabs will be happy with this.
Didn't bust any extensions for me and comes very close to a true "Full screen" presentation, something I've personally wanted for a long time.
Comment 87 Mihovil Stanic [:Mikeyy - L10n HR] 2011-11-29 22:20:12 PST
Sorry if this was asked before, but I skimmed though about 20 comments, used search and nothing appeared.

I'm looking at pictures you provided and I don't see Thunderbird button instead of menu bar on any of them.
Firefox has it in topleft corner, orange Firefox button. Do you plan to implement it later on or there are no plans for it?

I'm asking becaused that frees up 1 more row of space, since tabs are placed next to it on my XP FF.
Comment 88 Charles 2011-11-30 04:08:00 PST
(In reply to Mikeyy from comment #87)
> Sorry if this was asked before, but I skimmed though about 20 comments, used
> search and nothing appeared.
> 
> I'm looking at pictures you provided and I don't see Thunderbird button
> instead of menu bar on any of them.

That is a separate/different bug - bug 650170...
Comment 89 Andreas Nilsson (:andreasn) 2011-11-30 07:48:42 PST
Created attachment 577959 [details] [diff] [review]
Style fixes for Linux and Win7

Same as previous patch, but this time with Linux tab fixes to make the toolbar and tabs look more similar to Firefox.
Pinstripe tab fixes coming up, but those ended up being quite complicated to port, so that is still work-in-progress.
Comment 90 Mike Conley (:mconley) 2011-11-30 14:03:50 PST
Created attachment 578077 [details] [diff] [review]
1px separator between tab selector and content in contentTabs

Some work I did today as requested by bwinton - we're going to put a 1px separator between the tab selector and contentTab content.  I'm using a toolbox/toolbar to do it because in the not-so-distant future, we'll probably want some back/forth buttons for contentTabs.

I still need to work this into Linux / OSX - which I'll do tomorrow.
Comment 91 Mike Conley (:mconley) 2011-12-01 13:52:26 PST
Created attachment 578378 [details] [diff] [review]
1px toolbar in content tabs, now invisible in about:addons, and in Linux

Alright, so we don't want the 1px separator appearing in certain in-content dialogs, like the addons:manager.  This patch does an imitation of what Firefox does - it attaches a listener to the browser, checks the location of the page against a whitelist, and if it's in the whitelist, marks the root node with the attribute "disablechrome".

After some CSS magic, voila, the toolbar is invisible for pages in that whitelist.

I've also added CSS support for this toolbar in Linux.  Next stop, OSX.

squib:  just wanted to get another dev's feedback on this code.  Is this patch relatively sane?  Or do you have any suggestions?
Comment 92 Jim Porter (:squib) 2011-12-01 17:07:05 PST
Comment on attachment 578378 [details] [diff] [review]
1px toolbar in content tabs, now invisible in about:addons, and in Linux

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

Overall, this seems like the right thing to do, modulo one small comment below.

::: mail/base/content/specialTabs.js
@@ +331,5 @@
>  
> +  hideChromeForLocation: function hideChromeForLocation(aLocation) {
> +    return this.inContentWhitelist.some(function(aSpec) {
> +      return aSpec == aLocation
> +    });

You could just use indexOf here.
Comment 93 Eric Ding 2011-12-01 17:18:57 PST
(In reply to Mike Conley (:mconley) from comment #37 in Bug 666227)

> This is, actually, kind of by design - the toolbars in Ubuntu's default
> theme (Ambiance) are supposed to be dark.  However, since the toolbar in the
> mail window is within the tab, and the tab background is that lighter color,
> it made sense to inherit that lighter color.  We ran this past John Lea from
> the Canonical design team, and he concurred that this was probably the right
> way to go about it.

Mike, since you asked me to comment in bug 644169, I'll do so here, though it breaks up the flow of conversation a bit.  Here's my basic response: yes, the toolbars in Ubuntu's default theme are dark.  But that's not the case for every theme.  So I think you're still misreading me: I'm not critical of the lighter color for toolbars under tabs.  Rather, I'd say the problem is that the TB theme is overriding the system theme's settings for toolbars that are *not* under tabs, giving them the same background as menubars even if that's not what the system theme specifies.

Unless TB is going to insist that users cannot have separate Compose windows anymore (which I think would be a mistake), then the Compose toolbar needs to follow the system theme.  The fact that Ubuntu's default theme has the same background for both menu bar and toolbar just means that it hides this bug in your theme.

Am I making any more sense?  What do you think?

Eric
Comment 94 Mike Conley (:mconley) 2011-12-02 07:08:51 PST
Eric:

I'm sorry, I misunderstood you again - I thought you had found or were concerned about a bug in the tabs on top build I did, which is why I asked you to move the conversation here.

Andreas, you're far more familiar with Ubuntu themes than I am.  Regarding toolbars that are not under tabs, do you understand the problem that Eric is referring to?

-Mike
Comment 95 Mike Conley (:mconley) 2011-12-02 07:41:01 PST
Created attachment 578579 [details] [diff] [review]
1px toolbar seperator in content tabs on all platforms, invisible in about:addons

Thanks for the feedback squib - and I've fixed the problem you found in this patch.

I've also copied the styling over for pinstripe, so we now have the 1px separator in all 3 platforms.  Still need it for XP though - that's next.
Comment 96 Mike Conley (:mconley) 2011-12-02 11:41:32 PST
Hm - just noticed that Ctrl-tabbing through tabs is broken by this patch.  Investigating...
Comment 97 Mike Conley (:mconley) 2011-12-05 08:31:50 PST
Created attachment 579076 [details] [diff] [review]
Patch v9

This patch merges in Andrea's fix, the 1px separator patch, and fixes tabbing forward and back via Ctrl-Tab.
Comment 98 Richard Marti (:Paenglab) 2011-12-06 12:42:40 PST
Created attachment 579417 [details] [diff] [review]
Windows XP additional patch

Patch for Win XP using the same Tabs as Aero.

I fixed also on Aero the bottom border when in customize a additional toolbar is added.
Comment 99 Andreas Nilsson (:andreasn) 2011-12-06 17:40:36 PST
Created attachment 579551 [details] [diff] [review]
Patch v9 (minor update)

This is basically patch v9 with some rules added in messenger.css for toolbars on gnomestripe so that if you have several toolbars beneath the main toolbar, it they will behave as one toolbar (Firefox does this). This also works if you hide the main toolbar. Small catch that if you create two new toolbars and disable both the main toolbar and the first custom toolbar, the second custom toolbar won't get the nice gradient.
I'll do the same for qute tomorrow.
Comment 100 Richard Marti (:Paenglab) 2011-12-07 00:03:01 PST
Andreas, qute should be fixed with attachment 579417 [details] [diff] [review]. You can try this and modify if you like.

My different approach is I applied the gradient to #mail-toolbox and all toolbars are transparent. A downside could be that the gradient grows with the more toolbars you have. If this isn't desired we could give absolute values in the gradient definition.
Comment 101 Richard Marti (:Paenglab) 2011-12-07 10:29:39 PST
Created attachment 579738 [details] [diff] [review]
Windows XP additional patch

The same patch as from yesterday but on XP and Aero gradients with absolute values instead of percents. Additionally I changed the Aero theme to use the gradient on #mail-toolbox instead of #mail-bar3.
Comment 102 Mike Conley (:mconley) 2011-12-07 13:02:54 PST
Created attachment 579806 [details] [diff] [review]
Patch v10

This patch merges in Andreas' and Paenglab's fixes, and puts back styling for the content tab toolbox/toolbars on OSX that mysteriously didn't make it into Patch v9.
Comment 103 Andreas Nilsson (:andreasn) 2011-12-08 07:01:32 PST
Created attachment 580030 [details] [diff] [review]
pinstripe tab

Slightly rough pinstripe tab patch (some personas issues left still).
Comment 104 Philipp Kewisch [:Fallen] 2011-12-08 07:09:50 PST
Since with these changes most custom tabs will need their own toolbar, could you make sure that the css rules are general enough so that extensions can make use of them without the need for copying everything? This applies mostly to rules in primaryToolbar.css (possibly also unchanged rules).

One candidate would be:
http://mxr.mozilla.org/comm-central/source/mail/themes/pinstripe/mail/primaryToolbar.css#51
Comment 105 Andreas Nilsson (:andreasn) 2011-12-08 08:43:35 PST
Created attachment 580061 [details] [diff] [review]
slight update to previous patch

Still lots of junk lingering, but takes care of the inactive window state and is slightly nicer to Personas.
Comment 106 Andreas Nilsson (:andreasn) 2011-12-09 05:28:51 PST
Created attachment 580380 [details] [diff] [review]
pinstripe tabs v3

Works on both regular and personas as it should now!
Mike reported a odd square on the left to the first tab, but I've been unable to reproduce that.
The selected tab still have a somewhat heavy shadow, and I think I have an idea on how to fix that.
Comment 107 Mike Conley (:mconley) 2011-12-09 10:56:33 PST
Andreas:

Hey - that last patch won't build for me.  JarMaker is complaining that a file called "tab-top-selected-active.png" is missing.  Did you forget to include it in the diff?

-Mike
Comment 108 Andreas Nilsson (:andreasn) 2011-12-10 09:59:52 PST
(In reply to Mike Conley (:mconley) from comment #107)
> Andreas:
> 
> Hey - that last patch won't build for me.  JarMaker is complaining that a
> file called "tab-top-selected-active.png" is missing.  Did you forget to
> include it in the diff?
> 
> -Mike

No, I forgot to remove it from jar.nm actually. New patch coming up shortly, as I think I might have a fix to one of the other remaining issues as well.
Comment 109 Richard Marti (:Paenglab) 2011-12-10 10:21:02 PST
Andreas, when you are on it, could you also correct in tabmail-aero.css the line 150 from

background-image: -moz-linear-gradient(bottom, transparent 1px),

to

background-image: -moz-linear-gradient(bottom, transparent 1px, transparent),

The former line gives a warning in the error console and the rule isn't used.
Comment 110 Andreas Nilsson (:andreasn) 2011-12-10 11:52:08 PST
Created attachment 580664 [details] [diff] [review]
pinstripe tabs v3

Fixes the jar.nm issue (but despite about 2 hours of hacking, not the other one yet)
Comment 111 Andreas Nilsson (:andreasn) 2011-12-11 14:00:06 PST
Created attachment 580784 [details] [diff] [review]
pinstripe tabs v4

Addresses the issue with the dropmaker being cut off.
Comment 112 Mike Conley (:mconley) 2011-12-12 07:10:48 PST
Created attachment 580899 [details] [diff] [review]
Patch v11

Fixes up some bitrot from bug 680192.
Comment 113 Andreas Nilsson (:andreasn) 2011-12-12 08:41:09 PST
Created attachment 580922 [details]
pinstripe tabs v4(ish)

This is intended to be applied on top of Patch v11.
Comment 114 Mike Conley (:mconley) 2011-12-12 09:59:58 PST
Created attachment 580955 [details] [diff] [review]
Patch v12

Merging in Andreas's patch
Comment 115 Mike Conley (:mconley) 2011-12-12 10:32:27 PST
asuth:

CC'ing you because I think you're best equipped to review this code...is that an accurate assessment?

-Mike
Comment 116 Mike Conley (:mconley) 2011-12-12 11:08:55 PST
Comment on attachment 580955 [details] [diff] [review]
Patch v12

Blake:

Ok, you should have a few builds to play with and test now.  See what you think.

-Mike
Comment 117 Andrew Sutherland [:asuth] 2011-12-12 11:19:19 PST
Comment on attachment 580955 [details] [diff] [review]
Patch v12

Blake's review should suffice for code purposes too.  The only thing that looks potentially tricky implementation-wise is the customization logic, and I know that Blake has extensive experience with that.

If possible, it would be nice if you could throw in some minor comments in the XUL/XML.  For example, it seems like the mail toolbar now collapses under various circumstances, so maybe a short comment above its definition to that effect.  Similarly, the content tab has its dummy id's; I intuit from reading them that the DOM sub-tree is likely to be removed from the document and cloned on-demand with the id's being changed.  But a comment that says so would probably be more enlightening to anyone trying to read that code.
Comment 118 Mike Conley (:mconley) 2011-12-12 11:29:31 PST
Comment on attachment 580955 [details] [diff] [review]
Patch v12

Thanks asuth.

Cancelling ui-r / r requests - Paenglab and Fallen just pointed out a few last theme thing we need to fix.  Andreas is on that.
Comment 119 Andreas Nilsson (:andreasn) 2011-12-13 07:25:32 PST
Created attachment 581265 [details] [diff] [review]
patch v13 (wip)

Adds a class called .mail-toolbox to messenger.xul in order to fix the issue in comment 103. Only for gnomestripe for now, adding pinstripe and qute coming up.

This patch also fixes the issue in comment 109
Comment 120 Andreas Nilsson (:andreasn) 2011-12-13 08:28:06 PST
Created attachment 581284 [details] [diff] [review]
patch v13 (wip)

Same as previous patch, but with pinstripe fixes. Windows coming as soon as the build is done.
Comment 121 Andreas Nilsson (:andreasn) 2011-12-13 12:38:22 PST
Created attachment 581377 [details] [diff] [review]
patch v13

This takes care of the windows styling as well.
Comment 122 Mike Conley (:mconley) 2011-12-13 13:13:45 PST
Comment on attachment 581377 [details] [diff] [review]
patch v13

Let me know if you need an installer.
Comment 123 Mike Conley (:mconley) 2011-12-13 13:14:15 PST
Comment on attachment 581377 [details] [diff] [review]
patch v13

It'd probably help if I directed the ui-r to someone...
Comment 124 Mike Conley (:mconley) 2011-12-14 08:29:34 PST
Created attachment 581650 [details] [diff] [review]
Patch v13 - now with more context

Re-generated the patch with more context.
Comment 125 Blake Winton (:bwinton) (:☕️) 2011-12-14 11:36:28 PST
Comment on attachment 581650 [details] [diff] [review]
Patch v13 - now with more context

(Tested on Mac and Windows)

I mostly like it, but…

http://dl.dropbox.com/u/2301433/Screenshots/TabsOnTop/2-Mail%20Tab.png
http://dl.dropbox.com/u/2301433/Screenshots/TabsOnTop/4-Gloda.png
On Windows, the mail tab doesn't blend in well with the mail header.  I think we might want to change the mail header.

http://dl.dropbox.com/u/2301433/Screenshots/TabsOnTop/2-Mail%20Tab%20Mac.png
On the Mac, it's better.


http://dl.dropbox.com/u/2301433/Screenshots/TabsOnTop/3-OpenSearch.png
The icons on the side are really hard to read because of the transparent background.
http://dl.dropbox.com/u/2301433/Screenshots/TabsOnTop/3-OpenSearch%20Mac.png
Mac is better, but I feel we should do something with the personas to have them look less bad.  (That's also an issue on Windows.  Maybe we just make that sidebar not be transparent.)


http://dl.dropbox.com/u/2301433/Screenshots/TabsOnTop/5-Classic.png
Three Wolf Moon persona FTW!

http://dl.dropbox.com/u/2301433/Screenshots/TabsOnTop/1-First%20Screen%20Mac.png
We should also get rid of that extra shadow around the tab, but I think Andreas is already working on that.


Having said all that, this seems like an improvement, and I don't see any large problems with it, so I'ld be happy to land this and fix those issues in followup bugs.  ui-r=me!

Thanks,
Blake.
Comment 126 Jim Porter (:squib) 2011-12-14 11:41:15 PST
For the gloda screenshot, I think we could use a toolbar at the top of the gloda tab now that the gloda searchbox is only present (by default) in the 3pane tab. That would resolve the issue there.
Comment 127 Jim Porter (:squib) 2011-12-14 12:25:35 PST
Oh, one more thing: I notice the inactive tabs are translucent in Aero. Is this what Firefox is doing on nightlies? I worry that we'll get complaints from people who hate the translucency (especially if we're not consistent with Firefox).
Comment 128 Siddharth Agarwal [:sid0] (inactive) 2011-12-14 14:28:11 PST
Comment on attachment 581650 [details] [diff] [review]
Patch v13 - now with more context

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

I've reviewed everything from mailCore.js to tabmail.css. The rest comes later.

Per our discussion on IRC, the standalone message window customization code is broken. r- based on that.

::: mail/base/content/mailCore.js
@@ +261,5 @@
>  }
>  
>  function onViewToolbarCommand(aEvent, toolboxId)
>  {
> +  var toolbox = document.getElementById(toolboxId); 

Trailing whitespace.

@@ +270,5 @@
>                          "autohide" : "collapsed";
>  
>    toolbar.setAttribute(hidingAttribute,
>                         aEvent.originalTarget.getAttribute("checked") != "true");
> +  document.persist(toolbar.id, hidingAttribute); 

Trailing whitespace.

@@ +289,5 @@
>    }
>  
>    var firstMenuItem = popup.firstChild;
>  
> +  for (var [index, toolboxId] in Iterator(toolboxIds)) {

let, and you don't need index

@@ +290,5 @@
>  
>    var firstMenuItem = popup.firstChild;
>  
> +  for (var [index, toolboxId] in Iterator(toolboxIds)) {
> +    var toolbox = document.getElementById(toolboxId);

let

@@ +294,5 @@
> +    var toolbox = document.getElementById(toolboxId);
> +
> +    // We don't want to create closures here - so we use let to lock the
> +    // value of toolboxId into boundToolboxId.
> +    let boundToolboxId = toolboxId;

The comment doesn't make sense to me. You lock the value of toolboxId because JavaScript sucks and doesn't do fresh let-bindings per iteration.

@@ +296,5 @@
> +    // We don't want to create closures here - so we use let to lock the
> +    // value of toolboxId into boundToolboxId.
> +    let boundToolboxId = toolboxId;
> +
> +    var onMenuItemCommand = function(aEvent) {

let

::: mail/base/content/mailWindowOverlay.xul
@@ +1154,5 @@
>  
>      <!-- View -->
>      <menu id="menu_View">
>        <menupopup id="menu_View_Popup" onpopupshowing="view_init();">
> +        <menu id="menu_Toolbars" onpopupshowing="onViewToolbarsPopupShowing(event, ['mail-toolbox', 'navigation-toolbox']);">

The call above has ['navigation-toolbox', 'mail-toolbox']. If the order matters, why? If it doesn't, please make it consistent.

::: mail/base/content/messenger.xul
@@ +66,5 @@
>  %customizeToolbarDTD;
>  <!ENTITY % textcontextDTD SYSTEM "chrome://global/locale/textcontext.dtd">
>  %textcontextDTD;
> +<!ENTITY % tabMailDTD SYSTEM "chrome://messenger/locale/tabmail.dtd" >
> + %tabMailDTD;

Incorrect indentation for %tabmailDTD, doesn't match the examples above it.

@@ +250,5 @@
>  <menupopup id="messageIdContext"/>
>  
> +<menupopup id="tabContextMenu"
> +           onpopupshowing="var tabmail = document.getElementById('tabmail');
> +                           return tabmail.onTabContextMenuShowing(document.popupNode);">

Now that you're touching this code, let's make this document.getElementById('tabmail').onTabContextMenuShowing(...); to match the oncommands below.

@@ +253,5 @@
> +           onpopupshowing="var tabmail = document.getElementById('tabmail');
> +                           return tabmail.onTabContextMenuShowing(document.popupNode);">
> +  <menuitem label="&moveToNewWindow.label;"
> +                accesskey="&moveToNewWindow.accesskey;"
> +                id="openTabInWindow"

Please align the attributes here.

@@ +258,5 @@
> +                oncommand="document.getElementById('tabmail').replaceTabWithWindow(document.popupNode);"/>
> +  <menuseparator />
> +  <menuitem label="&closeOtherTabsCmd2.label;"
> +                accesskey="&closeOtherTabsCmd2.accesskey;"
> +                id="closeOtherTabs"

and here

@@ +263,5 @@
> +                oncommand="document.getElementById('tabmail').closeOtherTabs(document.popupNode);"/>
> +  <menuseparator />
> +  <menu label="&recentlyClosedTabsCmd.label;"
> +            accesskey="&recentlyClosedTabsCmd.accesskey;"
> +            id="recentlyClosedTabs" >

and here

@@ +269,5 @@
> +  </menu>
> +  <menuitem label="&closeTabCmd2.label;"
> +                accesskey="&closeTabCmd2.accesskey;"
> +                id="closeTab"
> +                oncommand="document.getElementById('tabmail').closeTab(document.popupNode);"/>

and here.

@@ +334,5 @@
>               (respectively, messagepanebox and threadpane-splitter).  This gives us
>               the folder pane next to the thread view, with the message pane/reader
>               beneath both of them. -->
>          <box id="mailContent" orient="vertical" flex="1">
> +          <toolbox id="mail-toolbox"

Per IRC discussion, this should live in mailWindowOverlay.js.

::: mail/base/content/msgHdrViewOverlay.xul
@@ +596,5 @@
>              </menupopup>
>            </toolbarbutton>
>          </toolbaritem>
>        </toolbarpalette>
> +      <toolbar id="attachment-view-toolbar" class="inline-toolbar"

Why this change?

::: mail/base/content/specialTabs.js
@@ +331,5 @@
>  
> +  hideChromeForLocation: function hideChromeForLocation(aLocation) {
> +    return this.inContentWhitelist.some(function(aSpec) {
> +      return aSpec == aLocation
> +    });

function (aSpec) (aSpec == aLocation)
Comment 129 Mike Conley (:mconley) 2011-12-15 08:17:58 PST
(In reply to Siddharth Agarwal [:sid0] from comment #128)
> Comment on attachment 581650 [details] [diff] [review]
> Patch v13 - now with more context
> 
> Review of attachment 581650 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

Thanks Sid - I've started to tackle the defects you've found so far.  

> 
> ::: mail/base/content/msgHdrViewOverlay.xul
> @@ +596,5 @@
> >              </menupopup>
> >            </toolbarbutton>
> >          </toolbaritem>
> >        </toolbarpalette>
> > +      <toolbar id="attachment-view-toolbar" class="inline-toolbar"
> 
> Why this change?
> 

This was introduced by Paenglab to fix a themeing issue - see https://bugzilla.mozilla.org/show_bug.cgi?id=644169#c59
Comment 130 Mike Conley (:mconley) 2011-12-15 13:45:27 PST
Created attachment 582099 [details] [diff] [review]
Patch v14

Alright - so here's what we've got:

We have a mail-toolbox that's defined in the mailWindowOverlay.  We have toolbars in the 3pane and the message window that link into that toolbox externally via toolboxid.  This allows us to customize all of these toolbars simultaneously with a shared palette.

It also puts a mail toolbar back in the message window.

Along with the changes in the XUL, I had to change a bunch of things in mailCore.js in order to accommodate for this.

Seems to work alright - though we've got a couple of small theme-ing issues now.  I think we're going to tackle those in separate bugs though.
Comment 131 Mike Conley (:mconley) 2011-12-15 14:03:28 PST
Created attachment 582104 [details] [diff] [review]
Patch v14

Bugzilla seems to have eaten my last patch.  Trying again...
Comment 132 Mike Conley (:mconley) 2011-12-15 14:06:30 PST
Comment on attachment 582104 [details] [diff] [review]
Patch v14

Well, you can still view the diff through Bugzilla.  Retrieving the raw patch seems a bit more difficult.

Hopefully Bugzilla will be feeling better tomorrow.
Comment 133 Mike Conley (:mconley) 2011-12-16 07:21:24 PST
Comment on attachment 582104 [details] [diff] [review]
Patch v14

Ack - a problem with this patch just revealed itself on Windows.  Cancelling review request until I can resolve.
Comment 134 Mike Conley (:mconley) 2011-12-16 09:46:12 PST
Created attachment 582303 [details] [diff] [review]
Patch v15

Alright, so this patch exposed what I believe to be a XUL bug (see bug 711508, which I've provided a patch for).

But yes, I think this ready for review now.  Finally.
Comment 135 Siddharth Agarwal [:sid0] (inactive) 2011-12-16 15:47:37 PST
Comment on attachment 582303 [details] [diff] [review]
Patch v15

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

I really have no idea how to review the CSS, heh. I've only reviewed it using https://developer.mozilla.org/en/Writing_Efficient_CSS and based on the fact that it got ui-r+.

This is great work. r- mainly because this patch has one major change suggested.

::: mail/base/content/mailWindowOverlay.xul
@@ +954,1 @@
>    <!-- Menu -->

Please leave a comment here explaining
- why the toolbar exists inside a toolbox in the first place (align)
- why the toolbar exists inside the navigation-toolbox and not in a dummy toolbox (overlays).

::: mail/base/content/messageWindow.js
@@ +1201,5 @@
>  }
>  
>  function getMailToolbox ()
>  {
> +  return document.getElementById("navigation-toolbox");

I think this should still return mail-toolbox.

::: mail/base/content/messenger.xul
@@ +248,5 @@
>  </menupopup>
>  
>  <menupopup id="messageIdContext"/>
>  
> +<menupopup id="tabContextMenu"

Per our IRC discussion, moving this out of tabmail might not be necessary. Let's try moving this back and reverting most of the changes to tabmail.xml.

::: mail/base/content/tabmail.xml
@@ +1295,3 @@
>              // by default "close other tabs" is disabled...
> +            document.getElementById("closeOtherTabs")
> +                    .setAttribute("disabled","true")

Semicolon here. (This doesn't need to be done if the change to this line is reverted.)

@@ +1301,3 @@
>                if (this.tabInfo[i].canClose && this.tabInfo[i] != tab) {
> +                document.getElementById("closeOtherTabs")
> +                        .setAttribute("disabled", "false")

And here. (This too.)

@@ +1453,5 @@
>    </binding>
>  
>    <binding id="tabmail-tab" display="xul:box"
>             extends="chrome://global/content/bindings/tabbox.xml#tab">
> +    <content closetabtext="&closeTab.label;" context="tabContextMenu">

As I said on IRC, we should probably be able to rely on the context event bubbling up to tabmail-strip.

@@ +2050,4 @@
>          if (dt.mozItemCount == 0)
>            return;
> +
> +        if (dt.mozGetDataAt("text/toolbarwrapper-id/messengerWindow", 0)) {

This returns a string, right? Could you check != null here?

@@ +2352,5 @@
>  
>        <method name="_menuItemOnCommand">
>          <parameter name="aEvent"/>
>          <body><![CDATA[
> +          var tabcontainer = document.getElementById('tabcontainer');

- You know what, let's access it through document.getElementById("tabmail").tabContainer. Thus if it changes again in the future then at least we won't have to change this everywhere. All problems in computer science can be solved by another level of indirection, etc.

- There's a doc comment for the binding which says that "[t]his binding relies on the structure of the tabbrowser binding. Therefore it should only be used as a child of the tabs element." Given that I think we've gotten rid of all the getBindingParent calls, is this comment valid any longer?

@@ +2415,5 @@
>        </method>
>  
>        <method name="_updateTabsVisibilityStatus">
>          <body><![CDATA[
> +          var tabContainer = document.getElementById(this.getAttribute("tabcontainer"));

document.getElementById("tabmail").tabContainer

@@ +2473,4 @@
>          var tabs = tabcontainer.childNodes;
>  
>          // Listen for changes in the tab bar.
> +        tabcontainer.tabmail.addEventListener("TabOpen", this, false);

var tabmail = document.getElementById("tabmail");
var tabcontainer = tabmail.tabContainer;

...

tabmail.addEventListener("TabOpen", ...);

@@ +2496,5 @@
>            menuItem.tab.removeEventListener("TabClose", this, false);
>            menuItem.tab.mCorrespondingMenuitem = null;
>            this.removeChild(menuItem);
>          }
> +        var tabcontainer = document.getElementById('tabcontainer');

document.getElementById("tabmail").tabContainer

::: mail/themes/pinstripe/mail/primaryToolbar.css
@@ +132,5 @@
>  toolbar[mode="text"] .toolbarbutton-menubutton-button > .toolbarbutton-text {
>    margin: 5px 4px 3px;
>  }
>  
> +#tabbar-toolbar .toolbarbutton-1 {

Descendant selector. Can we avoid this and use #tabbar-toolbar > .toolbarbutton-1 instead? And the same goes for the two instances right above, though a different bug would be fine for them.

(One thing I noticed while looking at this was that qfb-show-filter-bar doesn't have the toolbarbutton-1 class set on it. Do you know why that is so?)

@@ +797,5 @@
>  toolbar:-moz-lwtheme {
>    -moz-appearance: none !important;
>    background: none !important;
>  }
> +*/

Please get rid of this.

::: mail/themes/qute/mail/mailWindow1-aero.css
@@ +258,5 @@
>      border-top: none;
>      background-clip: padding-box;
>    }
>  
> +  #messengerWindow[sizemode=normal] #mail-toolbar-menubar2 {

I guess this can't be avoided, huh?

::: mail/themes/qute/mail/messageHeader-aero.css
@@ +92,5 @@
>    background-color: transparent;
>  }
>  
>  /* toolbars are optimized to look like OS-provided widgets; override this */
> +.main-header-area .inline-toolbar {

What about this?

::: mail/themes/qute/mail/messageHeader.css
@@ +94,5 @@
>    background-color: transparent;
>  }
>  
>  /* toolbars are optimized to look like OS-provided widgets; override this */
> +.main-header-area .inline-toolbar {

And this?

::: mail/themes/qute/mail/primaryToolbar.css
@@ +45,5 @@
>     ====================================================================== */
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
> +#navigation-toolbox > #tabs-toolbar {

What's the point of checking that an ID'd element is the child of another ID'd element?

@@ +50,5 @@
> +  -moz-appearance: none;
> +  border-bottom: 1px solid ThreeDShadow;
> +}
> +
> +#navigation-toolbox > #tabs-toolbar:-moz-lwtheme {

And here?
Comment 136 Siddharth Agarwal [:sid0] (inactive) 2011-12-16 15:49:52 PST
(I haven't reviewed the tests yet. I think I'll do so in the next iteration.)
Comment 137 Mike Conley (:mconley) 2011-12-17 08:54:06 PST
Created attachment 582545 [details] [diff] [review]
WIP Patch 16

Alright, I've moved the tabContextMenu out from the main document back into tabmail.xml.

There's a bit of a tradeoff though - due to the way that _child works, and limitations to how the arrowscrollbox operates, I cannot seem to get the toolbar context menu to appear when right-clicking on the empty tab space.  :/  Any ideas there would be really super helpful.
Comment 138 Siddharth Agarwal [:sid0] (inactive) 2011-12-17 08:59:41 PST
That's how it is right now too. Blake, thoughts?
Comment 139 Mike Conley (:mconley) 2011-12-17 12:01:36 PST
Created attachment 582571 [details] [diff] [review]
WIP Patch 17

Andreas noticed that custom toolbars weren't being hidden by the previous patch when in a tab other than the 3pane.  This occurred when we moved the mail-3bar outside of the mail-toolbox.  This patch fixes that regression.
Comment 140 Mike Conley (:mconley) 2011-12-17 12:34:47 PST
Comment on attachment 582571 [details] [diff] [review]
WIP Patch 17

Blake:

So one of the consequences of this change is that new custom toolbars are placed *above* the mail toolbar.  Is that a dealbreaker?

-Mike
Comment 141 Mike Conley (:mconley) 2011-12-17 12:37:07 PST
Also, when adding *another* custom toolbar, it appears *below* the first custom toolbar.  So it feels a bit inconsistent.

Fixing this could get a bit hairy.
Comment 142 Blake Winton (:bwinton) (:☕️) 2011-12-17 14:25:16 PST
So, I'm not really happy with the combination of the odd custom behaviour of the toolbars, and the lack of a context menu in the empty space beside the tabs.

Is there another way to fix the problem, if we revert this change?
Comment 143 Mike Conley (:mconley) 2011-12-17 14:54:24 PST
The context menu can be put back if it's put back into messenger.xul, instead of it being a child of tabmail-tabs.  I put it back as per Sid's review, but it looks like we've broken that functionality.  Sid, is it OK with you if I put it back the way it was?

The odd custom toolbar behaviour is symptomatic of the way the toolbars have been divided up.  I'll look into some alternatives tonight and tomorrow.
Comment 144 Siddharth Agarwal [:sid0] (inactive) 2011-12-17 14:56:10 PST
(In reply to Mike Conley (:mconley) from comment #143)
> The context menu can be put back if it's put back into messenger.xul,
> instead of it being a child of tabmail-tabs.  I put it back as per Sid's
> review, but it looks like we've broken that functionality.  Sid, is it OK
> with you if I put it back the way it was?

Sure, so long as you add a comment explaining this.
Comment 145 Mike Conley (:mconley) 2011-12-17 16:16:49 PST
Alright, can do.

I think I've also found a solution to our toolbar customization problem to boot.

I'll post up a polished patch as early as possible tomorrow.

Andreas - sorry for the shifting sands.  :/
Comment 146 Blake Winton (:bwinton) (:☕️) 2011-12-18 08:49:27 PST
Comment on attachment 582571 [details] [diff] [review]
WIP Patch 17

(Setting ui-r- based on my comments.  Also, I haven't tested this, but because you can't get the context menu in the blank space beside the tab bar, I'm guessing that when you're customizing the area beside the tab bar, you can't drag stuff onto that area to have it show up to the right…)
Comment 147 Mike Conley (:mconley) 2011-12-18 13:14:06 PST
Created attachment 582704 [details] [diff] [review]
WIP Patch 18

Ok, so this patch does the following:

1)  Moves the mail-toolbox into mailWindowOverlay.xul, and adds hooks for it in messenger.xul and messageWindow.xul
2)  Moves the tabContextMenu back into the main document of messenger.xul, in order to have the mail-toolbox context menu work properly again when right-clicking on the tab strip (see the comment I put in about that).

I believe I've also addressed most of Sid's review comments.  This is still a WIP though, since there are a few comments from Sid that involve themeing and CSS stuff that I think Andreas would be best to answer / address.
Comment 148 Andreas Nilsson (:andreasn) 2011-12-19 08:15:02 PST
Created attachment 582832 [details] [diff] [review]
WIP patch 19

Takes care of the css fixes Sid commented on that's Mike hadn't fixed already.
Comment 149 Andreas Nilsson (:andreasn) 2011-12-19 08:30:18 PST
> ::: mail/themes/pinstripe/mail/primaryToolbar.css
> > +#tabbar-toolbar .toolbarbutton-1 {
> 
> Descendant selector. Can we avoid this and use #tabbar-toolbar >
> .toolbarbutton-1 instead? And the same goes for the two instances right
> above, though a different bug would be fine for them.

Fixed.


> ::: mail/themes/qute/mail/messageHeader-aero.css
> >  /* toolbars are optimized to look like OS-provided widgets; override this */
> > +.main-header-area .inline-toolbar {
> What about this?

> ::: mail/themes/qute/mail/messageHeader.css
> >  /* toolbars are optimized to look like OS-provided widgets; override this */
> > +.main-header-area .inline-toolbar {
> 
> And this?

Removing this made no change whatsoever. Removed


> ::: mail/themes/qute/mail/primaryToolbar.css
> > +#navigation-toolbox > #tabs-toolbar {
> 
> What's the point of checking that an ID'd element is the child of another
> ID'd element?

Fixed


> > +#navigation-toolbox > #tabs-toolbar:-moz-lwtheme {
> And here?

Fixed
Comment 150 Mike Conley (:mconley) 2011-12-19 08:45:11 PST
(In reply to Siddharth Agarwal [:sid0] from comment #135)
> Comment on attachment 582303 [details] [diff] [review]
> Patch v15
> 
> Review of attachment 582303 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the awesome reviews, Sid!

> 
> ::: mail/base/content/mailWindowOverlay.xul
> @@ +954,1 @@
> >    <!-- Menu -->
> 
> Please leave a comment here explaining
> - why the toolbar exists inside a toolbox in the first place (align)
> - why the toolbar exists inside the navigation-toolbox and not in a dummy
> toolbox (overlays).
> 

Done.

> ::: mail/base/content/messageWindow.js
> @@ +1201,5 @@
> >  }
> >  
> >  function getMailToolbox ()
> >  {
> > +  return document.getElementById("navigation-toolbox");
> 
> I think this should still return mail-toolbox.

Agreed - fixed.

> 
> ::: mail/base/content/messenger.xul
> @@ +248,5 @@
> >  </menupopup>
> >  
> >  <menupopup id="messageIdContext"/>
> >  
> > +<menupopup id="tabContextMenu"
> 
> Per our IRC discussion, moving this out of tabmail might not be necessary.
> Let's try moving this back and reverting most of the changes to tabmail.xml.

And per our discussion later on in the bug, I kept this change, but put in a comment detailing why we have tabContextMenu available in the main messenger.xul document.

> 
> ::: mail/base/content/tabmail.xml
> @@ +1295,3 @@
> >              // by default "close other tabs" is disabled...
> > +            document.getElementById("closeOtherTabs")
> > +                    .setAttribute("disabled","true")
> 
> Semicolon here. (This doesn't need to be done if the change to this line is
> reverted.)

Fixed.  Also, so as not to expose things like "closeTab" element ID's to the messenger.xul window, I've kept the anonid attributes of the menuitems under tabContextMenu, and I use querySelector to grab them.

> 
> @@ +1301,3 @@
> >                if (this.tabInfo[i].canClose && this.tabInfo[i] != tab) {
> > +                document.getElementById("closeOtherTabs")
> > +                        .setAttribute("disabled", "false")
> 
> And here. (This too.)

Fixed.

> 
> @@ +2050,4 @@
> >          if (dt.mozItemCount == 0)
> >            return;
> > +
> > +        if (dt.mozGetDataAt("text/toolbarwrapper-id/messengerWindow", 0)) {
> 
> This returns a string, right? Could you check != null here?

Done.

> 
> @@ +2352,5 @@
> >  
> >        <method name="_menuItemOnCommand">
> >          <parameter name="aEvent"/>
> >          <body><![CDATA[
> > +          var tabcontainer = document.getElementById('tabcontainer');
> 
> - You know what, let's access it through
> document.getElementById("tabmail").tabContainer. Thus if it changes again in
> the future then at least we won't have to change this everywhere. All
> problems in computer science can be solved by another level of indirection,
> etc.

Done.

> 
> - There's a doc comment for the binding which says that "[t]his binding
> relies on the structure of the tabbrowser binding. Therefore it should only
> be used as a child of the tabs element." Given that I think we've gotten rid
> of all the getBindingParent calls, is this comment valid any longer?

Fixed.

> 
> @@ +2415,5 @@
> >        </method>
> >  
> >        <method name="_updateTabsVisibilityStatus">
> >          <body><![CDATA[
> > +          var tabContainer = document.getElementById(this.getAttribute("tabcontainer"));
> document.getElementById("tabmail").tabContainer

Fixed.

> 
> @@ +2473,4 @@
> >          var tabs = tabcontainer.childNodes;
> >  
> >          // Listen for changes in the tab bar.
> > +        tabcontainer.tabmail.addEventListener("TabOpen", this, false);
> 
> var tabmail = document.getElementById("tabmail");
> var tabcontainer = tabmail.tabContainer;
> 
> ...
> 
> tabmail.addEventListener("TabOpen", ...);
> 

Fixed.

> @@ +2496,5 @@
> >            menuItem.tab.removeEventListener("TabClose", this, false);
> >            menuItem.tab.mCorrespondingMenuitem = null;
> >            this.removeChild(menuItem);
> >          }
> > +        var tabcontainer = document.getElementById('tabcontainer');
> 
> document.getElementById("tabmail").tabContainer
> 

Fixed.

> 
> @@ +797,5 @@
> >  toolbar:-moz-lwtheme {
> >    -moz-appearance: none !important;
> >    background: none !important;
> >  }
> > +*/
> 
> Please get rid of this.

Done.
Comment 151 Mike Conley (:mconley) 2011-12-19 08:49:51 PST
Created attachment 582842 [details] [diff] [review]
Patch v19

Hey Sid, I think we're ready for another review iteration.  Have at it!

Thanks,

-Mike
Comment 152 Mike Conley (:mconley) 2011-12-19 09:05:08 PST
Comment on attachment 582842 [details] [diff] [review]
Patch v19

As Sid pointed out in IRC, we're re-organizing some pretty long-standing XUL structures here...so, requesting sr.
Comment 153 Siddharth Agarwal [:sid0] (inactive) 2011-12-20 04:21:55 PST
Comment on attachment 582842 [details] [diff] [review]
Patch v19

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

Looks good for the most part. r- because of a test that I think will fail on Mac.

::: mail/base/content/mailCore.js
@@ +269,3 @@
>  
>    // Empty the menu
> +  for (let i = popup.childNodes.length-1; i >= 0; --i) {

Now that you're touching this, it'd be great if you put spaces around the minus sign.

::: mail/test/mozmill/tabmail/test-tabmail-customize.js
@@ +55,5 @@
> +}
> +
> +/* Test that we can access the customize context menu by right
> + * clicking on the tabs toolbar.
> + */

This should be a doc comment. A doc comment looks like

/**
 * Test that...
 * ...
 */

@@ +58,5 @@
> + * clicking on the tabs toolbar.
> + */
> +function test_open_context_menu() {
> +  // First, ensure that the context menu is closed.
> +  let contextPopup = mc.eid('toolbar-context-menu').getNode();

mc.e('toolbar-context-menu')?

@@ +70,5 @@
> +
> +/* Test that, when customizing the toolbars, if the user drags an item onto
> + * the tab bar, they're redirected to the toolbar directly to the right of
> + * the tab bar.
> + */

This too.

@@ +73,5 @@
> + * the tab bar.
> + */
> +function test_redirects_toolbarbutton_drops() {
> +  let tabbar = mc.eid("tabcontainer").getNode();
> +  let toolbar = mc.eid("tabbar-toolbar").getNode();

mc.e?

@@ +79,5 @@
> +  // First, let's open up the customize toolbar window.
> +  mc.rightClick(mc.eid("tabcontainer"));
> +  mc.click(mc.eid("CustomizeMailToolbar"));
> +
> +  let ctw = wait_for_existing_window("CustomizeToolbarWindow");

Customize toolbar pulls down a sheet on Mac. I suspect this isn't going to work there. You'll need to do something like https://mxr.mozilla.org/comm-central/source/mail/test/mozmill/message-header/test-header-toolbar.js#493.

@@ +88,5 @@
> +   "wrapper-button-replylist",
> +   "wrapper-button-forward",
> +   "wrapper-button-archive",
> +  ].forEach(function(aButtonId) {
> +    let button = ctw.eid(aButtonId).getNode();

ctw.e?

@@ +109,5 @@
> +   "button-newmsg",
> +   "button-address",
> +   "button-tag",
> +  ].forEach(function(aButtonId) {
> +    let button = mc.eid(aButtonId).getNode();

mc.e?

@@ +115,5 @@
> +    let dt = synthesize_drag_start(mc.window, button, mc.window);
> +    assert_true(dt, "Drag target was undefined");
> +
> +    synthesize_drag_over(mc.window, tabbar, dt);
> +    synthesize_drop(mc.window, tabbar, dt);

I think you should either reverse all these changes that you make in a teardownModule function, or move this test out into a separate directory so that it doesn't affect other tests.

::: mail/test/mozmill/tabmail/test-tabmail-dragndrop.js
@@ +157,1 @@
>                "Moving tab1 failed");

Alignment issue.
Comment 154 Siddharth Agarwal [:sid0] (inactive) 2011-12-20 04:25:07 PST
(In reply to Siddharth Agarwal [:sid0] from comment #153)
> I think you should either reverse all these changes that you make in a
> teardownModule function, or move this test out into a separate directory so
> that it doesn't affect other tests.

Oh sorry, it's already in a separate directory. Could you add a readme file to the directory saying that no other tests should be in here -- also I think the name of the directory's too generic and that other tests could easily make their way in there. Let's call it tabmail-dragndrop.
Comment 155 Siddharth Agarwal [:sid0] (inactive) 2011-12-20 05:03:47 PST
I'm running with the patch right now, and things I can notice (! indicates things that count towards r-):

! The folder pane gets hidden at every startup with my usual profile, even in safe mode. This shouldn't happen. I'm looking into this...
- The tabs look pretty different (on Win7/Aero) from Firefox tabs. They seem to be much more transparent when focused, leading to poorer visibility on darker backgrounds.
- The throbber, if on a tab, is too close to its edge.
Comment 156 Siddharth Agarwal [:sid0] (inactive) 2011-12-20 05:06:10 PST
(In reply to Siddharth Agarwal [:sid0] from comment #154)
> 
> Oh sorry, it's already in a separate directory. Could you add a readme file
> to the directory saying that no other tests should be in here -- also I
> think the name of the directory's too generic and that other tests could
> easily make their way in there. Let's call it tabmail-dragndrop.

Oops, sorry, I got confused here. What I'd like you to do is move test-tabmail-customize.js out to a directory called tabmail-customize, and add a readme to the directory explaining that no other files should be added to it.
Comment 157 Siddharth Agarwal [:sid0] (inactive) 2011-12-20 05:22:58 PST
(In reply to Siddharth Agarwal [:sid0] from comment #155) 
> ! The folder pane gets hidden at every startup with my usual profile, even
> in safe mode. This shouldn't happen. I'm looking into this...

Never mind, Test Pilot seems to be causing this.

Another thing I noticed: with the Add-ons Manager open, View -> Toolbars displays http://grab.by/br1G. Note the empty space below Status Bar, which would be Quick Filter in a 3pane folder tab.
Comment 158 Mike Conley (:mconley) 2011-12-20 07:05:59 PST
Created attachment 583153 [details] [diff] [review]
Patch v20

> Review of attachment 582842 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------

>::: mail/base/content/mailCore.js
>@@ +269,3 @@
>>  
>>    // Empty the menu
>> +  for (let i = popup.childNodes.length-1; i >= 0; --i) {
>
> Now that you're touching this, it'd be great if you put spaces around the minus sign.

Done.

> ::: mail/test/mozmill/tabmail/test-tabmail-customize.js
> @@ +55,5 @@
>> +}
>> +
>> +/* Test that we can access the customize context menu by right
>> + * clicking on the tabs toolbar.
>> + */
>
> This should be a doc comment.

Done.


>@@ +58,5 @@
>> + * clicking on the tabs toolbar.
>> + */
>> +function test_open_context_menu() {
>> +  // First, ensure that the context menu is closed.
>> +  let contextPopup = mc.eid('toolbar-context-menu').getNode();
>
> mc.e('toolbar-context-menu')?

Done.

>@@ +70,5 @@
>> +
>> +/* Test that, when customizing the toolbars, if the user drags an item onto
>> + * the tab bar, they're redirected to the toolbar directly to the right of
>> + * the tab bar.
>> + */
>
> This too.

Done.

>@@ +73,5 @@
>> + * the tab bar.
>> + */
>> +function test_redirects_toolbarbutton_drops() {
>> +  let tabbar = mc.eid("tabcontainer").getNode();
>> +  let toolbar = mc.eid("tabbar-toolbar").getNode();
>
> mc.e?

Done.

>@@ +79,5 @@
>> +  // First, let's open up the customize toolbar window.
>> +  mc.rightClick(mc.eid("tabcontainer"));
>> +  mc.click(mc.eid("CustomizeMailToolbar"));
>> +
>> +  let ctw = wait_for_existing_window("CustomizeToolbarWindow");

> Customize toolbar pulls down a sheet on Mac. I suspect this isn't going to work there. You'll need to do something like https://mxr.mozilla.org/comm-central/source/mail/test/mozmill/message-header/test-header-toolbar.js#493.

Done - testing on OSX now.

> @@ +88,5 @@
>> +   "wrapper-button-replylist",
>> +   "wrapper-button-forward",
>> +   "wrapper-button-archive",
>> +  ].forEach(function(aButtonId) {
>> +    let button = ctw.eid(aButtonId).getNode();
>
> ctw.e?

Done

> @@ +109,5 @@
>> +   "button-newmsg",
>> +   "button-address",
>> +   "button-tag",
>> +  ].forEach(function(aButtonId) {
>> +    let button = mc.eid(aButtonId).getNode();
>
> mc.e?

Done.

> @@ +115,5 @@
>> +    let dt = synthesize_drag_start(mc.window, button, mc.window);
>> +    assert_true(dt, "Drag target was undefined");
>> +
>> +    synthesize_drag_over(mc.window, tabbar, dt);
>> +    synthesize_drop(mc.window, tabbar, dt);
>
>I think you should either reverse all these changes that you make in a teardownModule function, or move this test out into a separate directory so that it doesn't affect other tests.

I've used a teardownModule function.

> ::: mail/test/mozmill/tabmail/test-tabmail-dragndrop.js
> @@ +157,1 @@
>>                "Moving tab1 failed");
>
> Alignment issue.

Fixed.
Comment 159 Mike Conley (:mconley) 2011-12-20 07:45:31 PST
Created attachment 583161 [details] [diff] [review]
Patch v21

Alright, just tested this on OSX, and the tabmail tests pass with this patch.
Comment 160 Mike Conley (:mconley) 2011-12-20 08:46:02 PST
Comment on attachment 583161 [details] [diff] [review]
Patch v21

As per Sid's advisement, I'm re-requesting superreview, since - as stated before - this patch re-orders some XUL nodes that have been pretty stable for the past few years.
Comment 161 Siddharth Agarwal [:sid0] (inactive) 2011-12-20 11:12:49 PST
Comment on attachment 583161 [details] [diff] [review]
Patch v21

OK, I think the patch is good enough to land, modulo figuring out the RDF persistence issue.
Comment 162 Mike Conley (:mconley) 2011-12-20 14:26:42 PST
Checked in to comm-central as http://hg.mozilla.org/comm-central/rev/3f507b7d7ee6

\o/
Comment 163 Jim Porter (:squib) 2011-12-21 00:04:54 PST
Comment on attachment 583161 [details] [diff] [review]
Patch v21

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

While looking through this to try to solve the ctrl+tab bug, I came across the following change, which I'm not sure about...

::: mail/base/content/mailWindowOverlay.xul
@@ +1852,5 @@
>                          class="toolbarbutton-1 delete-button"
>                          label="&undeleteButton.label;"
>                          tooltiptext="&undeleteButton.tooltip;"
>                          observes="button_delete"
> +                        oncommand="goDoCommand('cmd_delete')"/>

Any reason for this change?
Comment 164 Mark Banner (:standard8, afk until Dec) 2011-12-21 00:37:28 PST
(In reply to Jim Porter (:squib) from comment #163)
> ::: mail/base/content/mailWindowOverlay.xul
> @@ +1852,5 @@
> >                          class="toolbarbutton-1 delete-button"
> >                          label="&undeleteButton.label;"
> >                          tooltiptext="&undeleteButton.tooltip;"
> >                          observes="button_delete"
> > +                        oncommand="goDoCommand('cmd_delete')"/>
> 
> Any reason for this change?

Well if the command wasn't activating, then the correct change would have been s/observes/command/ ... but that depends why it was no longer being activated in the first place.
Comment 165 Mike Conley (:mconley) 2011-12-21 06:34:11 PST
(In reply to Jim Porter (:squib) from comment #163)
> Comment on attachment 583161 [details] [diff] [review]
> Patch v21
> 
> Review of attachment 583161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> While looking through this to try to solve the ctrl+tab bug, I came across
> the following change, which I'm not sure about...
> 
> ::: mail/base/content/mailWindowOverlay.xul
> @@ +1852,5 @@
> >                          class="toolbarbutton-1 delete-button"
> >                          label="&undeleteButton.label;"
> >                          tooltiptext="&undeleteButton.tooltip;"
> >                          observes="button_delete"
> > +                        oncommand="goDoCommand('cmd_delete')"/>
> 
> Any reason for this change?

Wow, how did that slip in?  I don`t actually remember performing that change...

Investigating...
Comment 166 Mike Conley (:mconley) 2011-12-21 07:51:48 PST
Created attachment 583495 [details] [diff] [review]
Patch for accidental undelete command reassignment

squib:

So I looked my patch up and down, and I cannot figure out how that slipped in.  It certainly contributes nothing to tabs on top, and should be reverted.

This patch reverts the change.

-Mike
Comment 167 Mike Conley (:mconley) 2011-12-22 06:20:02 PST
We've changed the positioning of some pretty long-standing XUL nodes.  We'll almost certainly need to alert add-on developers.
Comment 168 Jim Porter (:squib) 2011-12-26 12:29:50 PST
Comment on attachment 583495 [details] [diff] [review]
Patch for accidental undelete command reassignment

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

Looks good to me! (Though I didn't run with the patch as I'm not near a compiler.)
Comment 169 Mike Conley (:mconley) 2011-12-28 06:50:12 PST
Comment on attachment 583495 [details] [diff] [review]
Patch for accidental undelete command reassignment

Requesting approval for Aurora
Comment 170 Mike Conley (:mconley) 2011-12-28 06:54:44 PST
Thanks squib, committed to comm-central as http://hg.mozilla.org/comm-central/rev/3b4cb1418917
Comment 171 Mike Conley (:mconley) 2011-12-29 07:25:14 PST
Attachment 583495 [details] [diff] committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/8d390f8116cf
Comment 172 :aceman 2012-01-04 00:37:08 PST
It seems the tabs can't be toggled to be below the toolbar.
Time to update https://wiki.mozilla.org/Features/Thunderbird/Tabs_On_Top which says they should be?
Comment 173 Mike Conley (:mconley) 2012-01-04 06:50:20 PST
aceman:

Thanks, done. :)

-Mike
Comment 174 Charles 2012-01-05 04:23:36 PST
I'm concerned about whether or not this will impact the ability of an extension author to do something they have expressed an interest in doing...

Blake/Mike, can you please answer the question below (after some qualifying comments)...

Yes, Firefox has moved the tabs to the top by default, *but* it does give the user the choice/ability to change this - choice is good, right?

I use three extensions in Firefox that are *critical* to my UX - Treestyle Tab gives me the tabs on the left side, and 'Movable Firefox Button' (to convert the huge button to a smaller one t hat I can move to any toolbar I want), and finally the 'Personal Titlebar' extension, that moves the Menubar up into the Window Titlebar.

If Thunderbird goes the route of *not* allowing the tabs to be moved below the menubar, will this *prevent* the Personal Titlebar extension author from adding support for Thunderbird (he is currently waiting for the 'Thunderbird App Button' to be implemented because he says it is necessary for it to work) to the 'Personal Titlebar' extension allowing me to put *everything* on the menubar, then have that moved to the Window Titlebar? This obviously would result in the tabs being *below* the menubar (which would then be up in the Window Titlebar)?

Thanks...
Comment 175 Mike Conley (:mconley) 2012-01-05 06:44:07 PST
Hey Charles,

Thanks for bringing up these concerns.

I should preface by mentioning that I have no inkling whatsoever of how the Treestyle Tab and Personal Titlebar add-ons that you've mentioned work.

What you're describing, however, *should* be possible.  The tab selector should be decoupled from the tab panels, meaning that an alternative tab selector could be swapped in.

Similarly, the menubar is not explicitly coupled to it's location in the tree.  I don't see why it shouldn't be possible for an add-on developer to move that menubar into the window titlebar.

Having the menubar appear above the tab selector is less about node placement in the tree, and more about CSS.  I assume you're using the Windows Aero Glass theme.  If you switch to Aero Basic, for example, you'll notice that the menubar has been placed above the tab selector.

But, again, I'm only guessing at how the Treestyle Tab and Personal Titlebar add-ons work, and how they'd like to work.

I hope that clears things up.  Let me know if you have further questions,

-Mike
Comment 176 Charles 2012-01-05 09:16:52 PST
Hi Mike,

Thanks for the reply...

Treestyle tab (but its for Firefox only) simply puts the tabs on the side (I use the left side)...

It is the Personal Titlebar addon that does the magic with moving the menubar into the window titlebar.

From what you said it sounds like this won't be a problem, so I'll just keep my fingers crossed that it isn't a big deal.

Thanks again!
Comment 177 Matthew Atkinson 2012-01-26 03:36:49 PST
Apologies if I've missed something here, but on my Earlybird running on Windows XP, my tabs aren't on top, they are under the Menu. I can attach a screenshot if it's useful. 

On my other computer which runs Windows Vista, it's switched round and the tabs are genuinely on the top. 

This is REALLY confusing, and presumably is not what's supposed to happen. 

I've skimmed through the comments here, and can't see anyone discussing this. Have I missed something, or is something genuinely wrong?
Comment 178 Mihovil Stanic [:Mikeyy - L10n HR] 2012-01-26 03:43:57 PST
Same here and tab background color is not same as opened e-mail background color on XP. You can see line between tab and email body.
On Win7 it's ok.
Comment 179 Mihovil Stanic [:Mikeyy - L10n HR] 2012-01-26 04:01:15 PST
Created attachment 591748 [details]
Border between tab and email body

Border between tab and email body is marked with arrow.
You can also see that tabs are not on top.

WinXP
Comment 180 Mike Conley (:mconley) 2012-01-26 06:38:09 PST
(In reply to Matthew Atkinson from comment #177)
> Apologies if I've missed something here, but on my Earlybird running on
> Windows XP, my tabs aren't on top, they are under the Menu. I can attach a
> screenshot if it's useful. 
> 

Hey Matthew, thanks for finding the bug and writing in!

> On my other computer which runs Windows Vista, it's switched round and the
> tabs are genuinely on the top. 
> 
> This is REALLY confusing, and presumably is not what's supposed to happen. 

Believe it or not, this *is* what's supposed to happen.  Native Windows applications from Microsoft are starting to adopt a UI of tabs above the menubar.  Pop open IE9+ and press Alt if you don't believe me.

On Windows XP, however, for native apps, the menu always appears above the tabs.

So what's happening here is that we're doing our best to blend in with other native apps on each operating system.  Thunderbird on XP gets the old menu placement, and Thunderbird on Vista or 7 with Aero glass enabled have the menu in the new placement.

> I've skimmed through the comments here, and can't see anyone discussing
> this. Have I missed something, or is something genuinely wrong?

Nope, it's all part of the plan. :)

-Mike
Comment 181 Mike Conley (:mconley) 2012-01-26 06:40:40 PST
(In reply to Mihovil Stanic from comment #178)
> Same here and tab background color is not same as opened e-mail background
> color on XP. You can see line between tab and email body.
> On Win7 it's ok.

Hey Mihovil:

See above for my response.  That discolouration you've noticed *is* a bug, and is being tracked in bug 710831, if you'd like to follow along.

All the best,

-Mike
Comment 182 Matthew Atkinson 2012-01-26 07:38:43 PST
Mike, thanks for your reply in #180.

I've had a look round my Windows computer here, and I'm struggling to find another application which even has tabs, to support your assertion that M$ apps are adopting a UI of tabs above the menubar. On many newer M$ apps, there isn't a menubar as such, it's been replaced by a tab selector (Word 2007, Excel 2007 for example). IE9 isn't a good example as it isn't available on Windows XP at all.

I've been looking round my applications, and I can't think of a single other one which changes its layout depending on the OS version (not the OS, note, these are both Windows)

It's commonplace for newer applications to use newer styles on older versions of operating systems. You have only to look at the titlebar of the Office 2007 suite running on Windows XP to see how they look like they are running on newer OS versions. Surely this is what Thunderbird should be copying, not making it difficult for people to use on different computers by moving things around for no good reason.

Please reconsider and make the application consistent. 

As an alternative, why not put the menu and tabs in the same line? There should be plenty of room for this, as the menu isn't big and most people probably don't have that many tabs open.
Comment 183 Charles 2012-01-26 07:50:50 PST
(In reply to Matthew Atkinson from comment #182)
> I've had a look round my Windows computer here, and I'm struggling to find
> another application which even has tabs, to support your assertion that M$
> apps are adopting a UI of tabs above the menubar. On many newer M$ apps,
> there isn't a menubar as such, it's been replaced by a tab selector (Word
> 2007, Excel 2007 for example). IE9 isn't a good example as it isn't
> available on Windows XP at all.

Ummm... read?

He said (or meant) in the newer version of Windows (7+)... he specifically said that XP doesn't and won't be changing this.
Comment 184 Mike Conley (:mconley) 2012-01-26 07:57:43 PST
Matthew:

> Mike, thanks for your reply in #180.

My pleasure. :)

> I've had a look round my Windows computer here, and I'm struggling to find
> another application which even has tabs, to support your assertion that M$
> apps are adopting a UI of tabs above the menubar. On many newer M$ apps,
> there isn't a menubar as such, it's been replaced by a tab selector (Word
> 2007, Excel 2007 for example). IE9 isn't a good example as it isn't
> available on Windows XP at all.

So, here are steps to reproduce the difference:

On Windows XP:

1)  Open up a recent version of Internet Explorer (I think IE8 is the last one it supported)
2)  This version has tabs.  Hitting the Alt key, you should see the menubar appear above the tab selector.

On Windows 7

1)  Open up IE9
2)  Hit the Alt key - you should see the menubar appear below the tab selector.

> I've been looking round my applications, and I can't think of a single other
> one which changes its layout depending on the OS version (not the OS, note,
> these are both Windows)

Believe it or not, we have two entirely different style sheets for Windows XP and Windows 7! Or, rather, we have one specifically designed for the Aero desktop environment, and one for the basic desktop environment.

> Surely this is what Thunderbird should be
> copying, not making it difficult for people to use on different computers by
> moving things around for no good reason.

This is a bit of a contradiction, depending on your point of view I guess.

I assert that from Windows 95 to Windows XP, when there has been a tab selector, that the menubar has always appeared above it.  For a user who was used to this layout, I imagine finding the menubar below the tab selector would be a bit of a shock.

If we do as you suggest, and move the tab selector above the menubar, we will have "moved things around for no good reason" to a Windows XP user.  For someone still using XP as their primary OS, I imagine this would be quite annoying.

On Windows 7, I assert that tabbed interfaces (few though there are) have the tab selector appear above the menubar.  IE9 is a great example of this.  I believe the File Explorer behaves the same way.  I agree that menubars in general are on the way out though - "ribbons" seem to be the way to go now for Windows.

If we were to have the tab selector appear below the menubar, we would have again "moved things around for no good reason" from the perspective of a user who uses Windows 7 as their primary OS.  Again, I imagine this would be quite annoying.

So the compromise is to appear native in each.

> 
> Please reconsider and make the application consistent. 
> 

I don't think we'll be doing that, for the reasons stated above.  You can, if you'd like, alter your userChrome.css file to change the ordering of the tab selector and the menubar.

http://mxr.mozilla.org/comm-central/source/mail/themes/qute/mail/primaryToolbar-aero.css#361

^-- This should give you some hints.

All the best,

-Mike
Comment 185 Matthew Atkinson 2012-01-26 08:26:33 PST
Mike, 

I accept your reasoning, but not your conclusions

(In reply to Mike Conley (:mconley) from comment #184)
> Matthew:
> 
> > Mike, thanks for your reply in #180.
> 
> My pleasure. :)
> 
> > I've had a look round my Windows computer here, and I'm struggling to find
> > another application which even has tabs, to support your assertion that M$
> > apps are adopting a UI of tabs above the menubar. On many newer M$ apps,
> > there isn't a menubar as such, it's been replaced by a tab selector (Word
> > 2007, Excel 2007 for example). IE9 isn't a good example as it isn't
> > available on Windows XP at all.
> 
> So, here are steps to reproduce the difference:
> 
> On Windows XP:
> 
> 1)  Open up a recent version of Internet Explorer (I think IE8 is the last
> one it supported)
> 2)  This version has tabs.  Hitting the Alt key, you should see the menubar
> appear above the tab selector.
> 
> On Windows 7
> 
> 1)  Open up IE9
> 2)  Hit the Alt key - you should see the menubar appear below the tab
> selector.

This isn't a valid comparison. These are different applications. If IE8 behaved like this, you would have a precedent to follow, but I don't think it does. (I may be wrong, however, as I never use IE)

> 
> > I've been looking round my applications, and I can't think of a single other
> > one which changes its layout depending on the OS version (not the OS, note,
> > these are both Windows)
> 
> Believe it or not, we have two entirely different style sheets for Windows
> XP and Windows 7! Or, rather, we have one specifically designed for the Aero
> desktop environment, and one for the basic desktop environment.
> 
> > Surely this is what Thunderbird should be
> > copying, not making it difficult for people to use on different computers by
> > moving things around for no good reason.
> 
> This is a bit of a contradiction, depending on your point of view I guess.
> 
> I assert that from Windows 95 to Windows XP, when there has been a tab
> selector, that the menubar has always appeared above it.  For a user who was
> used to this layout, I imagine finding the menubar below the tab selector
> would be a bit of a shock.

No more of a shock than discovering that the tabs have moved above the toolbar, which looks quite different from before. People are used to things changing between versions, and they get used to the new behaviour. What's much more difficult is when things change every time you go to work or go home!

> 
> If we do as you suggest, and move the tab selector above the menubar, we
> will have "moved things around for no good reason" to a Windows XP user. 
> For someone still using XP as their primary OS, I imagine this would be
> quite annoying.
> 
> On Windows 7, I assert that tabbed interfaces (few though there are) have
> the tab selector appear above the menubar.  IE9 is a great example of this. 
> I believe the File Explorer behaves the same way.  I agree that menubars in
> general are on the way out though - "ribbons" seem to be the way to go now
> for Windows.

As far as I can see, IE is the ONLY tabbed interface in Win 7. I haven't got a Win 7 to hand here, but fairly sure that File Explorer doesn't have a tabbed interface. 

> 
> If we were to have the tab selector appear below the menubar, we would have
> again "moved things around for no good reason" from the perspective of a
> user who uses Windows 7 as their primary OS.  Again, I imagine this would be
> quite annoying.
> 
> So the compromise is to appear native in each.
> 
> > 
> > Please reconsider and make the application consistent. 
> > 
> 
> I don't think we'll be doing that, for the reasons stated above.  You can,
> if you'd like, alter your userChrome.css file to change the ordering of the
> tab selector and the menubar.

I will if I must, but I thought this was supposed to be moving away from having to tweak Thunderbird to make it properly usable? I was looking forward to being able to read the menu on my Vista box without having to install an add-on (which should be for adding on features, not correcting inadequacies), but I'm now going to have to tweak settings to make it so that I don't keep shouting at the computer because the menus have switched round again. 

> 
> http://mxr.mozilla.org/comm-central/source/mail/themes/qute/mail/
> primaryToolbar-aero.css#361
> 
> ^-- This should give you some hints.
> 
> All the best,
> 
> -Mike

You didn't answer my last point, about putting them on the same line. Why not do this? That's essentially what Firefox does, although it's only got one menu item. Then there's no argument.
Comment 186 Mike Conley (:mconley) 2012-01-26 09:18:54 PST
> You didn't answer my last point, about putting them on the same line. Why
> not do this? That's essentially what Firefox does, although it's only got
> one menu item. Then there's no argument.

It sounds like you're referring to the one-day-it-will-be-developed "Thunderbutton" (see http://areweprettyyet.com/thunderbird/1/index.htm).

Yes, that's the direction we're tending towards.  However, there are a few things left to be done:

We need to determine what menu items need to go into that button, and how to arrange them.  Firefox did a great study on menu item usage and popularity (http://blog.mozilla.com/faaborg/2010/03/23/visualizing-usage-of-the-firefox-menu-bar/) that they used to design the Firefox button.

We've landed Test Pilot in Thunderbird, and we're hoping to build a test to give us similar information.

With that completed, we'll be able to construct the "Thunderbutton", at which point, this will become the primary means of accessing menu items.

I understand your frustration and annoyance with the inconsistency from one OS to another with the same version of software. This decision was not made lightly, and we weighed the pros and cons, and fell upon our compromise solution.

So, to sum up:  every solution would have caused frustration with some subset of users.  This solution causes the least.

If we're wrong about this decision, and the uproar is deafening, we will cross that bridge when we get there.

-Mike
Comment 187 Matthew Atkinson 2012-01-26 09:33:54 PST
Thanks for your comments Mike. I suspect that there will be relatively few people who use Thunderbird on both XP and Vista/7, and you won't be deafened. However, I think it's fair to say that Mozilla's record on listening and reacting isn't very good. It's taken 6 versions to get the dreadful unreadable menus sorted out despite quite a significant uproar, and Extra Folder Columns remains an add-on 8 versions later, with an enormous number of people putting that functionality back.
Comment 188 Matt 2012-02-03 23:34:54 PST
Windows XP uses Menu then toolbar then Tab bar.  For some reason, which if far from clearly explained here, Thunderbird developers have decided to go against the accepted norm. This is fine, if I can change it to suit my needs. I came here to go through the process of locating the changes, and documenting the process for the tens of thousands who will not be happy with this. (9% of users based on the Firefox experience) But what is there to document?  Some arcane CSS for the computer literate, but nothing for the average user.  Where is the user interface to control the order of tabs, toolbars and menus?

The title of this bug is. "Provide tabs on top feature like FF 4.0" where is the like Firefox ui to just change the thing.  
Perhaps this will be a reminder as to just how controversial this was in Firefox http://blog.mozilla.com/faaborg/2010/06/24/why-tabs-are-on-top-in-firefox-4/


The job of the developer here is only half done. How a radical change to the UI can get through without a simple user interface to change it escapes me.  Does no one remember just how popular and still controversial the Aero Glass stuff was.
I have filed bug 724213 requesting that a UI be instituted to support this bug before V11 is released.

Bugzilla shows bug 717264 as blocking. It's not resolved so why is this out there?

Clicking the quick search opens a pane a whole tab bar away. Hardly a polished UI design.
Comment 189 Matthew Atkinson 2012-02-13 10:49:30 PST
On my Thunderbird 11, I've got Tabs on top (though under the menu bar), but they are still using an extra line. 

On My Firefox 11, I've got Tabs on top, but they are in the title bar, which means that they usefully save screen space. 

The Firefox implementation is MUCH better, especially since I rarely even have more than one tab open in Thunderbird, and almost never more than two. 

The title of the bug contains 'like FF 4.0'. What has been implemented isn't like this at all. Surely this shouldn't be released until it's been done properly like Firefox did.
Comment 190 Mike Conley (:mconley) 2012-02-13 11:21:54 PST
Hey Matthew,

Luckily, we have long-term plans to migrate to having a similar design as Firefox (see http://areweprettyyet.com/thunderbird/1/index.htm#).

If you'd like to help us get there faster, we'd love to have some contributors work on this!

Thanks,

-Mike
Comment 191 Thomas D. (needinfo?me) 2012-02-13 12:41:46 PST
(In reply to Matthew Atkinson from comment #189)
> On my Thunderbird 11, I've got Tabs on top (though under the menu bar), but
> they are still using an extra line. 

Meanwhile, what you could do to save some vertical space is to hide the menu bar (View > Toolbars > Menu bar). On windows, you can temporarily show it if needed by pressing the ALT key, or F10. On other OS, I don't know if any of these also work.

(In reply to Mike Conley (:mconley) from comment #190)
> Luckily, we have long-term plans to migrate to having a similar design as
> Firefox (see http://areweprettyyet.com/thunderbird/1/index.htm#).

Interestingly, even that mockup does *not* place the tabs next to the Thunderbird application button, so the title bar of TB window is still unused. Is this intentional or just a glitch in the mockup?
Comment 192 Jim Porter (:squib) 2012-02-13 12:48:54 PST
(In reply to Thomas D. from comment #191)
> Interestingly, even that mockup does *not* place the tabs next to the
> Thunderbird application button, so the title bar of TB window is still
> unused. Is this intentional or just a glitch in the mockup?

Tabs aren't in-line with the Firefox button when the window isn't maximized, since it makes it impossible to move the window by dragging the top portion.
Comment 193 Joachim Herb 2012-02-13 13:20:36 PST
There is a very rudimentary version of the firefox addon "hide caption titlebar plus" (https://addons.mozilla.org/firefox/addon/hide-caption-titlebar-plus-sma/) for thunderbird: http://code.google.com/p/firefox-hide-caption-titlebar-plus/issues/detail?id=23&colspec=ID%20Type%20Status%20Priority%20Modified%20Summary 

With it you can save some more vertical space.
Comment 194 Matthew Atkinson 2012-02-14 04:04:21 PST
(In reply to Thomas D. from comment #191)

> Meanwhile, what you could do to save some vertical space is to hide the menu
> bar (View > Toolbars > Menu bar). On windows, you can temporarily show it if
> needed by pressing the ALT key, or F10. On other OS, I don't know if any of
> these also work.

Thanks, Thomas, I have done that, not really to save space, but to avoid the menu bar moving around between my work and home computers. 

(In reply to Jim Porter (:squib) from comment #192)
> (In reply to Thomas D. from comment #191)
> > Interestingly, even that mockup does *not* place the tabs next to the
> > Thunderbird application button, so the title bar of TB window is still
> > unused. Is this intentional or just a glitch in the mockup?
> 
> Tabs aren't in-line with the Firefox button when the window isn't maximized,
> since it makes it impossible to move the window by dragging the top portion.

Jik, if you're trying to maximise space, then logically you'll have maximised your window, so I don't have an issue with putting the tabs into the title bar only when it's maximised. However, Thunderbird doesn't do this at all, so this bug isn't yet fixed, it's just half a job.

(In reply to Mike Conley (:mconley) from comment #190)
> Hey Matthew,
> 
> Luckily, we have long-term plans to migrate to having a similar design as
> Firefox (see http://areweprettyyet.com/thunderbird/1/index.htm#).
> 
> If you'd like to help us get there faster, we'd love to have some
> contributors work on this!
> 
> Thanks,
> 
> -Mike

Mike, I don't have adequate Windows coding skills to help, I'm afraid, but I am trying to help in other ways. I'm not sure that it's coders which you need really, but more of a steadying hand to make sure that you stop releasing code which is really incomplete. I've been trying to do this (along with rsx11m I also notice) for quite a while, but it seems as if people are determined to get every little bit out as quickly as possible.

Here's a proposal:

This bug says 'like FF 4.0'. Why not commit to not releasing any code which makes things a bit more like Firefox until you've done enough so that it's as good as Firefox. Specifically, you'll need a Thunderbird button, you'll need tabs in the title bar, you'll need the option to choose whether Tabs are on top or not, plus I'm sure there are more I haven't thought of. 

I've been talking to Blake Winton about the UI, and I'm happy to try and help, but must admit that I'm not entirely sure how to make the jump to actually contributing code, even if it's just css.
Comment 195 Matthew Atkinson 2012-02-14 04:06:19 PST
Slightly off topic, but http://areweprettyyet.com/thunderbird/1/index.htm# fails with errors on all the bugs here: "NetworkError: 407 Proxy Authentication Required - https://api-dev.bugzilla.mozilla.org/latest/bug/644169". That makes it difficult to comment on!
Comment 196 Mike Conley (:mconley) 2012-02-14 07:32:29 PST
(In reply to Matthew Atkinson from comment #194)
> Jik, if you're trying to maximise space, then logically you'll have
> maximised your window, so I don't have an issue with putting the tabs into
> the title bar only when it's maximised. However, Thunderbird doesn't do this
> at all, so this bug isn't yet fixed, it's just half a job.

This is true - there is still work to do.

> Mike, I don't have adequate Windows coding skills to help, I'm afraid, but I
> am trying to help in other ways. I'm not sure that it's coders which you
> need really, but more of a steadying hand to make sure that you stop
> releasing code which is really incomplete.

I can assure you, as an engineer hacking on Thunderbird, that we could use more developers.  We have a massive buglist, and we're all pretty stretched to maximum.  The more hands carry the load, the faster we move, the better we get.

Windows-coding skills are not a prerequisite, necessarily.  Do you know Javascript?  CSS?  XHTML?  If you do, there's a lot you can do to improve Thunderbird.

Or perhaps you know a few developers who might be interested in hacking on Thunderbird?  If you do, send them to #maildev on irc.mozilla.org.

> Here's a proposal:
> 
> This bug says 'like FF 4.0'.

Yes, this is a bit misleading.  I've corrected the title of the bug.  Thanks for pointing that out.

> Why not commit to not releasing any code which
> makes things a bit more like Firefox until you've done enough so that it's
> as good as Firefox. 

Having more developers hacking on Thunderbird would help ensure this. :) We'd love your coding assistance!

> Specifically, you'll need a Thunderbird button, you'll
> need tabs in the title bar, you'll need the option to choose whether Tabs
> are on top or not, plus I'm sure there are more I haven't thought of. 

We need more data before we can do those projects.  We've recently integrated Test Pilot into Thunderbird, which will give us quantitative data to help inform our design decisions in those areas.

> I've been talking to Blake Winton about the UI, and I'm happy to try and
> help, but must admit that I'm not entirely sure how to make the jump to
> actually contributing code, even if it's just css.

Start by building Thunderbird for your platform (https://developer.mozilla.org/en/Simple_Thunderbird_build)

Join us in #maildev on irc.mozilla.org.

We'd love to have you help us. :)
Comment 197 rsx11m 2012-02-14 08:21:49 PST
(In reply to Mike Conley (:mconley) from comment #196)
> (In reply to Matthew Atkinson from comment #194)
> > Why not commit to not releasing any code which
> > makes things a bit more like Firefox until you've done enough so that it's
> > as good as Firefox. 
> 
> Having more developers hacking on Thunderbird would help ensure this. :)
> We'd love your coding assistance!

I think the point Matt was trying to convey (and which I'd agree with) is to defer features with such substantial UX impact from hitting the release channel until they are considered reasonably complete in contrast to squeezing it into 11.0 the day before the merge (comment #162). It just doesn't look good and first complaints came in already from 11.0b1 testers finding themselves unable to perform specific actions from a message tab.

The constraints of the rapid-release process quasi-imposing an automatism that everything checked into comm-central will ultimately end up in the n+2nd release could be considered by checking in such large changes /after/ rather than before a merge to give it another round of scrutiny and better opportunities to fix things, or by more rigorously backing out large changes when issues are encountered that would make one think again about a change.

So, it's not just a matter of coding resources but also of a bit of discipline and willingness to occasionally take a step back as opposed to pushing new features.
Comment 198 Mike Conley (:mconley) 2012-02-14 08:27:52 PST
(In reply to rsx11m from comment #197)
> So, it's not just a matter of coding resources but also of a bit of
> discipline and willingness to occasionally take a step back as opposed to
> pushing new features.

You have my full agreement on this point.

One of the advantages of the rapid release cycle was that if things weren't ready to ship, we'd simply wait for the next train.

However, we did (and still do!) feel that tabs-on-top was ready to ship, and so it will ship for TB 11.
Comment 199 Blake Winton (:bwinton) (:☕️) 2012-02-14 11:01:02 PST
Just a couple of quick notes:

(In reply to Matthew Atkinson from comment #194)
> it seems as if people are determined to get every little bit out as quickly as possible.

It probably seems that way because they are.  In my opinion, we've pushed out a few features recently which would have benefited greatly from having more time to polish them.  And tabs-on-top may even be one of those features.  But not releasing them on that schedule wasn't an option for various reasons.  :(

On a more positive note, after the last Thunderbird meeting, we've decided to shift gears to be more focused on the community, and less on the schedule, so I feel it's unlikely we'll see this sort of thing happen in the future.  (Well, unless the community decides that they want Thunderbird to release a bunch of half-baked features…  ;)

> Here's a proposal:
> 
> This bug says 'like FF 4.0'. Why not commit to not releasing any code which
> makes things a bit more like Firefox until you've done enough so that it's
> as good as Firefox. Specifically, you'll need a Thunderbird button, you'll
> need tabs in the title bar, you'll need the option to choose whether Tabs
> are on top or not, plus I'm sure there are more I haven't thought of. 

Given the relative size of the Firefox team and the Thunderbird team, this would mean that we never released any code, since they are ahead of us, and moving faster than we can, so we would never catch up.  I don't think that bringing some improvements to Thunderbird users should be prevented because we can't do everything we want to.  (Or, as someone more eloquent than I once said, "The perfect is the enemy of the good."  ;)

Thanks,
Blake.
Comment 200 Mihovil Stanic [:Mikeyy - L10n HR] 2012-02-14 11:17:46 PST
I agree with you on this. If you wait until something is perfect to release it, you will probably never release it.

There is always something to add and improve.

I for one am glad that you are doing tabs on top and I hope you will release TB button soon (TB12? :-)) and copy FF tabs behaviour ( Bug 508776 ).

Last one is very annoying since I have Calendar/Tasks tab open all the time.
Comment 201 Thomas D. (needinfo?me) 2012-02-14 12:54:04 PST
> However, we did (and still do!) feel that tabs-on-top was ready to ship, and
> so it will ship for TB 11.

That's an amazing view given the following:

Tabs on top breaks a lot of things that haven't been fixed yet:

1) Users can no longer refine gloda search terms from global search results (!), or start a global search from anywhere in the application (bug 719008, bug 717261)

2) Users can no longer use tag button to tag the currently viewed message from a message tab (!) (bug 456169, which btw is the result of still unfinished feature new message header pane, see bug 523544)

3) Users can no longer find a button to write a new message from a message tab, or, probably less severe, to open their address book (i.e. from anywhere within the application) (that's a subset of bug 725507, I think)

4) More generally, you are ripping out the fully customizable main mail toolbar (!) from places where users might depend on it to get their stuff done (!): E.g., users can no longer use toolbar buttons to apply the following commands on multiple selected messages from global search results, "open as list" mode:
  Tag, Junk, Reply, Reply All, Forward, Print, File, Mark, Back, Forward, Next,
  Previous (it's a similar problem like bug 725507, only for multiple messages -
  I don't think there's a bug for that yet)

5) Minor UI problem: 3 magnifiers in one place... (somewhere along bug 717261, bug 717264, neither being good solutions, and, perhaps, bug 570815, bug 667246, bug 526221)
Comment 202 Mike Conley (:mconley) 2012-02-14 13:06:37 PST
Hey Thomas,

Thanks for collecting all of those bugs in one place - that's a handy list.

Feel like hacking on any of them?  Got any cycles?  I'd be happy to mentor you on them.

-Mike
Comment 203 rsx11m 2012-02-14 13:08:27 PST
(In reply to Mihovil Stanic from comment #200)
> If you wait until something is perfect to release
> it, you will probably never release it.

Sure, the "ready to ship?" threshold is frequently a subjective one and a trade-off between making a new feature available as early as possible vs. considering and supporting all use cases to the extent possible in order to not upset unsuspecting users. Thus, I certainly see that line of argumentation.

One of my personal criteria for such an assessment is whether or not the feature makes life more difficult for users who don't necessarily use it (which was the motivation for me to file bug 725507, to name an example), and that's usually the type of users we see complaining in the forums.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #199)
> On a more positive note, after the last Thunderbird meeting, we've decided
> to shift gears to be more focused on the community, and less on the schedule

That's appreciated, thanks.
Comment 204 Charles 2012-02-15 02:41:15 PST
(In reply to Thomas D. from comment #201)
> > However, we did (and still do!) feel that tabs-on-top was ready to ship, and
> > so it will ship for TB 11.
> 
> That's an amazing view given the following:

<snip>

Yikes.

Thanks Thomas for pointing all of those out. In my opinion, these should serve as a catalyst for pulling this new feature from TB11, and waiting until most if not all of these are fixed before rolling it out. At a minimum, I'd like to see/hear the reasoning from the devs as to why they don't think these bugs qualify as blockers...

I guess I'm glad I haven't re-enabled auto-update for Thunderbird yet for our users (did that for both TB and FF after  the fast release switch, but have re-enabled it for Firefox without much trouble - was getting ready to do the same for Thunderbird, but this has me rethinking that decision).
Comment 205 Matthew Atkinson 2012-02-15 02:59:59 PST
I'm glad I'm not the only one who thinks this isn't a feature ready for release, but rather part of a solution which should now wait for the other half. 

Thanks to Thomas D. for pointing out some of the other parts of this solution which are still missing. 

And Thanks to Blake for saying that this shouldn't happen in the future.

rsx11m is right in saying that you should look at people who don't use the feature, and for those people this 'fix' is definitely a fail. 

I dare say I'm wasting my breath, but please can whoever has the authority do the right thing and stop this half-feature making it into live code. You could think of it as the first step to the way of thinking which Blake has said is planned for the future. Otherwise, you will be flamed, and those of us here will justifiably say 'I told you so'.
Comment 206 Thomas D. (needinfo?me) 2012-03-03 08:39:14 PST
Tweaking summary to ensure that this bug can be found with appropriate search terms.
Comment 207 Will Pittenger 2012-03-23 01:35:59 PDT
How do you turn Tabs on Top off in TB 11?  I don't like it.
Comment 208 :aceman 2012-03-23 01:45:17 PDT
In that you vote in bug 735622 and bug 724213.

Note You need to log in before you can comment on or make changes to this bug.