488118 - Cannot see selected message tag colors in message list (thread) pane

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[ojab]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090413 Minefield/3.6a1pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090413 Lightning/1.0pre Shredder/3.1a1pre

After https://bugzilla.mozilla.org/show_bug.cgi?id=483761 was fixed it's impossible to see if selected message tagged or not. Selection in message pane is always blue (depends on theme, I think, but one color anyway). Previously selection color was dependent on message tags.

In recent builds I should deselect message to see if I should set tag or not, rather annoying.

Reproducible: Always

Comment 1

[Phil Ringnalda (:philor)]

Without having looked very closely, I'd expect that to be the result of adding 

#threadTree treechildren::-moz-tree-row(selected) {
  background-color: Highlight;
}

which I'm not entirely sure about anyway: toolkit makes (selected) -moz-cellhighlight and (selected, focus) Highlight, so I'd expect this to have regressed being able to tell the difference between those two states, along with probably being more specific than the rule a tag adds (wherever _that_ happens, dunno offhand).

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

hmm, that's not good.  Yeah, that might be overriding the tagColors.css.  Not sure why I didn't see that when testing.

http://mxr.mozilla.org/comm-central/source/mail/themes/gnomestripe/mail/tagColors.css

Comment 3

[Phil Ringnalda (:philor)]

Ah, double overriding: a rule in a sheet overrides a rule imported by that sheet, plus |#foo whatever| is (expensively) more specific even if it hadn't already won in the cascade.

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: make tagColors more sepecific selectors

I've gone with the approach of making the tagColors.css more specific as other methods seem insane and AFAIK the tagColors doesn't need to apply to anything but the #threadPane.

I prefixed the tagColors treechildren selector so it looks like this:

tree#threadTree treechildren

This allows the existing selector to remain as is and tags colors continue to work.  Also changed the Highlight to -moz-cellhighlight and applied it to both (selected), and (selected, focus) rules.  The (selected) rule means the cell is still highlighted but I think it looks a lot better than when it turns into the -moz-oddtreerow color and blends into the background.

Let me know what you think of this approach.

Comment 6

[ojab]

Don't know about approach, but with this patch I have gray selection instead of blue. Absolutely looks like inactive selection, but with white font color (as it should be).

Comment 7

[Magnus Melin [:mkmelin]]

just using "!important;" for the background colors seems to work.

Also, per comment 1, in mailWindow.css it should be two different rules - on for selected, and one for selected + focus. 
The selectors can be more specific: #threadTree > treechildren::..... instead of #threadTree treechildren::....

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #7)
> just using "!important;" for the background colors seems to work.

Ok, I can do that instead.

> Also, per comment 1, in mailWindow.css it should be two different rules - on
> for selected, and one for selected + focus.

This I wasn't sure about because of the way we indicate selected but not focused.  Most times the selected, !focused uses the background color change to indicate that.  However that selection background color is the same as the -moz-oddtreerow color and so you end up with either no indication of selection on odd rows or three rows of the same background color.

Right now it changes the font color automatically through toolkit css rules such that we keep the highlight background color for selected, !focus and selected, focus but the font color changes.

> The selectors can be more specific: #threadTree > treechildren::..... instead
> of #threadTree treechildren::....

These didn't look so good to me. The best I could do for a specific selector would be something like this:

#threadTree > .tree-stack > .tree-rows > .tree-bodybox > treechildren

Alternately it could also look like this (using elements instead of classes):

#threadTree > stack > treerows > hbox > treechildren

I know it's better performance but it's also pretty big.  Let me know what you think is best.

Comment 9

[Magnus Melin [:mkmelin]]

I think I'd prefer the element approach.

Comment 10

[klonos]

I don't see this in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20090227 Lightning/1.0pre Shredder/3.0b2

Fixed?

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #8)
> > The selectors can be more specific: #threadTree > treechildren::..... instead
> > of #threadTree treechildren::....
> 
> These didn't look so good to me. The best I could do for a specific selector
> would be something like this:
> 
> #threadTree > .tree-stack > .tree-rows > .tree-bodybox > treechildren
> 
> Alternately it could also look like this (using elements instead of classes):
> 
> #threadTree > stack > treerows > hbox > treechildren

I was completely wrong about this and your, much simpler approach, works.  I looked in the DOM Inspector and my element selector should have worked but it appears even the toolkit css is using tree > treechildren directly.

patch coming up with your simpler selector and the !important flag on tagColors

Comment 12

[Magnus Melin [:mkmelin]]

(In reply to comment #10)
> I don't see this in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre)
> Gecko/20090227 Lightning/1.0pre Shredder/3.0b2
> 
> Fixed?

No, that's a build from before the regression happened.

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: using the #threadTree > treechildren selector and the !important flag for tagColors

here's the patch with all the recommended fixes.  Magnus, can you review this for me?

I'm going to upload a screenshot comparison of the selected, focus differences as well so you can see what it looks like.  I think we've picked the best option keeping focus and selected the same background colour.

Comment 14

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: comparison of background colors for selected and selected, focus CSS selectors

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 374278 [details] [diff] [review]
using the #threadTree > treechildren selector and the !important flag for tagColors

Looks good Bryan! r=mkmelin

Comment 16

[Magnus Melin [:mkmelin]]

changeset:   2464:eb7247133c36
http://hg.mozilla.org/comm-central/rev/eb7247133c36

->FIXED

Comment 17

[ojab]

hmm.. Active selection color still gray instead of blue (without tags) after this patch. Is this appropriate?

Comment 18

[ojab]

Attachment: screenshot, wrong active selection color in thread pane

Comment 19

[Bryan Clark (DevTools PM) [:clarkbw]]

Strange.  That makes me wonder what theme you're using and if a different theme doesn't have this issue.

Comment 20

[Tuukka Tolvanen (sp3000)]

I'm seeing focused selection color in unfocused selection in the threadpane in 20090516 -- the kind of color that mixes badly with tag-colored text.