Persona doesn't apply to sidebar

RESOLVED FIXED in seamonkey2.1b1

Status

SeaMonkey
Themes
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: JasnaPaka, Assigned: Robert Kaiser)

Tracking

Trunk
seamonkey2.1b1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Created attachment 459824 [details]
Firefox 4.0 NB with Persona on sidebar
(Assignee)

Comment 2

7 years ago
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.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #476140 - Flags: review?(neil)

Comment 3

7 years ago
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

7 years ago
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.
(Assignee)

Comment 5

7 years ago
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

7 years ago
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.
Attachment #476140 - Flags: review?(neil) → review+

Comment 7

7 years ago
Comment on attachment 476140 [details] [diff] [review]
fix sidebar header(s)

This patch is missing the mac classic file.
(Assignee)

Comment 8

7 years ago
(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.
(Assignee)

Updated

7 years ago
Summary: Persona isn't apply on sidebars → Persona doesn't apply to sidebar
(Assignee)

Comment 9

7 years ago
(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.
(Assignee)

Comment 10

7 years ago
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.
Attachment #476140 - Attachment is obsolete: true
Attachment #477999 - Flags: review?(neil)
(Assignee)

Updated

7 years ago
Attachment #477999 - Flags: review?(stefanh)
(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?

Updated

7 years ago
Attachment #477999 - Flags: review?(neil) → review+

Comment 12

7 years ago
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.
Attachment #477999 - Flags: review?(stefanh) → review+
(Assignee)

Comment 13

7 years ago
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b1

Comment 14

7 years ago
I was wrong about comment #12, so I filed bug 599621 (discovered another thing also).
Depends on: 599621
You need to log in before you can comment on or make changes to this bug.