Last Comment Bug 712938 - Polish tabs-on-top theme on Mac OS X
: Polish tabs-on-top theme on Mac OS X
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: Thunderbird 12.0
Assigned To: Florian Quèze [:florian] [:flo]
:
Mentors:
Depends on:
Blocks: tb-tabsontop
  Show dependency treegraph
 
Reported: 2011-12-22 06:57 PST by Florian Quèze [:florian] [:flo]
Modified: 2012-01-12 07:03 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Suggested changes (1.64 KB, patch)
2011-12-22 06:57 PST, Florian Quèze [:florian] [:flo]
bwinton: ui‑review+
bugs: feedback-
Details | Diff | Splinter Review
Screnshot before/after applying the patch (52.92 KB, image/png)
2011-12-22 07:03 PST, Florian Quèze [:florian] [:flo]
no flags Details
patch using personas (191.91 KB, image/png)
2011-12-22 11:45 PST, Andreas Nilsson (:andreasn)
no flags Details
Patch v2 (2.86 KB, patch)
2012-01-09 02:53 PST, Florian Quèze [:florian] [:flo]
mconley: review+
mconley: ui‑review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review
Icons are not vertically centered with tab titles (18.79 KB, image/png)
2012-01-09 12:57 PST, Mike Conley (:mconley) - (Needinfo me!)
no flags Details
Firefox tabs for reference (12.53 KB, image/png)
2012-01-09 12:57 PST, Mike Conley (:mconley) - (Needinfo me!)
no flags Details
Screenshot (31.46 KB, image/png)
2012-01-09 13:12 PST, Florian Quèze [:florian] [:flo]
no flags Details
Patch for Aurora (2.93 KB, patch)
2012-01-11 09:51 PST, Mike Conley (:mconley) - (Needinfo me!)
florian: feedback+
Details | Diff | Splinter Review

Description Florian Quèze [:florian] [:flo] 2011-12-22 06:57:19 PST
Created attachment 583785 [details] [diff] [review]
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] [:flo] 2011-12-22 07:03:51 PST
Created attachment 583787 [details]
Screnshot before/after applying the patch
Comment 2 Mike Conley (:mconley) - (Needinfo me!) 2011-12-22 07:10:30 PST
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) (:☕️) 2011-12-22 07:38:27 PST
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] [:flo] 2011-12-22 07:58:14 PST
(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) 2011-12-22 11:45:24 PST
Created attachment 583882 [details]
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) 2011-12-22 11:53:10 PST
Comment on attachment 583785 [details] [diff] [review]
Suggested changes

feedback- due to persona theme issue.
Comment 7 Nomis101 2011-12-28 16:59:30 PST
(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) - (Needinfo me!) 2011-12-29 06:15:20 PST
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) - (Needinfo me!) 2011-12-30 07:40:04 PST
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] [:flo] 2012-01-09 02:53:38 PST
Created attachment 586940 [details] [diff] [review]
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) 2012-01-09 09:36:47 PST
(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] [:flo] 2012-01-09 10:05:48 PST
(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) - (Needinfo me!) 2012-01-09 10:52:47 PST
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] [:flo] 2012-01-09 12:48:18 PST
(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) - (Needinfo me!) 2012-01-09 12:57:13 PST
Created attachment 587091 [details]
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) - (Needinfo me!) 2012-01-09 12:57:54 PST
Created attachment 587092 [details]
Firefox tabs for reference
Comment 17 Florian Quèze [:florian] [:flo] 2012-01-09 13:12:23 PST
Created attachment 587098 [details]
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) - (Needinfo me!) 2012-01-09 13:26:50 PST
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) - (Needinfo me!) 2012-01-09 13:31:06 PST
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) - (Needinfo me!) 2012-01-09 13:33:37 PST
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) - (Needinfo me!) 2012-01-10 07:01:37 PST
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) - (Needinfo me!) 2012-01-10 07:02:16 PST
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/e89709934a86
Comment 23 Mike Conley (:mconley) - (Needinfo me!) 2012-01-10 07:02:31 PST
Comment on attachment 586940 [details] [diff] [review]
Patch v2

We'll want this for TB 11
Comment 24 Mike Conley (:mconley) - (Needinfo me!) 2012-01-11 09:51:59 PST
Created attachment 587737 [details] [diff] [review]
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] [:flo] 2012-01-12 02:34:05 PST
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) - (Needinfo me!) 2012-01-12 06:20:18 PST
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/4326aa65d49a

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