Closed Bug 763308 Opened 12 years ago Closed 12 years ago

Implement Australis tabs with Firefox tab construct

Categories

(Thunderbird :: Toolbars and Tabs, defect)

defect
Not set
normal

Tracking

(thunderbird15 fixed)

RESOLVED FIXED
Thunderbird 16.0
Tracking Status
thunderbird15 --- fixed

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(3 files, 3 obsolete files)

We should make the tab construct like the one from Firefox. Firefox is using a stack in the tab to separate the background (for the tab shape) and the content (for the icon, the text and the close button).

With this we can style the Australis tabs similar to Fx. Also the tab shrink and overflow is working correctly.
Attached patch WIP patch (obsolete) — Splinter Review
Patch which implements the Fx construct and styles the XP and OSX tabs. It is also needed to adapt the already implemented Aero and Linux tabs.

This patch also implements on all platform the correct click area. Also changed is the tab width of 250px to 210px. The mockup is using 180px but is to small for a standard tab like "Inbox - Local Folders" in for example German.

Before I'm asking for review I'm awaiting the result of the review of the last patch in bug 754035 which is the same as this bug but only for OSX.
Attached image double border
The patch running on light Linux themes (clearlooks) seems to generate a double border.
(In reply to Andreas Nilsson (:andreasn) from comment #2)
> Created attachment 632282 [details]
> double border
> 
> The patch running on light Linux themes (clearlooks) seems to generate a
> double border.

I added a border for the toolbox because under all Ubuntu 12.04 themes there is no border. What should I do, remove or leave it?
Attached patch patch (obsolete) — Splinter Review
Patch for review after the positive feedback of the OSX patch.

I removed under Linux again the tabbar bottom border. Now it's the same as already in trunk.

I took mconley for review to check my XUL addition and naturally the CSS.
Assignee: nobody → richard.marti
Attachment #631749 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #633558 - Flags: ui-review?(nisses.mail)
Attachment #633558 - Flags: review?(mconley)
Attached image tab colors
On Linux, compared to before, there is a hue difference between the top of the toolbar and the tab borders. This looks all right in the default Ubuntu theme (due to the dark area behind it), but odd on Fedora that uses Clearlooks.
Attached patch patch v2 (obsolete) — Splinter Review
(In reply to Andreas Nilsson (:andreasn) from comment #5)
> Created attachment 634465 [details]
> tab colors
> 
> On Linux, compared to before, there is a hue difference between the top of
> the toolbar and the tab borders. This looks all right in the default Ubuntu
> theme (due to the dark area behind it), but odd on Fedora that uses
> Clearlooks.

Okay gave on top of the tabs a less transparent white (.7 instead of .5). Is this better?
Attachment #633558 - Attachment is obsolete: true
Attachment #633558 - Flags: ui-review?(nisses.mail)
Attachment #633558 - Flags: review?(mconley)
Attachment #634501 - Flags: ui-review?(nisses.mail)
Attachment #634501 - Flags: review?(mconley)
Comment on attachment 634501 [details] [diff] [review]
patch v2

Yup, that's good.
Tested on mac+linux+windows and looks good!
Attachment #634501 - Flags: ui-review?(nisses.mail) → ui-review+
Comment on attachment 634501 [details] [diff] [review]
patch v2

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

Richard:

This looks awesome - I'm really happy with it. Just one or two nits, and a few questions.

Going to hold off on r+ until I hear your responses to my questions,

-Mike

::: mail/base/content/tabmail.css
@@ +34,5 @@
>    display: none;
>  }
>  
> +.tabmail-tabs:not([closebuttons="noclose"]):not([closebuttons="closeatend"]) >
> +.tabmail-tab[selected="true"]:not(:only-child) > .tab-stack > .tab-content > .tab-close-button {

Is this chain of child selectors really needed, or can we be more precise?  See https://developer.mozilla.org/en/Writing_Efficient_CSS#Use_the_most_specific_category_possible

@@ +39,5 @@
>    display: -moz-box;
>  }
>  
> +.tabmail-tabs[closebuttons="alltabs"] > .tabmail-tab:not(:only-child) >
> +.tab-stack > .tab-content > .tab-close-button {

Same as above. Can these be made more direct, or are we stuck with them?

::: mail/themes/gnomestripe/mail/icons/tabActiveMiddle.svg
@@ +1,1 @@
> +<svg

I've never really worked with svg's like this before, so maybe it's supposed to look like this.

Having said that, I'm not too wild about the formatting of this file. Kinda hard to read.

I'd prefer breaking up tag parameters like this:

<svg xmlns="http://www.w3.org/2000/svg"
     xmlns:xlink="http://www.w3.org/1999/xlink"
     width="6"
     height="31">

I'd also prefer two-space indentation as opposed to 1.

Same goes for the other svg files in here.

::: mail/themes/gnomestripe/mail/tabmail.css
@@ +22,5 @@
>    -moz-padding-start: 0px;
>  }
>  
> +.tabmail-tabs > .tabmail-tab:first-child > .tab-stack > .tab-content >
> +.tab-close-button {

Same as earlier - do we need this chain of child selectors? If we need them, fine - but if we can do without, we'd probably be better off.

There are a few other points in your patch where we see the same kind of chains, but I'm not going to mention it anymore.

@@ +61,5 @@
> +}
> +
> +.tabmail-tab:hover > .tab-stack > .tab-background:not([selected=true]),
> +.tab-background[selected=true] {
> +  background-repeat: no-repeat,

Three no-repeat's?  What does the third no-repeat give us? My knowledge of background-repeat is limited, but I'm pretty sure it only takes two values. Or am I wrong?

See https://developer.mozilla.org/en/CSS/background-repeat

(Unless the star in background-repeat: repeat-style[, repeat-style]* means "0 or more", which it might. The rest of the documentation doesn't really explain such a possibility.)

::: mail/themes/pinstripe/mail/primaryToolbar.css
@@ +24,5 @@
>  }
>  
>  .mail-toolbox:not(:-moz-lwtheme),
>  .contentTabToolbox:not(:-moz-lwtheme) {
> +  background-image: -moz-linear-gradient(hsl(0, 0%, 93%), hsl(0, 0%, 83%)98%,

Nit - space before 98%.
(In reply to Mike Conley (:mconley) from comment #8)
> Comment on attachment 634501 [details] [diff] [review]
> patch v2
> 
> Review of attachment 634501 [details] [diff] [review]:
> > +.tabmail-tabs:not([closebuttons="noclose"]):not([closebuttons="closeatend"]) >
> > +.tabmail-tab[selected="true"]:not(:only-child) > .tab-stack > .tab-content > .tab-close-button {
> 
> Is this chain of child selectors really needed, or can we be more precise? 
> See
> https://developer.mozilla.org/en/
> Writing_Efficient_CSS#Use_the_most_specific_category_possible

This is the most specific selector as every level is in it.

.tabmail-tabs:not([closebuttons="noclose"]):not([closebuttons="closeatend"]) >
> > +.tabmail-tab[selected="true"]:not(:only-child) is needed and then it's the best to give every level in selector until .tab-close-button.

The removed code was already that specific but is shorter because the FX construct has two levels more.

> @@ +39,5 @@
> >    display: -moz-box;
> >  }
> >  
> > +.tabmail-tabs[closebuttons="alltabs"] > .tabmail-tab:not(:only-child) >
> > +.tab-stack > .tab-content > .tab-close-button {
> 
> Same as above. Can these be made more direct, or are we stuck with them?

A .tabmail-tabs[closebuttons="alltabs"] > .tabmail-tab:not(:only-child) .tab-close-button { would also work but is less specific and slower.

> ::: mail/themes/gnomestripe/mail/icons/tabActiveMiddle.svg
> @@ +1,1 @@
> > +<svg
> 
> I've never really worked with svg's like this before, so maybe it's supposed
> to look like this.
> 
> Having said that, I'm not too wild about the formatting of this file. Kinda
> hard to read.
> 
> I'd prefer breaking up tag parameters like this:
> 
> <svg xmlns="http://www.w3.org/2000/svg"
>      xmlns:xlink="http://www.w3.org/1999/xlink"
>      width="6"
>      height="31">
> 
> I'd also prefer two-space indentation as opposed to 1.
> 
> Same goes for the other svg files in here.

I can do this for the check-in patch when no other issues are in this patch.

> ::: mail/themes/gnomestripe/mail/tabmail.css
> @@ +22,5 @@
> >    -moz-padding-start: 0px;
> >  }
> >  
> > +.tabmail-tabs > .tabmail-tab:first-child > .tab-stack > .tab-content >
> > +.tab-close-button {
> 
> Same as earlier - do we need this chain of child selectors? If we need them,
> fine - but if we can do without, we'd probably be better off.

I copied mostly all from Jared's patch and I would say he made this all with focus on speed

> There are a few other points in your patch where we see the same kind of
> chains, but I'm not going to mention it anymore.
> 
> @@ +61,5 @@
> > +}
> > +
> > +.tabmail-tab:hover > .tab-stack > .tab-background:not([selected=true]),
> > +.tab-background[selected=true] {
> > +  background-repeat: no-repeat,
> 
> Three no-repeat's?  What does the third no-repeat give us? My knowledge of
> background-repeat is limited, but I'm pretty sure it only takes two values.
> Or am I wrong?

Where are three background images and the three no-repeat are for each of them. Only one would also work (I'd need to test it to be 100% sure) but Jared and Frank used the three so maybe it's also a speed thing.

> ::: mail/themes/pinstripe/mail/primaryToolbar.css
> @@ +24,5 @@
> >  }
> >  
> >  .mail-toolbox:not(:-moz-lwtheme),
> >  .contentTabToolbox:not(:-moz-lwtheme) {
> > +  background-image: -moz-linear-gradient(hsl(0, 0%, 93%), hsl(0, 0%, 83%)98%,
> 
> Nit - space before 98%.

I'll fix it with the check-in patch.
Comment on attachment 634501 [details] [diff] [review]
patch v2

Okie doke, r=me with those few nits fixed. Great job!
Attachment #634501 - Flags: review?(mconley) → review+
Fixed the review comments.

Carrying over r+ and ui-r+ from previous patch
Attachment #634501 - Attachment is obsolete: true
Attachment #635007 - Flags: ui-review+
Attachment #635007 - Flags: review+
Keywords: checkin-needed
Comment on attachment 635007 [details] [diff] [review]
Patch for check-in addressing comment

[Approval Request Comment]
Blake thought it would be good when this patch lands also on Aurora.

I see no risk landing it, the XUL changes are small and most of the CSS is already on Aero/Linux. The advantage would be that the new tabs are on all platforms than only Aero and Linux.
Attachment #635007 - Flags: approval-comm-aurora?
https://hg.mozilla.org/comm-central/rev/de9b3a5e963a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
Blocks: 767162
I'm not seeing any signs of this checkin using:
Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/16.0 Thunderbird/16.0a1 ID:20120621030537
Winxp
No curves, no bg image, really no changes at all.
OK for the 20120622030200 build in XP on my work laptop
Either it didn't make it into the 20120621 nightly or there is some kind of conflict on my home desktop.
Attachment #635007 - Flags: approval-comm-aurora? → approval-comm-aurora+
Depends on: 776349
You need to log in before you can comment on or make changes to this bug.