721327 - Implement Tabs Toolbar for Thunderbird and Lightning Compatibility.

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Philip Chee]

Lighting moved their tabbar buttons into the new Thunderbird tabbar-toolbar. We should somehow support this.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Proposed fix.

> +++ b/suite/themes/modern/messenger/mailWindow1.css
> 
> +.tabbar-toolbar {
> +  border-bottom: 1px solid #000000;
> +  min-height: 0;
> +}
Couldn't find out why the bottom border disappeared so I added this hack.

Comment 2

[[e-mail]]

(In reply to Philip Chee from comment #1)
> > +.tabbar-toolbar {
> > +  border-bottom: 1px solid #000000;
> > +  min-height: 0;
> > +}
> Couldn't find out why the bottom border disappeared so I added this hack.
It's because the toolbar isn't transparent. It probably needs to be. I don't know if toolbars are transparent under Aero; they do have -moz-appearance and it might or might not need to be turned off, as well as the border, for which I'd prefer separate bottom/top border styles please.

Comment 3

[Philip Chee]

Attachment: Patch v1.1 Better fix.

>> Couldn't find out why the bottom border disappeared so I added this hack.
> It's because the toolbar isn't transparent. It probably needs to be. I don't

Aha. Setting |background-color: transparent;| works.

> know if toolbars are transparent under Aero; they do have -moz-appearance
> and it might or might not need to be turned off,
Yeah I needed to turn off -moz-appearance for aero as I did in the v1 patch.

> as well as the border, for
> which I'd prefer separate bottom/top border styles please.
Done!

----------------------------------------------------------

Stefanh I don't have a Mac so I'm totally guessing here. Please recommend if
any styles are needed at all.

> +++ b/suite/themes/classic/mac/messenger/mailWindow1.css

> +
> +.tabbar-toolbar {
> +  -moz-appearance: none;
> +}

Comment 4

[Philip Chee]

> but I have one question: why not use the id selector?

I might want to reuse the class? Also:

http://csswizardry.com/2011/09/writing-efficient-css-selectors/

It is important to note that, although an ID is technically faster and more performant, it is barely so. Using Steve Souders’ CSS Test Creator we can see that an ID selector and a class selector show very little difference in reflow speed.

In Firefox 6 on a Windows machine I get an average reflow figure of 10.9 for a simple class selector. An ID selector gave a mean of 12.5, so this actually reflowed slower than a class.

Comment 5

[Stefan [:stefanh]]

Comment on attachment 596969 [details] [diff] [review]
Patch v1.1 Better fix.

Looks fine to me on Mac. But you should add 'nowindowdrag="true"' to the toolbar in messenger.xul. I'm pretty sure we don't want the transparent toolbar to be draggable ;-)

Actually, not using the nowindowdrag attribute made me spot an error in toolbar.css - we give the 'xpfe="false"' toolbars a grippy under certain circumstances (I'll fix that in a separate bug).

Re the class usage, I don't think there's any perf issues with using a class (even though I'm sceptical to the reflow results you're refering to), but I think it's confusing using a class on a unique element with an id attribute. Also, using a class forces you to add extra code, but you have the id for free.

Comment 6

[Philip Chee]

Attachment: Patch v1.2 Use id selector in CSS. ui-r=stefanh.

> Looks fine to me on Mac. But you should add 'nowindowdrag="true"' to the
> toolbar in messenger.xul. I'm pretty sure we don't want the transparent
> toolbar to be draggable ;-)
Fixed.

> Re the class usage, I don't think there's any perf issues with using a class
> (even though I'm sceptical to the reflow results you're refering to), but I
> think it's confusing using a class on a unique element with an id attribute.
> Also, using a class forces you to add extra code, but you have the id for
> free.
Fixed.

Comment 7

[[e-mail]]

Comment on attachment 597812 [details] [diff] [review]
Patch v1.2 Use id selector in CSS. ui-r=stefanh.

>+  min-height: 0;
Don't pinstripe/Modern have a min-height that you need to override too?

>+@media (-moz-windows-theme: aero) {
There's no @media block in toolbar.css so I'd remove it for consistency.

Comment 8

[Stefan [:stefanh]]

(In reply to [e-mail] from comment #7)
> Comment on attachment 597812 [details] [diff] [review]
> Patch v1.2 Use id selector in CSS. f=stefanh.
> 
> >+  min-height: 0;
> Don't pinstripe/Modern have a min-height that you need to override too?

Ah, that's true.

Comment 9

[Philip Chee]

Attachment: Patch v1.3 Fix More Nits ui-r=stefanh.

>>+  min-height: 0;
> Don't pinstripe/Modern have a min-height that you need to override too?
Fixed.

>>+@media (-moz-windows-theme: aero) {
> There's no @media block in toolbar.css so I'd remove it for consistency.
Fixed.

Comment 10

[[e-mail]]

Comment on attachment 598189 [details] [diff] [review]
Patch v1.3 Fix More Nits ui-r=stefanh.

>+#tabbar-toolbar {
>+  min-height: 0;
>+  background-color: transparent;
>+}
One last nit, put the min-height last for consistency with the other themes.

Comment 11

[Philip Chee]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/a35da4eb6892

Comment 12

[Philip Chee]

Oops forgot to close this bug.

Comment 13

[Philip Chee]

Comment on attachment 598189 [details] [diff] [review]
Patch v1.3 Fix More Nits ui-r=stefanh.

[Approval Request Comment]
Regression caused by ): Lighting Bug 709572 - Add Calendar and Task toolbar for tabs on top 
User impact if declined: User will be unable to open a Calendar or Tasks tab even from the menu.
Testing completed (on m-c, etc.): Baked and tested on comm-central.
Risk to taking this patch (and alternatives if risky): low risk. Alternative is to write an addon but then we'd need to localize that addon too.
String changes made by this patch: Two additional strings.
 +<!ENTITY showTabsToolbarCmd.label "Tabs Toolbar">
 +<!ENTITY showTabsToolbarCmd.accesskey "T">

Comment 14

[Justin Wood (:Callek)]

Comment on attachment 598189 [details] [diff] [review]
Patch v1.3 Fix More Nits ui-r=stefanh.

Ratty seems to have a patch (from a convo in IRC) that would make this useable on beta/aurora without l10n changes.

Has a slight code-change to make it sane, but if that can pass a review I give it pre-emptive a+

Comment 15

[Philip Chee]

To summarize a IRC conversation with Callek a L10n safe patch for branches.

1. Remove the toolbarname and accesskey, thus removing the need for L10n entities.

2. Then modify utilityOverlay->onViewToolbarsPopupShowing() to take into account external toolbars without the "toolbarname" attribute.

replace:
  var toolbars = Array.slice(toolbox.getElementsByAttribute("toolbarname", "*"));
  toolbars = toolbars.concat(toolbox.externalToolbars);

with:
  var externalToolbars = Array.filter(toolbox.externalToolbars,
                                      function(toolbar) {
                                        return toolbar.hasAttribute("toolbarname")});
  var toolbars = Array.slice(toolbox.getElementsByAttribute("toolbarname", "*"));
  toolbars = toolbars.concat(externalToolbars);

Comment 16

[Philip Chee]

Attachment: Patch v2.1 L10n safe Branch Patch.

Not tested on Aurora/Beta. But works on trunk.

Comment 17

[Justin Wood (:Callek)]

Comment on attachment 600988 [details] [diff] [review]
Patch v2.1 L10n safe Branch Patch.

Neil can you do a fast turnaround here, its just a branch version of what you reviewed already, but has js changes not in original.

Comment 18

[Philip Chee]

I should add that the js changes are needed on trunk as well to take into account third party external toolbars (from addons) that don't bother to have toolbarname attributes.

Comment 19

[[e-mail]]

Comment on attachment 600988 [details] [diff] [review]
Patch v2.1 L10n safe Branch Patch.

I don't see why we need to make the toolbar customisable on branch. I'm not even sure why we need it on trunk; the calendar buttons aren't removable, and it doesn't make sense to change the settings for the toolbar. At least on trunk the l10n changes allow us to hide the toolbar completely...

Comment 20

[Philip Chee]

Attachment: Patch v2.2 Branch patch with non-customizable toolbar.

> I don't see why we need to make the toolbar customisable on branch. I'm not
> even sure why we need it on trunk; the calendar buttons aren't removable,
> and it doesn't make sense to change the settings for the toolbar. At least
> on trunk the l10n changes allow us to hide the toolbar completely...

The Thunderbird tabbar-toolbar which Lightning inserts it's button into is
customizable and is meant to be a replacement for the non-customizable
<box id="tabmail-buttons">.

The intention of TB is to get extensions to migrate any UI they insert in the
latter to the toolbar.

IIRC The lightning toolbar buttons are deliberately non-customizable.

Comment 21

[Justin Wood (:Callek)]

Comment on attachment 602288 [details] [diff] [review]
Patch v2.2 Branch patch with non-customizable toolbar.

[Triage Comment]

Comment 22

[Philip Chee]

Pushed to branches.
http://hg.mozilla.org/releases/comm-aurora/rev/80ddee7da026
http://hg.mozilla.org/releases/comm-beta/rev/18c5e81531db