Last Comment Bug 743629 - Implement the Australis tabs on Aero
: Implement the Australis tabs on Aero
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Theme (show other bugs)
: unspecified
: All Windows 7
: -- normal (vote)
: Thunderbird 15.0
Assigned To: Richard Marti (:Paenglab)
:
:
Mentors:
Depends on: 763308
Blocks: 733854
  Show dependency treegraph
 
Reported: 2012-04-09 02:44 PDT by Richard Marti (:Paenglab)
Modified: 2012-06-10 08:01 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (16.42 KB, patch)
2012-04-09 02:57 PDT, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
Patch in action (182.07 KB, image/png)
2012-04-09 03:03 PDT, Richard Marti (:Paenglab)
no flags Details
Patch v2 (14.66 KB, patch)
2012-04-10 10:15 PDT, Richard Marti (:Paenglab)
bugs: review-
bugs: ui‑review-
Details | Diff | Splinter Review
new tab graphics (4.42 KB, patch)
2012-04-17 04:44 PDT, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
Patch v3 (17.44 KB, patch)
2012-04-28 08:14 PDT, Richard Marti (:Paenglab)
bugs: review-
bugs: ui‑review-
Details | Diff | Splinter Review
Patch v4 (21.67 KB, patch)
2012-05-05 02:31 PDT, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
Patch v4.1 (21.84 KB, patch)
2012-05-05 07:08 PDT, Richard Marti (:Paenglab)
bugs: review+
bugs: ui‑review+
Details | Diff | Splinter Review
Patch for check-in (21.83 KB, patch)
2012-05-09 09:50 PDT, Richard Marti (:Paenglab)
richard.marti: review+
richard.marti: ui‑review+
Details | Diff | Splinter Review

Description Richard Marti (:Paenglab) 2012-04-09 02:44:12 PDT
This bug is for implementing the Australis tabs on Windows Vista/7.
Comment 1 Richard Marti (:Paenglab) 2012-04-09 02:57:20 PDT
Created attachment 613257 [details] [diff] [review]
Patch

This patch implements the Australis tabs for Vista and Win7. If everything is okay with this patch I'll implement it for all other platforms.

The only issue I saw was the tab overflow begins earlier than with the actual tabs. This comes because of the left/right margins of -14px to make the tabs superimpose. Without this negative margins the overflow works as before. I propose to file a new bug to solve this issue.
Comment 2 Richard Marti (:Paenglab) 2012-04-09 03:03:08 PDT
Created attachment 613259 [details]
Patch in action

On top my default theme, then Win7 default, Basic, Classic and on bottom High Contrast Nr. 1.

The first tab is selected, the second hovered and the third is inactive.
Comment 3 Sean Smith 2012-04-09 06:22:09 PDT
The background image in the tabs toolbar looks to hang over a pixel on the right and left.  You either need to add a 1px margin to both sides, or a 1px solid transparent border.  Of course, this becomes a moot point if they ever fix the bug to building the frame completely in XUL (bug 590945) because then you can more easily blend the frame to the tab toolbar.
Comment 4 Richard Marti (:Paenglab) 2012-04-10 10:15:14 PDT
Created attachment 613655 [details] [diff] [review]
Patch v2

Patch addressing comment 3 and small changes for easier implementation on other platforms.
Comment 5 Andreas Nilsson (:andreasn) 2012-04-16 09:10:04 PDT
Comment on attachment 613655 [details] [diff] [review]
Patch v2

The shapes of the tabs looks slightly off, and I think this is because of the graphics used. The graphics used in the design spec have a smoother curve. Will attach new graphics to this bug.

1. https://people.mozilla.com/~shorlander/files/australis-designSpecs/images-win7/tabActiveStart.png
Comment 6 Andreas Nilsson (:andreasn) 2012-04-17 04:44:14 PDT
Created attachment 615667 [details] [diff] [review]
new tab graphics

to be applied on top of Patch v2
Comment 7 Andreas Nilsson (:andreasn) 2012-04-17 06:05:34 PDT
Comment on attachment 613655 [details] [diff] [review]
Patch v2

setting r- here, as the tabs are not good enough yet, so I think the svg mask needs it's numbers tweaked a bit.
Comment 8 Richard Marti (:Paenglab) 2012-04-28 08:14:14 PDT
Created attachment 619301 [details] [diff] [review]
Patch v3

This patch is based on WIP patch for Firefox from Frank Yan (Bug 732583). I changed the images for active tabs to work also for Personas and Classic theme.
Comment 9 Andreas Nilsson (:andreasn) 2012-05-03 05:02:04 PDT
Comment on attachment 619301 [details] [diff] [review]
Patch v3

Woah, really nice tab rounding now!

Some nitpicks:
* Both the tab favicon and the close buttons are too far away from the edges of the tab. Looks like it's 14px instead of 10px
* The first tab is too far away from the left edge, 3px too much.
* The edges also needs some rounding, but that's outside the scope of this bug, so I filed bug #751527 about that.
* The color of the close button too weak.
* The hover of the close tab icon needs to be fixed to become red. The active state looks good.

Code looks good at quick glance, but giving r- there due to ui-review minus.
Comment 10 Richard Marti (:Paenglab) 2012-05-03 09:34:51 PDT
(In reply to Andreas Nilsson (:andreasn) from comment #9)
> * The first tab is too far away from the left edge, 3px too much.
> * The edges also needs some rounding, but that's outside the scope of this
> bug, so I filed bug #751527 about that.

This two are opposite needs for your desire :) When I remove the gap of 3px then the rounding will go into the tab.

I can change the .tabmail-tabs padding of now 3px to the value of the radius needed for the rounding. What do you think which radius would be good, 1.5px or 2px?
Comment 11 Richard Marti (:Paenglab) 2012-05-05 02:31:51 PDT
Created attachment 621270 [details] [diff] [review]
Patch v4

(In reply to Andreas Nilsson (:andreasn) from comment #9)
> Comment on attachment 619301 [details] [diff] [review]
> Patch v3
> 
> Some nitpicks:
> * Both the tab favicon and the close buttons are too far away from the edges
> of the tab. Looks like it's 14px instead of 10px

Fixed

> * The first tab is too far away from the left edge, 3px too much.

Removed the 3px padding. Needs to be re-added depending of the radius in bug 751527

> * The color of the close button too weak.
> * The hover of the close tab icon needs to be fixed to become red. The
> active state looks good.

I took now the close button from the design specs page. If the color is still to weak, the it's by design ;)
Comment 12 Richard Marti (:Paenglab) 2012-05-05 07:08:20 PDT
Created attachment 621295 [details] [diff] [review]
Patch v4.1

Improved inactive tab "splitter"
Comment 13 Andreas Nilsson (:andreasn) 2012-05-09 03:20:45 PDT
(In reply to Richard Marti [:paenglab] from comment #10)
> (In reply to Andreas Nilsson (:andreasn) from comment #9)
> > * The first tab is too far away from the left edge, 3px too much.
> > * The edges also needs some rounding, but that's outside the scope of this
> > bug, so I filed bug #751527 about that.
> 
> This two are opposite needs for your desire :) When I remove the gap of 3px
> then the rounding will go into the tab.

Indeed, I realized this when playing around a bit in DOMi. The spec page shows a rounding on the toolbar, so I think we should go for that too. I was looking at these two images [1] [2] that seems to have little spacing before the first page as well as rounded toolbar, but if that is too complex to implement in a sane way, then I think the rounded toolbar is more important.

1. https://people.mozilla.com/~shorlander/files/australis-designSpecs/images-win7/style-appTab-notifications.png
2. http://28.media.tumblr.com/tumblr_m0iwtyy6nh1qkoea4o1_1280.png
Comment 14 Andreas Nilsson (:andreasn) 2012-05-09 03:23:16 PDT
(In reply to Richard Marti [:paenglab] from comment #11)
> > * The color of the close button too weak.
> > * The hover of the close tab icon needs to be fixed to become red. The
> > active state looks good.
> 
> I took now the close button from the design specs page. If the color is
> still to weak, the it's by design ;)

Looking at it a second time, you're right. I was looking at it using a dark background, and the mockups are probably supposed to use a light background.
Comment 15 Andreas Nilsson (:andreasn) 2012-05-09 03:24:55 PDT
Comment on attachment 621295 [details] [diff] [review]
Patch v4.1

Setting ui-review+ as all my nitpicks have been addressed or explained.
Comment 16 Andreas Nilsson (:andreasn) 2012-05-09 03:32:42 PDT
So one thing that crossed my mind, but that I forgot to mention in the review is that the spec specifies that the tabs should be 180px wide, but I think that it would be good to keep our slightly wider tabs, as it will be good to still be able to fit the title "Inbox - name@domain.com" all right. Plus the fact that I assume Thunderbird don't tend to have as many tabs as Firefox open at the same time. Therefore we can afford the extra ~20px(?).
Comment 17 Andreas Nilsson (:andreasn) 2012-05-09 03:36:44 PDT
Comment on attachment 621295 [details] [diff] [review]
Patch v4.1

...and in this comment I set the ui-review flag instead of the code-review flag I accidentally set previously. I also hear there is coffee in the kitchen now. :)
Comment 18 Andreas Nilsson (:andreasn) 2012-05-09 03:57:30 PDT
Comment on attachment 621295 [details] [diff] [review]
Patch v4.1

> a/mail/themes/qute/mail/tabmail-aero.css line 74
> .tabmail-tab[selected] {
> position: relative;
> z-index: 100;
> text-shadow: none;
> }

and

> a/mail/themes/qute/mail/tabmail-aero.css line 287
> .tab-close-button {
> ...
> z-index: 150;
> }

Why the high z-index number here?
Everything else looks good, so code-review+ with that explained!
Comment 19 Richard Marti (:Paenglab) 2012-05-09 05:50:54 PDT
I checked m-c and c-c what other values are used and found from 1 to 1000050. I decided to use secure values with 100 and 150. I can try values like 10 and 15 if you prefer them. Or which values do you think would fit better?
Comment 20 Richard Marti (:Paenglab) 2012-05-09 09:50:01 PDT
Created attachment 622400 [details] [diff] [review]
Patch for check-in

I've set now a z-index of 1 and 2 and it still works.
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-05-09 15:34:59 PDT
https://hg.mozilla.org/comm-central/rev/e0829842407d

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