Closed Bug 644169 (tb-tabsontop) Opened 13 years ago Closed 12 years ago

Move tab selector above the mail toolbar, and give each tab its own toolbar (tabs on top for Thunderbird)

Categories

(Thunderbird :: Toolbars and Tabs, enhancement)

x86
All
enhancement
Not set
normal

Tracking

(thunderbird11 fixed)

RESOLVED FIXED
Thunderbird 11.0
Tracking Status
thunderbird11 --- fixed

People

(Reporter: Aureliano, Assigned: mconley)

References

(Depends on 1 open bug, )

Details

(Keywords: dev-doc-needed, Whiteboard: [gs])

Attachments

(16 files, 61 obsolete files)

67.92 KB, image/png
Details
127.50 KB, image/png
Details
86.20 KB, image/png
Details
55.79 KB, image/png
Details
44.95 KB, image/png
Details
39.11 KB, image/png
Details
29.38 KB, image/png
Details
35.82 KB, image/png
Details
80.17 KB, image/png
Details
106.02 KB, image/png
Details
204.98 KB, image/png
Details
297.37 KB, image/png
Details
192.46 KB, image/png
Details
139.92 KB, patch
rain1
: review+
standard8
: superreview+
Details | Diff | Splinter Review
1.11 KB, patch
squib
: review+
Details | Diff | Splinter Review
19.83 KB, image/png
Details
Implements "Tabs on top" feature (for me is to have opacity of toolbar buttons on Aero Theme (like in FF 4.0)).
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).
Attached image Thunderbird problem (obsolete) —
Attached image Tabs on top with global menu on FF (obsolete) —
(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.
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
Attachment #529399 - Attachment is obsolete: true
@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?
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)
Confirming, since bwinton and I have discussed this and agree it's a good thing to do.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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).
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...
Assignee: nobody → mconley
Depends on: 687836
Attached patch WIP Patch 1 (obsolete) — Splinter Review
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.
Attached image Tabs on top - the 3pane (obsolete) —
Attachment #529400 - Attachment is obsolete: true
Attachment #529457 - Attachment is obsolete: true
OS: Windows 7 → All
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)...
Attached patch WIP Patch 2 (obsolete) — Splinter Review
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
Attachment #566228 - Attachment is obsolete: true
Attachment #566275 - Flags: feedback?(squibblyflabbetydoo)
Attachment #566275 - Flags: feedback?(jonathan.protzenko)
Attachment #566275 - Flags: feedback?(bwinton)
Attachment #566234 - Attachment is obsolete: true
Attached patch WIP Patch 3 (obsolete) — Splinter Review
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.
Attachment #566275 - Attachment is obsolete: true
Attachment #566275 - Flags: feedback?(squibblyflabbetydoo)
Attachment #566275 - Flags: feedback?(jonathan.protzenko)
Attachment #566275 - Flags: feedback?(bwinton)
Attachment #566909 - Flags: feedback?(squibblyflabbetydoo)
Attachment #566909 - Flags: feedback?(jonathan.protzenko)
Hey Andreas - do you know where that little dark gap between the tab and the tabpanel is likely coming from?
Attached image Tabs on Top - 3 pane - Windows 7 (obsolete) —
Attachment #566907 - Attachment description: Tabs on top - the 3pane - better styling → Tabs on top - the 3pane - better styling - Ubuntu
Attachment #566235 - Attachment description: Tabs on top - search → Tabs on top - search - Ubuntu
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.
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).
This patch is only to get rid of the dark gap between the tab and the tabpanel under Linux. Windows needs more tweaking.
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.
Attached patch Mac enhancement CSS patch (obsolete) — Splinter Review
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 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
Attachment #566909 - Flags: feedback?(jonathan.protzenko) → feedback+
Attached patch WIP Patch 4 (obsolete) — Splinter Review
This patch fixes a few bugs (the new message window bug, restore default set bug) and integrates Paenglab's splendid CSS patches.
Attachment #566909 - Attachment is obsolete: true
Attachment #567126 - Attachment is obsolete: true
Attachment #567175 - Attachment is obsolete: true
Attachment #567322 - Attachment is obsolete: true
Attachment #566909 - Flags: feedback?(squibblyflabbetydoo)
(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!
Here's the patch running on Windows 7 with the menubar hidden.
Attached patch WIP Patch 5 (obsolete) — Splinter Review
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.
Attachment #567830 - Attachment is obsolete: true
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.
Yes, but this will be *optional*, correct (just as it is in Firefox)?
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.
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
Attached patch WIP Patch 6 (obsolete) — Splinter Review
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?
Attachment #568451 - Attachment is obsolete: true
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.
(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.
Attached patch Theme patch (obsolete) — Splinter Review
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;
}
Maybe a border between mail toolbar and content could look good. I let Blake decide if needed.
Here's a screenshot of ToT on Windows 7 Aero, with the menubar off.
Attachment #566912 - Attachment is obsolete: true
And here we are with the menubar on.
And just for good measure, here's Windows 7 Aero, menubar off, with a Persona.
Attachment #569055 - Attachment description: Windows 7 - Addons Manager - Menubar on → Windows 7 Aero - Addons Manager - Menubar on
Attached patch Patch v1 (obsolete) — Splinter Review
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
Attachment #568655 - Attachment is obsolete: true
Attachment #568908 - Attachment is obsolete: true
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...
Attached patch Mac CSS patch (obsolete) — Splinter Review
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.
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #569062 - Attachment is obsolete: true
Attachment #569181 - Attachment is obsolete: true
Attached patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #569694 - Attachment is obsolete: true
Flags: in-testsuite+
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
Attached patch Use the actual Fx tabs on aero (obsolete) — Splinter Review
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)
Attached patch Patch v4 (obsolete) — Splinter Review
Integrating Paenglab's fixes, and now the menubar is customizable.
Attachment #569769 - Attachment is obsolete: true
Attachment #570082 - Attachment is obsolete: true
Attachment #570567 - Attachment is obsolete: true
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
Attachment #570720 - Flags: ui-review?(bwinton)
Attachment #570720 - Flags: review?(squibblyflabbetydoo)
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 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.
Attached patch remove the align="start" (obsolete) — Splinter Review
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.
Whiteboard: [gs]
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?
(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'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).
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
(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(
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.  ;)
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".
(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.
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).
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.
Comments 9 through 12 of bug 702797 are relevant to this bug's comments
Attached patch Patch v5 (obsolete) — Splinter Review
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.
Attachment #570720 - Attachment is obsolete: true
Attachment #572541 - Attachment is obsolete: true
Attachment #570720 - Flags: ui-review?(bwinton)
Attachment #570720 - Flags: review?(squibblyflabbetydoo)
> 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.
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.
Attached patch Patch v6 (obsolete) — Splinter Review
Fixes a small Persona theme-ing bug that caused the tabs to overlap the mail toolbar by 1 pixel.
Attachment #576206 - Attachment is obsolete: true
Attached patch tiny update (obsolete) — Splinter Review
Same as v6, but adds a line between the toolbar and the panes.
above also takes care of a bitrot
Blocks: 508776
Attached patch tiny bitrot fix (obsolete) — Splinter Review
Lets see if the size goes bananas on this one as well. Only fixes a bitrot this time around.
Attachment #576783 - Attachment is obsolete: true
Attached patch trying again (obsolete) — Splinter Review
Fixes bitrot and adds a line under the toolbar. Size of patch probably shrinks this time too, no idea why...
Attachment #576938 - Attachment is obsolete: true
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)
Attachment #576945 - Attachment is obsolete: true
(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.
Blocks: 705381
Paenglab:

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

-Mike
Attachment #576218 - Attachment is obsolete: true
Attached patch Patch v8 (obsolete) — Splinter Review
This patch fixes the look of the menu in Linux, now that it's back above the tab selector.
Attachment #576951 - Attachment is obsolete: true
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
Attached patch style fixes for Win7 (obsolete) — Splinter Review
This small patch takes care of tab appearance on Win7 so the color change between the tab and the toolbar looks smooth.
(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.
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.
(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...
Attached patch Style fixes for Linux and Win7 (obsolete) — Splinter Review
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.
Attachment #577648 - Attachment is obsolete: true
Blocks: 661742
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.
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?
Attachment #578077 - Attachment is obsolete: true
Attachment #578378 - Flags: feedback?(squibblyflabbetydoo)
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.
Attachment #578378 - Flags: feedback?(squibblyflabbetydoo) → feedback+
(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
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
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.
Attachment #578378 - Attachment is obsolete: true
Hm - just noticed that Ctrl-tabbing through tabs is broken by this patch.  Investigating...
Attached patch Patch v9 (obsolete) — Splinter Review
This patch merges in Andrea's fix, the 1px separator patch, and fixes tabbing forward and back via Ctrl-Tab.
Attachment #577364 - Attachment is obsolete: true
Attachment #577959 - Attachment is obsolete: true
Attachment #578579 - Attachment is obsolete: true
Attached patch Windows XP additional patch (obsolete) — Splinter Review
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.
Attached patch Patch v9 (minor update) (obsolete) — Splinter Review
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.
Attachment #579076 - Attachment is obsolete: true
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.
Attached patch Windows XP additional patch (obsolete) — Splinter Review
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.
Attachment #579417 - Attachment is obsolete: true
Attached patch Patch v10 (obsolete) — Splinter Review
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.
Attachment #579551 - Attachment is obsolete: true
Attachment #579738 - Attachment is obsolete: true
Attached patch pinstripe tab (obsolete) — Splinter Review
Slightly rough pinstripe tab patch (some personas issues left still).
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
Attached patch slight update to previous patch (obsolete) — Splinter Review
Still lots of junk lingering, but takes care of the inactive window state and is slightly nicer to Personas.
Attachment #580030 - Attachment is obsolete: true
Attached patch pinstripe tabs v3 (obsolete) — Splinter Review
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.
Attachment #580061 - Attachment is obsolete: true
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
(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.
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.
Attached patch pinstripe tabs v3 (obsolete) — Splinter Review
Fixes the jar.nm issue (but despite about 2 hours of hacking, not the other one yet)
Attached patch pinstripe tabs v4 (obsolete) — Splinter Review
Addresses the issue with the dropmaker being cut off.
Attachment #580380 - Attachment is obsolete: true
Attachment #580664 - Attachment is obsolete: true
Attached patch Patch v11 (obsolete) — Splinter Review
Fixes up some bitrot from bug 680192.
Attachment #579806 - Attachment is obsolete: true
Attachment #580784 - Attachment is obsolete: true
Attached file pinstripe tabs v4(ish) (obsolete) —
This is intended to be applied on top of Patch v11.
Attached patch Patch v12 (obsolete) — Splinter Review
Merging in Andreas's patch
Attachment #580899 - Attachment is obsolete: true
Attachment #580922 - Attachment is obsolete: true
asuth:

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

-Mike
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
Attachment #580955 - Flags: ui-review?(bwinton)
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.
Attachment #580955 - Flags: review?(bwinton)
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.
Attachment #580955 - Flags: ui-review?(bwinton)
Attachment #580955 - Flags: review?(bwinton)
Attached patch patch v13 (wip) (obsolete) — Splinter Review
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
Attached patch patch v13 (wip) (obsolete) — Splinter Review
Same as previous patch, but with pinstripe fixes. Windows coming as soon as the build is done.
Attachment #581265 - Attachment is obsolete: true
Attached patch patch v13 (obsolete) — Splinter Review
This takes care of the windows styling as well.
Attachment #580955 - Attachment is obsolete: true
Attachment #581284 - Attachment is obsolete: true
Comment on attachment 581377 [details] [diff] [review]
patch v13

Let me know if you need an installer.
Attachment #581377 - Flags: ui-review?
Comment on attachment 581377 [details] [diff] [review]
patch v13

It'd probably help if I directed the ui-r to someone...
Attachment #581377 - Flags: ui-review? → ui-review?(bwinton)
Re-generated the patch with more context.
Attachment #581377 - Attachment is obsolete: true
Attachment #581377 - Flags: ui-review?(bwinton)
Attachment #581650 - Flags: ui-review?(bwinton)
Attachment #581650 - Flags: review?(sagarwal)
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.
Attachment #581650 - Flags: ui-review?(bwinton) → ui-review+
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.
Blocks: 710831
Blocks: 710838
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).
Depends on: 710867
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)
Attachment #581650 - Flags: review?(sagarwal) → review-
(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
Target Milestone: --- → Thunderbird 11.0
Attached patch Patch v14 (obsolete) — Splinter Review
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.
Attachment #581650 - Attachment is obsolete: true
Attachment #582099 - Flags: review?(sagarwal)
Attached patch Patch v14 (obsolete) — Splinter Review
Bugzilla seems to have eaten my last patch.  Trying again...
Attachment #582099 - Attachment is obsolete: true
Attachment #582099 - Flags: review?(sagarwal)
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.
Attachment #582104 - Flags: review?(sagarwal)
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.
Attachment #582104 - Flags: review?(sagarwal)
Depends on: 711508
Attached patch Patch v15 (obsolete) — Splinter Review
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.
Attachment #582104 - Attachment is obsolete: true
Attachment #582303 - Flags: review?(sagarwal)
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?
Attachment #582303 - Flags: review?(sagarwal) → review-
(I haven't reviewed the tests yet. I think I'll do so in the next iteration.)
Attached patch WIP Patch 16 (obsolete) — Splinter Review
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.
Attachment #582303 - Attachment is obsolete: true
That's how it is right now too. Blake, thoughts?
Attached patch WIP Patch 17 (obsolete) — Splinter Review
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.
Attachment #582545 - Attachment is obsolete: true
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
Attachment #582571 - Flags: ui-review?(bwinton)
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.
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?
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.
(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.
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 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…)
Attachment #582571 - Flags: ui-review?(bwinton) → ui-review-
Attached patch WIP Patch 18 (obsolete) — Splinter Review
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.
Attachment #582571 - Attachment is obsolete: true
Attached patch WIP patch 19 (obsolete) — Splinter Review
Takes care of the css fixes Sid commented on that's Mike hadn't fixed already.
Attachment #582704 - Attachment is obsolete: true
> ::: 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
(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.
Attached patch Patch v19 (obsolete) — Splinter Review
Hey Sid, I think we're ready for another review iteration.  Have at it!

Thanks,

-Mike
Attachment #582832 - Attachment is obsolete: true
Attachment #582842 - Flags: review?(sagarwal)
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.
Attachment #582842 - Flags: superreview?(mbanner)
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.
Attachment #582842 - Flags: review?(sagarwal) → review-
(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.
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.
(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.
(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.
Blocks: 710867
No longer depends on: 710867
Attached patch Patch v20 (obsolete) — Splinter Review
> 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.
Attachment #582842 - Attachment is obsolete: true
Attachment #582842 - Flags: superreview?(mbanner)
Attached patch Patch v21Splinter Review
Alright, just tested this on OSX, and the tabmail tests pass with this patch.
Attachment #583153 - Attachment is obsolete: true
Blocks: 712322
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.
Attachment #583161 - Flags: superreview?(mbanner)
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.
Attachment #583161 - Flags: review+
Depends on: 712395
Attachment #583161 - Flags: superreview?(mbanner) → superreview+
Checked in to comm-central as http://hg.mozilla.org/comm-central/rev/3f507b7d7ee6

\o/
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 712560
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?
(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.
Depends on: 712624
(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...
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
Attachment #583495 - Flags: review?(squibblyflabbetydoo)
We've changed the positioning of some pretty long-standing XUL nodes.  We'll almost certainly need to alert add-on developers.
Keywords: dev-doc-needed
Depends on: 712938
Depends on: 713008
Depends on: 713058
Depends on: 713061
Depends on: 713071, 713079
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.)
Attachment #583495 - Flags: review?(squibblyflabbetydoo) → review+
Comment on attachment 583495 [details] [diff] [review]
Patch for accidental undelete command reassignment

Requesting approval for Aurora
Attachment #583495 - Flags: approval-comm-aurora?
Thanks squib, committed to comm-central as http://hg.mozilla.org/comm-central/rev/3b4cb1418917
Depends on: 713919
Attachment #583495 - Flags: approval-comm-aurora? → approval-comm-aurora+
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?
aceman:

Thanks, done. :)

-Mike
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...
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
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!
Depends on: 717264
Depends on: 717261
Depends on: 715565
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?
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.
Border between tab and email body is marked with arrow.
You can also see that tabs are not on top.

WinXP
(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
(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
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.
(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.
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
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.
> 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
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.
Depends on: 721405
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.
Blocks: 724213
Depends on: 725507
Depends on: 719008
Alias: tb-tabsontop
Depends on: 456169
Depends on: 724324
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.
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
(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?
(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.
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.
(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.
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!
(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. :)
Summary: Provide tabs on top feature like FF 4.0 → Move tab selector above the mail toolbar, and give each tab its own toolbar.
(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.
(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.
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.
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.
> 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)
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
(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.
(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).
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'.
Depends on: 728309
Tweaking summary to ensure that this bug can be found with appropriate search terms.
Summary: Move tab selector above the mail toolbar, and give each tab its own toolbar. → Move tab selector above the mail toolbar, and give each tab its own toolbar (tabs on top for Thunderbird)
Depends on: 732803
Depends on: 735622
How do you turn Tabs on Top off in TB 11?  I don't like it.
In that you vote in bug 735622 and bug 724213.
Depends on: 738857
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: