731264 - Support with multiple toolboxes in MailNews due to Lighting Calendar and Task Tabs.

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Philip Chee]

Lightning Bug 709572 Added Calendar and Task toolbars for Thunderbird tabs on top. At the moment I have some custom code in Lighting to adapt their customize menu to call our goCustomizeToolbar() with the proper argument. This could be simplified if I generalize our code to take into account multiple toolboxes in the window. This also includes the utilityOverlay.js of the branch fix in Bug 721327.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Proposed Fix.

> This also includes the utilityOverlay.js of the branch fix in Bug 721327.
*This also includes the utilityOverlay.js changes from the branch fix in Bug 721327.*

> +  // XXXRatty: Lightning adds a customize menu to a vbox enclosing the toolbox.
> +  // Have to think of what to do here and in the *popupShowing handlers.

I don't know why they did it that way. When I asked Squib, he said he mostly cargo-culted stuff wholesale from Thunderbird.

> +  var externalToolbars = Array.filter(toolbox.externalToolbars,
> +                                      function(toolbar) {
> +                                        return toolbar.hasAttribute("toolbarname")});
Extensions with external toolbars might not have toolbarnames especially since Thunderbird's external tabbar-toolbar doesn't have one.

Comment 2

[[e-mail]]

Comment on attachment 601297 [details] [diff] [review]
Patch v1.0 Proposed Fix.

>+  // XXXRatty: Lightning adds a customize menu to a vbox enclosing the toolbox.
>+  // Have to think of what to do here and in the *popupShowing handlers.
I think the code to deal with the customize context menu should be in a separate method, called directly from the customize popup menuitem.

Comment 3

[[e-mail]]

It's also unclear who the caller is; I couldn't tell from reading bug 709572.

Comment 4

[Philip Chee]

Attachment: Patch v1.1

> I think the code to deal with the customize context menu should be in a
> separate method, called directly from the customize popup menuitem.

I hope I've understood you correctly in this updated patch.

> It's also unclear who the caller is; I couldn't tell from reading bug 709572.

OK. Lightning injects three additional toolboxes into messenger.xul:

calendar-toolbar2 in the Calendar tab.

task-toolbar2 and task-actions-toolbox in the Tasks tab.

Lighting has a separate context menu for each toolbox:
calendar-toolbar-context, task-toolbar-context, and task-actions-toolbox.

We use the same overlay context menu for each of SeaMonkeys toolboxes and
this works because we have only one toolbox in each window. The current
fix I did for each of Lightnings context menus is to point each to
goCustomizeToolbar(). But our context menu has more functionality and I want
to use that for all the Lightning toolboxes as well. We can do that if we
generalise GoCustomizeToolbar() to check where the context menu is attached
to when it is called. Once I do that I can override the Lightning context=
attributes like so:

  <toolbar id="task-actions-toolbar"
           xpfe="false"
           context="toolbar-context-menu"/>
  <toolbar id="calendar-toolbar2"
           context="toolbar-context-menu"/>
  <toolbar id="task-toolbar2"
           context="toolbar-context-menu"/>

See the Lightning WIP patch I'll attach later.

Now the context menu for the Task Actions Toolbar is also for some reason
used by an enclosing vbox:

        <vbox id="calendar-task-details-container"
              class="main-header-area"
              flex="1"
              persist="height"
              context="task-actions-toolbar-context-menu">

So I've split goCustomizeToolbar() into two and then pointed the context menu
for the vbox to the _customizeToolbox() function instead.

  <menuitem id="CustomizeTaskActionsToolbar"
            oncommand="_customizeToolbox(document.getElementById('task-actions-toolbox'))"/>

Hope this makes sense.

Comment 5

[Philip Chee]

Attachment: Lightning WIP v1.0 Patch. [for reference only]

Comment 6

[[e-mail]]

Comment on attachment 608783 [details] [diff] [review]
Lightning WIP v1.0 Patch. [for reference only]

Don't forget the toolbarset ;-)

Comment 7

[[e-mail]]

(In reply to Philip Chee from comment #4)
> > I think the code to deal with the customize context menu should be in a
> > separate method, called directly from the customize popup menuitem.
> I hope I've understood you correctly in this updated patch.
Don't worry, I didn't understand your original patch anyway.

Comment 8

[[e-mail]]

Comment on attachment 608777 [details] [diff] [review]
Patch v1.1

>-    <command id="cmd_CustomizeToolbars" oncommand="goCustomizeToolbar(getNavToolbox());"/>
Actually I was thinking that we should get rid of this command altogether. It only existed so that the menuitem knew the id of the toolbox it was customising. If it's going to work that out for itself then this isn't necessary, and the menuitem should just call goCustomize itself.

>-function goCustomizeToolbar(toolbox)
You should keep goCustomizeToolbar as the version that accepts a toolbox, whether for backward compatibility or so that CustomizeTaskActionsToolbar can call it, if you decide to implement it like that.

Comment 9

[[e-mail]]

(In reply to comment #8)
>(From update of attachment 608777 [details] [diff] [review])
>>-    <command id="cmd_CustomizeToolbars" oncommand="goCustomizeToolbar(getNavToolbox());"/>
>Actually I was thinking that we should get rid of this command altogether.
>It only existed so that the menuitem knew the id of the toolbox it was
>customising. If it's going to work that out for itself then this isn't
>necessary, and the menuitem should just call goCustomize itself.
And return the value, so goCustomize can return false if you can't find the toolbar because you clicked at the wrong place in the vbox ;-)

Comment 10

[Philip Chee]

Attachment: Patch v1.2 fix nits.

>>-    <command id="cmd_CustomizeToolbars" oncommand="goCustomizeToolbar(getNavToolbox());"/>
> Actually I was thinking that we should get rid of this command altogether.
> It only existed so that the menuitem knew the id of the toolbox it was
> customising. If it's going to work that out for itself then this isn't
> necessary, and the menuitem should just call goCustomize itself.

toolboxCustomizeInit() and toolboxCustomizeDone() set/removeAttribute disabled
on cmd_CustomizeToolbars.

I could make them do this on the menuitem directly, but I'd like to leave
this in in case other elements want to observe the disabled state.

> And return the value, so goCustomize can return false if you can't find
> the toolbar because you clicked at the wrong place in the vbox ;-)
Fixed.

>>-function goCustomizeToolbar(toolbox)
> You should keep goCustomizeToolbar as the version that accepts a toolbox,
> whether for backward compatibility or so that CustomizeTaskActionsToolbar
> can call it, if you decide to implement it like that.
Fixed.

> +function SuiteCustomizeToolbar(aMenuItem)
Firefox uses BrowserCustomizeToolbar() so I'll just follow Suite.

Comment 11

[[e-mail]]

Comment on attachment 610167 [details] [diff] [review]
Patch v1.2 fix nits.

I couldn't open any Calendar tabs with the two patches applied :-( Updating my build to see whether that fixes the issue.

>+  while (toolbar.localName != "toolbar") {
>+    toolbar = toolbar.parentNode;
>+  }
>+  if (!toolbar)
>+    return false;
toolbar can never be null because you checked its localName first. Need to move the null check inside the loop instead.

>+    <command id="cmd_CustomizeToolbars"/>
Please make this a <broadcaster>.

Comment 12

[[e-mail]]

So, I tried these patches, and I don't really like the result.

Firstly, the calendar and task tabs share a deck which contains two toolbars. This means that the height of the toolbars needs to be the same. If you try to change the settings of the toolbar this just makes it look wrong. (Being able to collapse the grippy is also wrong, and the overlay should add xpfe="false".)

Secondly, those toolbars can be accidentally hidden via the context menu, and there is no way of restoring them, because they don't show up in the main menu.

Comment 13

[Philip Chee]

Attachment: Lightning Lv1.1 Revised patch.

Comment 14

[Philip Chee]

Attachment: Patch v1.3 more fixes.

>>+  while (toolbar.localName != "toolbar") {
>>+    toolbar = toolbar.parentNode;
>>+  }
>>+  if (!toolbar)
>>+    return false;
> toolbar can never be null because you checked its localName first. Need to move the null check inside the loop instead.
Fixed.

>>+    <command id="cmd_CustomizeToolbars"/>
> Please make this a <broadcaster>.
Fixed.

> So, I tried these patches, and I don't really like the result.
> 
> Firstly, the calendar and task tabs share a deck which contains two
> toolbars. This means that the height of the toolbars needs to be the same.
> If you try to change the settings of the toolbar this just makes it look
> wrong.
This also happens in Thunderbird. I'd say this was a design problem in Lightning and can't be fixed in /suite.

> (Being able to collapse the grippy is also wrong, and the overlay should
> add xpfe="false".)
Fixed. But I used CSS and a moz-binding since users can add custom toolbars to the Calendar and Tasks toolboxes.

> Secondly, those toolbars can be accidentally hidden via the context menu,
> and there is no way of restoring them, because they don't show up in the
> main menu.
Fixed by preventing the lightning toolbars from being hidden. I added a |alwaysvisible| attribute to the Lightning toolboxes to do this. See attached revised Lightning patch as well.

Comment 15

[Philip Chee]

Comment on attachment 611864 [details] [diff] [review]
Patch v1.3 more fixes.

Oops forgot to set the review? flag.

Comment 16

[[e-mail]]

Comment on attachment 611864 [details] [diff] [review]
Patch v1.3 more fixes.

>+<broadcasterset id="mainBroadcasterSet"/>
navigator.xul already has this.

>+  if (toolbox.hasAttribute("alwaysvisible") || !toolbars.length) {
Hmm, this isn't ideal, because people would have to know to add this attribute... maybe we could check whether the first toolbar is a menu?

Comment 17

[[e-mail]]

(In reply to Philip Chee from comment #14)
> > (Being able to collapse the grippy is also wrong, and the overlay should
> > add xpfe="false".)
> Fixed. But I used CSS and a moz-binding since users can add custom toolbars
> to the Calendar and Tasks toolboxes.
OK, so given your other comments, we could stick with grippies, and assume Lightning will fix their toolbox at some point. Or, we could tweak our CSS so that xpfe="false" on the toolbox will affect all of the toolbars too. Either way, we should probably run the patch past stefanh at some point.

> > Secondly, those toolbars can be accidentally hidden via the context menu,
> > and there is no way of restoring them, because they don't show up in the
> > main menu.
> Fixed by preventing the lightning toolbars from being hidden. I added a
> |alwaysvisible| attribute to the Lightning toolboxes to do this. See
> attached revised Lightning patch as well.
Actually I just came up with an alternative option - exclude the first real toolbar if it hasn't already been excluded by virtue of not being named.

Comment 18

[Philip Chee]

Attachment: Lightning Lv1.2 Revised patch.

I'll file a separate bug for the Lighting changes. Meanwhile you'll need this to test the main Suite patch.

Comment 19

[Philip Chee]

Attachment: Patch v1.4 fix nits.

>>+<broadcasterset id="mainBroadcasterSet"/>
> navigator.xul already has this.
Fixed.

>>+  if (toolbox.hasAttribute("alwaysvisible") || !toolbars.length) {
> Hmm, this isn't ideal, because people would have to know to add this
> attribute... maybe we could check whether the first toolbar is a menu?
I've added a check for a menubar at any position in the toolbox.

>>> (Being able to collapse the grippy is also wrong, and the overlay should
>>> add xpfe="false".)
>> Fixed. But I used CSS and a moz-binding since users can add custom toolbars
>> to the Calendar and Tasks toolboxes.
> OK, so given your other comments, we could stick with grippies, and assume
> Lightning will fix their toolbox at some point. Or, we could tweak our CSS
> so that xpfe="false" on the toolbox will affect all of the toolbars too.
I've tweaked the CSS in communicator.css because this seems the easiest.

> Either way, we should probably run the patch past stefanh at some point.
OK. Setting review request.

>>> Secondly, those toolbars can be accidentally hidden via the context menu,
>>> and there is no way of restoring them, because they don't show up in the
>>> main menu.
>> Fixed by preventing the lightning toolbars from being hidden. I added a
>> |alwaysvisible| attribute to the Lightning toolboxes to do this. See
>> attached revised Lightning patch as well.
> Actually I just came up with an alternative option - exclude the first real
> toolbar if it hasn't already been excluded by virtue of not being named.
I think the menubar method is easier.

Comment 20

[Stefan [:stefanh]]

Philip,

I never used Lightning. I assume I need the lightning patch to test this. Do I need to build lightning or is there a way to build seamonkey so that lightning is included?

Comment 21

[Stefan [:stefanh]]

Philip,

I never used Lightning. I assume I need the lightning patch to test this. Do I need to build lightning or is there a way to build seamonkey so that lightning is included?

Thanks

Comment 22

[Philip Chee]

To build SeaMonkey with Lightning included, add to your mozconfig:
ac_add_options --enable-calendar

Comment 23

[Stefan [:stefanh]]

Comment on attachment 612911 [details] [diff] [review]
Patch v1.4 fix nits.

Some initial comments/questions:

1) If you don't want any grippy, you need to override the grippy binding in the mac version of toolbar.css

2) Hmm, I have the calendar and tasks icons in the tabs toolbar, but I also see them in the customize window. This is intentional?

3) One problem is that the calendar/tasks buttons in the tab bar increase the height of the mail tabs and therefore sort of messes up the styling. Is there any way we could use some other icons or perhaps put them somewhere else?

4) Does an "inline-toolbar" always appear below the tabs? The reason I ask is that the current styling doesn't really matches the tab(s), so I think it needs to be re-styled.

Comment 24

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #23)
> 3) One problem is that the calendar/tasks buttons in the tab bar increase
> the height of the mail tabs and therefore sort of messes up the styling. Is
> there any way we could use some other icons or perhaps put them somewhere
> else?

I guess not... I have to see how thunderbird does this...

Comment 25

[Philip Chee]

> 1) If you don't want any grippy, you need to override the grippy binding in
> the mac version of toolbar.css

Doesn't mac classic/skin/communicator.css import from content/communicator.css?

> 2) Hmm, I have the calendar and tasks icons in the tabs toolbar, but I also
> see them in the customize window. This is intentional?

It appears that it is intentional (at least in Thunderbird).

> 3) One problem is that the calendar/tasks buttons in the tab bar increase
> the height of the mail tabs and therefore sort of messes up the styling.
> Is there any way we could use some other icons or perhaps put them
> somewhere else?

Screenshot please? Looks OK on Windows.

The buttons/icons come from Lightning/Calendar. In winstripe and gnomestripe the tabbar-toolbar buttons are 16x16.
<http://hg.mozilla.org/comm-central/annotate/0f8b5129cfe6/calendar/lightning/themes/pinstripe/lightning.css#l66>
But the pinstripe icons are 24x24.

> 4) Does an "inline-toolbar" always appear below the tabs? The reason I ask
> is that the current styling doesn't really matches the tab(s), so I think
> it needs to be re-styled.

They can appear anywhere. For example open the Calendar Task tab. Create a
task. Highlight/Select it to open the Tasks Detail pane. The pane header has
an in-line toolbar.

The Thunderbird message preview pane also has an in-line toolbar.

Comment 26

[Stefan [:stefanh]]

(In reply to Philip Chee from comment #25)
> > 1) If you don't want any grippy, you need to override the grippy binding in
> > the mac version of toolbar.css
> 
> Doesn't mac classic/skin/communicator.css import from
> content/communicator.css?

Yes, but I'm talking about toolbar.css. Toolbars with grippies have overrides in toolbar.css so we get the grippytoolbar-drag binding. Toolbars with nowindowdrag attribute set to true, shouldn't get that binding, though. See also bug 727567 (I'm not in front of my mac atm and I don't remember offhand what attributes the toolbars that got the binding had).
> 
> 
> > 3) One problem is that the calendar/tasks buttons in the tab bar increase
> > the height of the mail tabs and therefore sort of messes up the styling.
> > Is there any way we could use some other icons or perhaps put them
> > somewhere else?
> 
> Screenshot please? Looks OK on Windows.
> 
> The buttons/icons come from Lightning/Calendar. In winstripe and gnomestripe
> the tabbar-toolbar buttons are 16x16.
> <http://hg.mozilla.org/comm-central/annotate/0f8b5129cfe6/calendar/lightning/
> themes/pinstripe/lightning.css#l66>
> But the pinstripe icons are 24x24.

Ah, that's the reason. Our Mac tab styling relies on surrounding element(s) that gets stretched. I suppose I could look at re-styling them.

> 
> > 4) Does an "inline-toolbar" always appear below the tabs? The reason I ask
> > is that the current styling doesn't really matches the tab(s), so I think
> > it needs to be re-styled.
> 
> They can appear anywhere. For example open the Calendar Task tab. Create a
> task. Highlight/Select it to open the Tasks Detail pane. The pane header has
> an in-line toolbar.
> 
> The Thunderbird message preview pane also has an in-line toolbar.

OK, but I guess they never appear above the tabs?
---------------------
Main toolbar
---------------------
 ____ ____ ____ ____ <-----?
|tab1|tab2|tab3|tab4|

Comment 27

[Philip Chee]

> Yes, but I'm talking about toolbar.css. Toolbars with grippies have
> overrides in toolbar.css so we get the grippytoolbar-drag binding.
> Toolbars with nowindowdrag attribute set to true, shouldn't get that
> binding, though. See also bug 727567 (I'm not in front of my mac atm
> and I don't remember offhand what attributes the toolbars that got the
> binding had).
Well since I don't have a mac I don't know how to fix it. So I'll just
assume that you'll fix it in bug 727567.

> OK, but I guess they never appear above the tabs?
Well above the tabs is our main toolbox and that should never contain an in-line toolbar.

Comment 28

[Stefan [:stefanh]]

(In reply to Philip Chee from comment #27)
> > Yes, but I'm talking about toolbar.css. Toolbars with grippies have
> > overrides in toolbar.css so we get the grippytoolbar-drag binding.
> > Toolbars with nowindowdrag attribute set to true, shouldn't get that
> > binding, though. See also bug 727567 (I'm not in front of my mac atm
> > and I don't remember offhand what attributes the toolbars that got the
> > binding had).
> Well since I don't have a mac I don't know how to fix it. So I'll just
> assume that you'll fix it in bug 727567.

As I said, I'm not in front of my mac atm. But when I get to it, I will tell you how to fix it.

Comment 29

[Stefan [:stefanh]]

Comment on attachment 612911 [details] [diff] [review]
Patch v1.4 fix nits.

First, I noticed that the close menu item (main menu) is disabled in Calendar/Tasks tabs (shortcut doesn't work either, of course). Context menu works, though. Also, there's no Calendar/Tasks icon in the alltabs menu.

Second, I spent some time looking at the css (most of this is ripped from thunderbird) and adding this to the mac classic files will make it look OK for now, I think:

mailWindow1.css
---------------
Remove the "border-bottom: 1px solid #A1A1A1;" from .tabmail-strip and add it to #messengerBox (you can just insert it below #messengerWindow styles)

Add this to fix the expanded tab height (bottom of file):
#tabbar-toolbar > .toolbarbutton-1 {
  margin-top: 1px;
  margin-bottom: 1px;
  padding-top: 0;
  padding-bottom: 0;
}

messageHeader.css
-----------------
The native styling is way to dark for us atm so we'll use the .mail-toolbox background:
.inline-toolbar {
  -moz-appearance: none;
}

primaryToolbar.css
------------------
Make toolboxes melt in with tab background and add a bottom border:
.mail-toolbox {
  border-bottom: 1px solid #A1A1A1;
  background-color: #E8E8E8;
}

(We need some more fundamental changes in the future, I think - we (I) should consider doing an overhaul of our tab and toolbar styles, see for example my last sentence)

Regarding the toolbox[xpfe="false"] > toolbar:

It's a bit tricky, since:
1) we first get the toolkit toolbar binding
2) In pinstripe's global.css we get (if nowindowdrag="true"):
-moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");
3) Then we override that in content/communicator.css and get the "normal" toolkit binding back.
4) Then it gets overridden again by style rules in classic/mac/communicator/toolbar.css. Now, we miss the xpfe="false" case there, but that doesn't help us here - we need a new rule anyway. This will work (lack of window drag attribute tells us that we want the drag binding):
toolbox[xpfe="false"] > toolbar:not([nowindowdrag="true"]) {
  -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");
}

The above rule is correct, apart from that we don't have a dark, draggable toolbar, but I think we have to live with that for now (after all, the xul tells us it should be draggable)

I said you could add the binding (on IRC), but it might actually be better to fix it in bug 727567, since I haven't looked through all cases. Also, all these overrides doesn't feel like the best way to handle it - maybe someone has a better idea for bug 727567? 

When you're done, can you please add a new patch with my suggested css, so I can do a final spin with it?

Comment 30

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #29)
> (We need some more fundamental changes in the future, I think - we (I)
> should consider doing an overhaul of our tab and toolbar styles, see for
> example my last sentence)
> 
 And by "last sentence", I ment this one:
> The above rule is correct, apart from that we don't have a dark, draggable
> toolbar, but I think we have to live with that for now (after all, the xul
> tells us it should be draggable)

Comment 31

[Philip Chee]

Attachment: Patch v1.5 Mac Styling added.

> mailWindow1.css
> ---------------
> Remove the "border-bottom: 1px solid #A1A1A1;" from .tabmail-strip and add it 
> to #messengerBox (you can just insert it below #messengerWindow styles)
Done.

> Add this to fix the expanded tab height (bottom of file):
> #tabbar-toolbar > .toolbarbutton-1 {
>   margin-top: 1px;
>   margin-bottom: 1px;
>   padding-top: 0;
>   padding-bottom: 0;
> }
Done.

> messageHeader.css
> -----------------
> The native styling is way to dark for us atm so we'll use the .mail-toolbox 
> background:
> .inline-toolbar {
>   -moz-appearance: none;
> }
Done. But we don't have an inline toolbar in our message header pane unlike
Thunderbird.

> primaryToolbar.css
> ------------------
> Make toolboxes melt in with tab background and add a bottom border:
> .mail-toolbox {
>   border-bottom: 1px solid #A1A1A1;
>   background-color: #E8E8E8;
> }
Done.

> 4) Then it gets overridden again by style rules in 
> classic/mac/communicator/toolbar.css. Now, we miss the xpfe="false" case there, 
> but that doesn't help us here - we need a new rule anyway. This will work (lack 
> of window drag attribute tells us that we want the drag binding):

> toolbox[xpfe="false"] > toolbar:not([nowindowdrag="true"]) {
>   -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-
> drag");
> }
Done.

> When you're done, can you please add a new patch with my suggested css, so I 
> can do a final spin with it?
OK.

Comment 32

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #29)
> Comment on attachment 612911 [details] [diff] [review]
> Patch v1.4 fix nits.
> 
> First, I noticed that the close menu item (main menu) is disabled in
> Calendar/Tasks tabs (shortcut doesn't work either, of course). Context menu
> works, though. Also, there's no Calendar/Tasks icon in the alltabs menu.
> 

I might not be able to test the css until the weekend, but what about the above?

Comment 33

[Philip Chee]

> I might not be able to test the css until the weekend, but what about the above?
Does this also happen without my patch?

Comment 34

[Stefan [:stefanh]]

Comment on attachment 614759 [details] [diff] [review]
Patch v1.5 Mac Styling added.

(In reply to Philip Chee from comment #31)
> Created attachment 614759 [details] [diff] [review]
> Patch v1.5 Mac Styling added.
> 
> > mailWindow1.css
> > ---------------
> > Remove the "border-bottom: 1px solid #A1A1A1;" from .tabmail-strip and add it 
> > to #messengerBox (you can just insert it below #messengerWindow styles)
> Done.

Sorry, it should be border-top for #messengerBox.
.
.

> > messageHeader.css
> > -----------------
> > The native styling is way to dark for us atm so we'll use the .mail-toolbox 
> > background:
> > .inline-toolbar {
> >   -moz-appearance: none;
> > }
> Done. But we don't have an inline toolbar in our message header pane unlike
> Thunderbird.

Ah, right. primaryToolbar.css seems to be the place then.

I found another thing we could fix here:
.main-header-area could use the 0.6ex padding that thunderbird has. That could go in messageHeader.css. iow:

.main-header-area {
  padding: 0.6ex
}

Comment 35

[Philip Chee]

Attachment: Patch v1.6 More Mac Styling r=stefanh

>>> mailWindow1.css
>>> ---------------
>>> Remove the "border-bottom: 1px solid #A1A1A1;" from .tabmail-strip and add it 
>>> to #messengerBox (you can just insert it below #messengerWindow styles)
>> Done.
> 
> Sorry, it should be border-top for #messengerBox.
Fixed.

>>> messageHeader.css
>>> -----------------
>>> The native styling is way to dark for us atm so we'll use the .mail-toolbox 
>>> background:
>>> .inline-toolbar {
>>>   -moz-appearance: none;
>>> }
>> Done. But we don't have an inline toolbar in our message header pane unlike
>> Thunderbird.
> 
> Ah, right. primaryToolbar.css seems to be the place then.
Moved.

> I found another thing we could fix here:
> .main-header-area could use the 0.6ex padding that thunderbird has. That could go in messageHeader.css. iow:
> 
> .main-header-area {
>   padding: 0.6ex
> }
Fixed using ##msgHeaderView since we don't have the .main-header-area class.

Comment 36

[[e-mail]]

Comment on attachment 619327 [details] [diff] [review]
Patch v1.6 More Mac Styling r=stefanh

>+  var hasMenubar = toolbox.getElementsByAttribute("type", "menubar").length;
Nit: slightly more efficient to say
var menubar = toolbox.getElementsByAttribute("type", "menubar").item(0);

>+  if (!hasMenubar || !toolbars.length) {
Nit: Why bother checking for toolbars.length? If there are no toolbars, then there can be no menubar, can there?

Comment 37

[Philip Chee]

> Nit: Why bother checking for toolbars.length? If there are no toolbars, then there can
> be no menubar, can there?
menubars don't have a toolbarname attribute so the set of toolbars here is disjoint from the menubar.

Comment 38

[Stefan [:stefanh]]

(In reply to Philip Chee from comment #35) 
> > I found another thing we could fix here:
> > .main-header-area could use the 0.6ex padding that thunderbird has. That could go in messageHeader.css. iow:
> > 
> > .main-header-area {
> >   padding: 0.6ex
> > }
> Fixed using ##msgHeaderView since we don't have the .main-header-area class.

I don't have the cycles to try this, so I think you should leave this change to the future.

Comment 39

[Stefan [:stefanh]]

(I'm worried that it affects things I haven't tried)

Comment 40

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #38) 
> I don't have the cycles to try this

Oops, I didn't mean that I will never be able to try this, append "right now" to that.

Comment 41

[Philip Chee]

Attachment: Patch 1.7 as checked-in r=stefanh r=Neil

>>+  var hasMenubar = toolbox.getElementsByAttribute("type", "menubar").length;
> Nit: slightly more efficient to say
> var menubar = toolbox.getElementsByAttribute("type", "menubar").item(0);
Fixed.

>>> I found another thing we could fix here:
>>> .main-header-area could use the 0.6ex padding that thunderbird has. That could go in messageHeader.css. iow:
>>> 
>>> .main-header-area {
>>>   padding: 0.6ex
>>> }
>> Fixed using ##msgHeaderView since we don't have the .main-header-area class.
> 
> I don't have the cycles to try this, so I think you should leave this change to the future.
Reverted.

Comment 42

[Philip Chee]

Pushed to comm-central