Last Comment Bug 763308 - Implement Australis tabs with Firefox tab construct
: Implement Australis tabs with Firefox tab construct
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 16.0
Assigned To: Richard Marti (:Paenglab)
:
Mentors:
Depends on: 776349
Blocks: 743629 753943 753954 754035 755793 767162
  Show dependency treegraph
 
Reported: 2012-06-10 07:59 PDT by Richard Marti (:Paenglab)
Modified: 2012-10-28 06:11 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
WIP patch (72.88 KB, patch)
2012-06-10 08:12 PDT, Richard Marti (:Paenglab)
no flags Details | Diff | Review
double border (8.88 KB, image/png)
2012-06-12 09:08 PDT, Andreas Nilsson (:andreasn)
no flags Details
patch (72.55 KB, patch)
2012-06-15 09:32 PDT, Richard Marti (:Paenglab)
no flags Details | Diff | Review
tab colors (18.47 KB, image/png)
2012-06-19 09:18 PDT, Andreas Nilsson (:andreasn)
no flags Details
patch v2 (72.55 KB, patch)
2012-06-19 10:55 PDT, Richard Marti (:Paenglab)
mconley: review+
bugs: ui‑review+
Details | Diff | Review
Patch for check-in addressing comment (73.47 KB, patch)
2012-06-20 12:22 PDT, Richard Marti (:Paenglab)
richard.marti: review+
richard.marti: ui‑review+
standard8: approval‑comm‑aurora+
Details | Diff | Review

Description Richard Marti (:Paenglab) 2012-06-10 07:59:30 PDT
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.
Comment 1 Richard Marti (:Paenglab) 2012-06-10 08:12:44 PDT
Created attachment 631749 [details] [diff] [review]
WIP patch

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.
Comment 2 Andreas Nilsson (:andreasn) 2012-06-12 09:08:06 PDT
Created attachment 632282 [details]
double border

The patch running on light Linux themes (clearlooks) seems to generate a double border.
Comment 3 Richard Marti (:Paenglab) 2012-06-13 06:18:05 PDT
(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?
Comment 4 Richard Marti (:Paenglab) 2012-06-15 09:32:08 PDT
Created attachment 633558 [details] [diff] [review]
patch

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.
Comment 5 Andreas Nilsson (:andreasn) 2012-06-19 09:18:28 PDT
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.
Comment 6 Richard Marti (:Paenglab) 2012-06-19 10:55:03 PDT
Created attachment 634501 [details] [diff] [review]
patch v2

(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?
Comment 7 Andreas Nilsson (:andreasn) 2012-06-20 05:15:36 PDT
Comment on attachment 634501 [details] [diff] [review]
patch v2

Yup, that's good.
Tested on mac+linux+windows and looks good!
Comment 8 Mike Conley (:mconley) - (Away until June 29th) 2012-06-20 08:04:04 PDT
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%.
Comment 9 Richard Marti (:Paenglab) 2012-06-20 09:18:34 PDT
(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 10 Mike Conley (:mconley) - (Away until June 29th) 2012-06-20 12:10:20 PDT
Comment on attachment 634501 [details] [diff] [review]
patch v2

Okie doke, r=me with those few nits fixed. Great job!
Comment 11 Richard Marti (:Paenglab) 2012-06-20 12:22:26 PDT
Created attachment 635007 [details] [diff] [review]
Patch for check-in addressing comment

Fixed the review comments.

Carrying over r+ and ui-r+ from previous patch
Comment 12 Richard Marti (:Paenglab) 2012-06-20 12:48:34 PDT
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.
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-06-20 18:47:25 PDT
https://hg.mozilla.org/comm-central/rev/de9b3a5e963a
Comment 14 Joe Sabash [:JoeS1] 2012-06-21 17:01:27 PDT
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.
Comment 15 Joe Sabash [:JoeS1] 2012-06-22 06:36:47 PDT
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.
Comment 16 Mike Conley (:mconley) - (Away until June 29th) 2012-07-16 08:40:57 PDT
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/875fc6a6a37a

Note You need to log in before you can comment on or make changes to this bug.