712938 - Polish tabs-on-top theme on Mac OS X

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Florian Quèze [:florian]]

Attachment: Suggested changes

After trying a build with Tabs on Top on Mac OS X 10.5.8, I've identified to things that I think could be polished:
- the tab icons and the tab titles aren't vertically centered with each other
- background of the bottom of the selected tab doesn't match the top of the mail toolbar, especially when the window is inactive.

The attached patch is only a suggestion. I created it by comparing the Mac themes of Thunderbird and Instantbird with DOM inspector to identify relevant differences.

I think you will want to take the .tab-text margin-top change. I'm not sure about the background changes: they look better on my Mac OS X 10.5, but that may not be the case on newer versions of the OS.

Comment 1

[Florian Quèze [:florian]]

Attachment: Screnshot before/after applying the patch

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 583785 [details] [diff] [review]
Suggested changes

I think Andreas would be better equipped to give feedback on these changes.  Also ui-r'ing for bwinton.  The screenshots look good!

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 583785 [details] [diff] [review]
Suggested changes

It's an improvement, but I think I'ld prefer to push the icon down a little, instead of pulling the text up a little…

(We have a lot of space under the tab, and not that much above it.)

Comment 4

[Florian Quèze [:florian]]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #3)
> Comment on attachment 583785 [details] [diff] [review]
> Suggested changes
> 
> It's an improvement, but I think I'ld prefer to push the icon down a little,
> instead of pulling the text up a little…
> 
> (We have a lot of space under the tab, and not that much above it.)

I also feel that there's more (unused) space under the tab than at the top of it. I think the cause of the excessive space under the tab is that the tab bar is 1px higher on Thunderbird than on Firefox/Instantbird. The XUL structure isn't exactly the same so I haven't managed to identify quickly with DOMi what causes this height difference though :-/.

Comment 5

[Andreas Nilsson (:andreasn)]

Attachment: patch using personas

With this patch applied, I get a odd looking tab look for the persona, but the regular theme looks all right. This is on Lion.
Without the patch I don't see the oddness is in the screenshot you posted, so I think that is specific to Leopard.

Comment 6

[Andreas Nilsson (:andreasn)]

Comment on attachment 583785 [details] [diff] [review]
Suggested changes

feedback- due to persona theme issue.

Comment 7

[Nomis101]

(In reply to Florian Quèze from comment #1)
> Created attachment 583787 [details]
> Screnshot before/after applying the patch

Nice to see that the look on OS X will pe polished. But interestingly for me the tabs in the recent daily build doesn't look to me as in your screenshot (the "before" pictures). Do you have applied any additional patches?
http://forums.mozillazine.org/viewtopic.php?f=29&t=2394743&p=11590463#p11590463
This is the newest daily on Lion.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Hey Nomis101,

What you're seeing is a result of Bug 497995 landing in trunk.  This bug changed some moz-prefixed border-radius rules, and made our tabs look a bit funny. A fix is being worked on in Bug 713852.

All the best,

-Mike

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Andreas:

Do we / can we do any sort of multiplexing between Lion and the other versions of OSX?

-Mike

Comment 10

[Florian Quèze [:florian]]

Attachment: Patch v2

New version of the patch with only margin changes (I've left out the color changes as I can't test on Lion). I tweaked them until the appearance matches the tabs of Firefox 9.

Not sure if this needs ui-review from Blake and feedback from Andreas again.

Comment 11

[Andreas Nilsson (:andreasn)]

(In reply to Mike Conley (:mconley) from comment #9)
> Andreas:
> 
> Do we / can we do any sort of multiplexing between Lion and the other
> versions of OSX?

Yes, looking at the Firefox browser.css, it seems like we can use @media (-moz-mac-lion-theme)

Comment 12

[Florian Quèze [:florian]]

(In reply to Andreas Nilsson (:andreasn) from comment #11)
> (In reply to Mike Conley (:mconley) from comment #9)
> > Andreas:
> > 
> > Do we / can we do any sort of multiplexing between Lion and the other
> > versions of OSX?
> 
> Yes, looking at the Firefox browser.css, it seems like we can use @media
> (-moz-mac-lion-theme)

As these variations of grey are made mostly with a variation of opacity without specifying the color directly, I think it should be possible to find a solution that doesn't require a special case per OS version. I haven't tested with a Persona at all, it's possible that with a persona my initial patch would look bad on 10.5 too.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

This latest patch looks pretty good - though the tab icon and title still don't appear to be vertically aligned with one another...

Comment 14

[Florian Quèze [:florian]]

(In reply to Mike Conley (:mconley) from comment #13)
> This latest patch looks pretty good - though the tab icon and title still
> don't appear to be vertically aligned with one another...

The icon is vertically bigger than the label, so I don't really see what you mean here. Except if you are seeing something different than what I see on my build, the alignment of the icon is the same as on Firefox.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Icons are not vertically centered with tab titles

Here we can see that the icons are not vertically aligned, where the vertical center of the icon matches the vertical center of the tab title.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Firefox tabs for reference

Comment 17

[Florian Quèze [:florian]]

Attachment: Screenshot

Here's how things look for me, on both Thunderbird (with attachment 586940 [details] [diff] [review] applied) and Firefox.
Neither of them look like on your screenshot. Do you have a Firefox add-on installed that could be changing the height of your tab bar?

Comment 18

[Mike Conley (:mconley) (:⚙️)]

I assume you meant a Thunderbird add-on. :)  I tried with a fresh profile, and the problem appears to persist.

Andreas, are you able to reproduce?

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Hm - so I may have tracked this down to a build problem.  DOM Inspector suggests that some of your CSS changes aren't taking.

Lemme do a fresh build.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 586940 [details] [diff] [review]
Patch v2

Ok, built it again, and my problems have vanished.  Sorry about that!

Code looks good, and I'm happy with the result!  Thanks for your work,

-Mike

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Florian:

Thanks for committing!  For future reference, once you've committed a patch, you need to comment in the bug with a link to the commit, and then you can mark the bug as RESOLVED FIXED.

Thanks,

-Mike

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Committed to comm-central as http://hg.mozilla.org/comm-central/rev/e89709934a86

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 586940 [details] [diff] [review]
Patch v2

We'll want this for TB 11

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch for Aurora

Florian:

I just tried bringing this into Aurora, and it looks like there's a conflict.

Comm-central received http://hg.mozilla.org/comm-central/rev/4c03954be2d1 after the behaviour of border-image changed.  Aurora did not.

I've attached a patch that puts your changes in, but can you confirm that they do what they're supposed to do?

Thanks,

-Mike

Comment 25

[Florian Quèze [:florian]]

Comment on attachment 587737 [details] [diff] [review]
Patch for Aurora

(In reply to Mike Conley (:mconley) from comment #24)
> Created attachment 587737 [details] [diff] [review]
> Patch for Aurora

> I've attached a patch that puts your changes in, but can you confirm that
> they do what they're supposed to do?

I unpacked omni.ja of the latest aurora build and applied your patch, I can confirm that the changes work as expected. Thanks for taking care of this!

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/4326aa65d49a