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).

(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

@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.

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...

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.

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)...

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

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.

Hey Andreas - do you know where that little dark gap between the tab and the tabpanel is likely coming from?

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.

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

This patch fixes a few bugs (the new message window bug, restore default set bug) and integrates Paenglab's splendid CSS patches.

(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.

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.

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

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?

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.

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.

And here we are with the menubar on.

And just for good measure, here's Windows 7 Aero, menubar off, with a Persona.

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

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...

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.

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.

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.

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

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)

Integrating Paenglab's fixes, and now the menubar is customizable.

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

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.

I've requested feedback about the try builds in http://getsatisfaction.com/mozilla_messaging/tags/bug_644169

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.

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

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.

> 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.

Fixes a small Persona theme-ing bug that caused the tabs to overlap the mail toolbar by 1 pixel.

Same as v6, but adds a line between the toolbar and the panes.

above also takes care of a bitrot

Lets see if the size goes bananas on this one as well. Only fixes a bitrot this time around.

Fixes bitrot and adds a line under the toolbar. Size of patch probably shrinks this time too, no idea why...

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)

(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.

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

This patch fixes the look of the menu in Linux, now that it's back above the tab selector.

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

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...

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.

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?

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.

(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.

Hm - just noticed that Ctrl-tabbing through tabs is broken by this patch. Investigating...

This patch merges in Andrea's fix, the 1px separator patch, and fixes tabbing forward and back via Ctrl-Tab.

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.

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.

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.

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.

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.

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

Still lots of junk lingering, but takes care of the inactive window state and is slightly nicer to Personas.

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.

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.

Fixes the jar.nm issue (but despite about 2 hours of hacking, not the other one yet)

Addresses the issue with the dropmaker being cut off.

Fixes up some bitrot from bug 680192.

This is intended to be applied on top of Patch v11.

Merging in Andreas's patch

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

Comment on attachment 580955 [details] [diff] [review] Patch v12 Blake's review should suffice for code purposes too. The only thing that looks potentially tricky implementation-wise is the customization logic, and I know that Blake has extensive experience with that. If possible, it would be nice if you could throw in some minor comments in the XUL/XML. For example, it seems like the mail toolbar now collapses under various circumstances, so maybe a short comment above its definition to that effect. Similarly, the content tab has its dummy id's; I intuit from reading them that the DOM sub-tree is likely to be removed from the document and cloned on-demand with the id's being changed. But a comment that says so would probably be more enlightening to anyone trying to read that code.

Comment 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.

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

Same as previous patch, but with pinstripe fixes. Windows coming as soon as the build is done.

This takes care of the windows styling as well.

Comment on attachment 581377 [details] [diff] [review] patch v13 Let me know if you need an installer.

Comment on attachment 581377 [details] [diff] [review] patch v13 It'd probably help if I directed the ui-r to someone...

Re-generated the patch with more context.

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.

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.

Oh, one more thing: I notice the inactive tabs are translucent in Aero. Is this what Firefox is doing on nightlies? I worry that we'll get complaints from people who hate the translucency (especially if we're not consistent with Firefox).

Comment 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)

(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

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.

Bugzilla seems to have eaten my last patch. Trying again...

Comment on attachment 582104 [details] [diff] [review] Patch v14 Well, you can still view the diff through Bugzilla. Retrieving the raw patch seems a bit more difficult. Hopefully Bugzilla will be feeling better tomorrow.

Comment 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.

Alright, so this patch exposed what I believe to be a XUL bug (see bug 711508, which I've provided a patch for). But yes, I think this ready for review now. Finally.

Comment 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?

(I haven't reviewed the tests yet. I think I'll do so in the next iteration.)

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.

That's how it is right now too. Blake, thoughts?

Andreas noticed that custom toolbars weren't being hidden by the previous patch when in a tab other than the 3pane. This occurred when we moved the mail-3bar outside of the mail-toolbox. This patch fixes that regression.

Comment 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

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…)

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.

Takes care of the css fixes Sid commented on that's Mike hadn't fixed already.

> ::: 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.

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

Comment on attachment 582842 [details] [diff] [review] Patch v19 As Sid pointed out in IRC, we're re-organizing some pretty long-standing XUL structures here...so, requesting sr.

Comment 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.

(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.

> 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.

Alright, just tested this on OSX, and the tabmail tests pass with this patch.

Comment on attachment 583161 [details] [diff] [review] Patch v21 As per Sid's advisement, I'm re-requesting superreview, since - as stated before - this patch re-orders some XUL nodes that have been pretty stable for the past few years.

Comment 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.