Closed Bug 712938 Opened 8 years ago Closed 8 years ago
Polish tabs-on-top theme on Mac OS X
52.92 KB, image/png
191.91 KB, image/png
2.86 KB, patch
|Details | Diff | Splinter Review|
18.79 KB, image/png
12.53 KB, image/png
31.46 KB, image/png
2.93 KB, patch
|Details | Diff | Splinter Review|
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 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 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.)
Attachment #583785 - Flags: ui-review?(bwinton) → ui-review+
(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 :-/.
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 on attachment 583785 [details] [diff] [review] Suggested changes feedback- due to persona theme issue.
Attachment #583785 - Flags: feedback?(nisses.mail) → feedback-
(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.
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
Andreas: Do we / can we do any sort of multiplexing between Lion and the other versions of OSX? -Mike
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.
(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)
(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.
This latest patch looks pretty good - though the tab icon and title still don't appear to be vertically aligned with one another...
(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.
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.
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?
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?
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 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
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
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/e89709934a86
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 586940 [details] [diff] [review] Patch v2 We'll want this for TB 11
Attachment #586940 - Flags: approval-comm-aurora?
Assignee: nobody → florian
Target Milestone: --- → Thunderbird 12.0
Attachment #586940 - Flags: approval-comm-aurora? → approval-comm-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
Attachment #587737 - Flags: feedback?(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!
Attachment #587737 - Flags: feedback?(florian) → feedback+
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/4326aa65d49a
You need to log in before you can comment on or make changes to this bug.