Last Comment Bug 581382 - Persona doesn't apply to sidebar
: Persona doesn't apply to sidebar
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Themes (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b1
Assigned To: Robert Kaiser
:
:
Mentors:
Depends on: 599621
Blocks: SM-lwtheme
  Show dependency treegraph
 
Reported: 2010-07-23 07:52 PDT by Pavel Cvrcek [:JasnaPaka]
Modified: 2010-09-25 08:21 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
SeaMonkey 2.1 with sidebar (59.40 KB, image/png)
2010-07-23 07:52 PDT, Pavel Cvrcek [:JasnaPaka]
no flags Details
Firefox 4.0 NB with Persona on sidebar (27.59 KB, image/png)
2010-07-23 07:55 PDT, Pavel Cvrcek [:JasnaPaka]
no flags Details
fix sidebar header(s) (1.80 KB, patch)
2010-09-16 18:15 PDT, Robert Kaiser
neil: review+
Details | Diff | Splinter Review
fix sidebar header(s), v1.1 (3.77 KB, patch)
2010-09-23 11:39 PDT, Robert Kaiser
neil: review+
stefanh: review+
Details | Diff | Splinter Review

Description Pavel Cvrcek [:JasnaPaka] 2010-07-23 07:52:02 PDT
Created attachment 459823 [details]
SeaMonkey 2.1 with sidebar

Persona should be used on sidebars too. At this moment I just see Persona on the right border. It doesn't look well.
Comment 1 Pavel Cvrcek [:JasnaPaka] 2010-07-23 07:55:35 PDT
Created attachment 459824 [details]
Firefox 4.0 NB with Persona on sidebar
Comment 2 Robert Kaiser 2010-09-16 18:15:47 PDT
Created attachment 476140 [details] [diff] [review]
fix sidebar header(s)

Here's a patch that should make sidebar look much better with personas, making the main header by styled with the persona, and removing the shadow from the individual headers.
Comment 3 neil@parkwaycc.co.uk 2010-09-17 02:56:13 PDT
Comment on attachment 459824 [details]
Firefox 4.0 NB with Persona on sidebar

Where's firefox's splitter? I can see the splitter in the SeaMonkey screen shot, and IMHO it looks really bad with a transparent background (I don't know why toolkit does that when Personas are active). What do you think?
Comment 4 neil@parkwaycc.co.uk 2010-09-17 14:06:58 PDT
I see now, Firefox manages to give its splitter a solid colour, but our splitter is transparent, so it shows bits of the underlying Persona. I'll file a separate bug for that.
Comment 5 Robert Kaiser 2010-09-17 14:15:18 PDT
Well, from what I see in the attached screen shot, their splitter is grippy-less 1px-wide version, so it's pretty different. Here on Linux, I see our slitter having a slid color even with my patch applied, but the effect in the screen shot is really looking strange, yes. If that's the common Windows experience with lwthemes, I'd be happy if someone on Windows can do a patch for that, as I can't test this here.
Comment 6 neil@parkwaycc.co.uk 2010-09-17 17:02:24 PDT
Comment on attachment 476140 [details] [diff] [review]
fix sidebar header(s)

>-#sidebar-panel-picker {
>+#sidebar-panel-picker:not(:-moz-lwtheme) {
This change isn't necessary, since the picker has to inherit the colour (rather than defaulting to -moz-dialogtext) whether or not a Persona is applied. r=me with that fixed.
Comment 7 Stefan [:stefanh] 2010-09-18 05:49:26 PDT
Comment on attachment 476140 [details] [diff] [review]
fix sidebar header(s)

This patch is missing the mac classic file.
Comment 8 Robert Kaiser 2010-09-22 13:58:34 PDT
(In reply to comment #7)
> Comment on attachment 476140 [details] [diff] [review]
> fix sidebar header(s)
> 
> This patch is missing the mac classic file.

Well, I know Mac sidebar styling is a bit different but I can't test if the same changes there would work. Can you try and tell me?

I still need to investigate Neil's comment #6 anyhow, as I remember I had a distinct reason for putting this on there, IIRC it actually had a wrong color in lwtheme, but I need to re-check. I'm using a dark lwtheme with light fonts for testing, that shows up font color problems. It could be that actually inheriting the light color on non-lwthemed headers is bad, but let me verify.
Comment 9 Robert Kaiser 2010-09-23 11:23:40 PDT
(In reply to comment #6)
> Comment on attachment 476140 [details] [diff] [review]
> fix sidebar header(s)
> 
> >-#sidebar-panel-picker {
> >+#sidebar-panel-picker:not(:-moz-lwtheme) {
> This change isn't necessary, since the picker has to inherit the colour (rather
> than defaulting to -moz-dialogtext) whether or not a Persona is applied. r=me
> with that fixed.

Actually, this is necessary, or else we'd need to do our own overrides of this for hover/active states. If we exclude this from lwtheme, then an inherit is set there and correctly overridden from there to not apply on hover/active when native styling takes over again. I added a comment to make that clear.
Comment 10 Robert Kaiser 2010-09-23 11:39:08 PDT
Created attachment 477999 [details] [diff] [review]
fix sidebar header(s), v1.1

Re-requesting review because I both didn't address your nit (see comment) and added Mac support (untested as I have no Mac) by just porting the same changes there. If there's anything more to do, I'll leave that to Mac people in a followup.
Comment 11 neil@parkwaycc.co.uk 2010-09-23 16:13:19 PDT
(In reply to comment #9)
> Actually, this is necessary, or else we'd need to do our own overrides of this
> for hover/active states. If we exclude this from lwtheme, then an inherit is
> set there and correctly overridden from there to not apply on hover/active when
> native styling takes over again. I added a comment to make that clear.
OMG that looks so fugly. How do Linux Firefox users cope with the fail?
Comment 12 Stefan [:stefanh] 2010-09-24 13:19:53 PDT
Comment on attachment 477999 [details] [diff] [review]
fix sidebar header(s), v1.1

+.box-texttab:-moz-lwtheme {
+  text-shadow: none;
+}
+

On mac, there's no text-shadow to override, so please remove the above rule.
Comment 13 Robert Kaiser 2010-09-25 07:00:27 PDT
(In reply to comment #12)
> On mac, there's no text-shadow to override, so please remove the above rule.

Interesting, I thought lwtheme applied it everywhere to ensure some contrast of fonts over background images (just that those box-texttab items have no background image). But then, that's why having someone on Mac look at it is important, you guys have done everything differently when it comes to theming, I guess ;-)

Pushed as http://hg.mozilla.org/comm-central/rev/42683c5b4db3
Comment 14 Stefan [:stefanh] 2010-09-25 08:21:33 PDT
I was wrong about comment #12, so I filed bug 599621 (discovered another thing also).

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