240320 - selection highlight color changed in 0.5+ to no longer use system selection color

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[xolaware.llc]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7b) Gecko/20040411 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7b) Gecko/20040411 Firefox/0.8.0+

it would appear that the new appearance of thunderbird 0.5+ for OS X has caused
the selection to always be blue as opposed to using the user's preferred
selection color as designated in the OS X Appearance System Preferences.  this
is only a minor annoyance in the folders pane, but in the messages pane, i have
long had one of my labels that is a color that is close to the color blue chosen
by thunderbird and have become accustomed to what messages of that color mean. 
now, i can no longer distinguish between a selected message that is an
unlabelled message and a selected message that i have labelled with my blue label.

Reproducible: Always
Steps to Reproduce:
1. label a message with the label that is blue
2. leave a message next to it unlabelled
3. use the up & down arrow keys to go between the two messages

Actual Results:  
both messages appear with the same or almost identical selection highlighting

Expected Results:  
unlabelled messages should appear selected with the user's system preference for
selection.

Comment 1

[Greg K.]

Confirmed using Thunderbird-Mac/2004-04-01. It WFM using
Mozilla-Mac/1.7b/classic. Yours, Kevin?

Comment 2

[Kevin Gerich]

I hardcode the blue highlight color for selected items in menus, listboxes,
trees because there is no way for me to get the real highlight color from the
system. For example, if you go into the Appearance page of the System Prefs and
set your Highlight Color to "Orange", you'll notice that in real Mac apps text
selections are highlighted with a light  orange (#FFD281), and tree and list
items are highlighted with a darker  orange (#FF8A22). I can use the lighter
orange with a "color: Highlight" CSS rule, but I can't get the darker orange.
This is in Firefox and Thunderbird only. It is a bug, but I'm not sure if
there's anything I can do about it unless there is a rule to get the correct
highlight color that I'm not aware of.

Comment 3

[xolaware.llc]

i don't care if it is a lighter orange or darker orange (orange is indeed my
selection color of choice) but i definitely prefer it to hard-coded blue.

my benchmark for comparing what items in the mail message list pane should be
compared to is the finder.  in the finder, individually selected file items are
highlighted in a "darker" color for selection.

perhaps closer would be Mail.app (which i clearly don't use.  in that, the items
in the mail message list pane, when selected, are also highlighted in the
"darker" selection color.

what did thunderbird do prior to these changes?  the items i am talking about
were highlighted correctly at that point.  where did the color come from then?

Comment 4

[Brion Vibber]

Attachment: Adds alternate primary & secondary highlight CSS colors

This patch adds -moz-mac-alternateprimaryhighlight (the darker color) and
-moz-mac-secondaryhighlight (a light gray for inactive list highlight) as CSS
color keywords. Use of these colors in Finder and similar is described here:
http://developer.apple.com/documentation/Carbon/Reference/databrow_reference/databrowser_ref/constant_23.html


I haven't tested it very thoroughly, and in particular am not sure it does the
right thing on earlier than 10.3.

Comment 5

[Kevin Gerich]

Thanks a lot for coming up with this patch Brion! I've tested on 10.3.3 and it
works great but I've had a real hard time packaging Thunderbird and moving it to
my iBook running 10.2 to test there. Can anyone else test this patch on a
pre-10.3 box?

Comment 6

[Brion Vibber]

Attachment: Adds alternate primary & secondary highlight CSS colors (corrected)

I haven't managed to make a real enough build that I can copy it to another
machine and run it there... However a quick test app that makes the same calls
hints that this should return the right colors on 10.2. On 10.1 there's no
kAlternatePrimaryHighlightThemeBrush so it'll then fall back to the lighter
highlight color.

This patch should correct the fallback on 10.1, I did it totally wrong the
first time.

Comment 7

[Kevin Gerich]

This would be a nice bit of polish for Thunderbird, Firefox and Seamonkey.
Requesting 1.0mac blockage. Can someone help us test this patch on 10.2 or earlier?

Comment 8

[Kevin Gerich]

Brion, can you please redo your patch against the trunk? Thanks!

Comment 9

[Brion Vibber]

Attachment: Updated patch to current trunk

Works for me, built and tested Thunderbird from trunk on 10.3.7.

Comment 10

[Stefan [:stefanh]]

Brion, did you ever tested this when building against the 10.2.8 sdk?

Comment 11

[Brion Vibber]

(In reply to comment #10)
> Brion, did you ever tested this when building against the 10.2.8 sdk?

Can't say that I have, that I know of. Any special directions for building it this way?

I've got a 10.2 partition I can boot to test, but I no longer have access to any 10.1 boxes.

Comment 12

[Stefan [:stefanh]]

(In reply to comment #11)
> (In reply to comment #10)
> > Brion, did you ever tested this when building against the 10.2.8 sdk?
> Can't say that I have, that I know of. Any special directions for building it
> this way?

Sure, just add the line "ac_add_options --with-macos-sdk=/Developer/SDKs/MacOSX10.2.8.sdk" to your .mozconfig (assuming you have the sdk's installed in that location).

The reason I ask is that the Appearance.h in the 10.2.8 sdk doesn't have any "kThemeBrushAlternatePrimaryHighlightColor", only "kThemeBrushPrimaryHighlightColor". And since official (ppc) builds use the 10.2.8 sdk (building on 10.3.9 with the 10.2.8 sdk for backward compability)... :/

I ran into this bug when trying to fix the selection colour in seamonkey classic theme, See bug 329144, comment #7. 

Comment 13

[Brion Vibber]

From http://developer.apple.com/releasenotes/Carbon/HIToolboxOlderNotes.html notes for the 10.3.9 SDK:

"kThemeBrushAlternatePrimaryHighlightColor is now documented. This brush is actually available on Mac OS X 10.2 and later. It represents the highlight color that is used in some list views."

If we need to compile with the older SDK, we can hardcode the value I guess. :)

Comment 14

[Stefan [:stefanh]]

Josh, Mark - are you guys ok with a new native highlight color? This would make theme authors able to use the "real" color instead of hard-coding a blue color.

Comment 15

[Stefan [:stefanh]]

Moving to the right component. IMHO this would be nice to have on branch - theme athours could then use the "real" selection colors.

Comment 16

[Mark Mentovai]

I don't have a problem with adding these brushes.

Comment 17

[Stefan [:stefanh]]

Brion, care to make a patch that also takes care of the 10.2.x stuff? Josh thought this was ok too.

Comment 18

[Mark Mentovai]

You need to do something like this:

#if MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_3
#define kThemeBrushAlternatePrimaryHighlightColor -5
[...]
#endif

MAC_OS_X_VERSION_MAX_ALLOWED corresponds to the SDK in use.  You don't use #ifdef kThemeBrushWhatever because they might be enums instead of macros.

Comment 19

[Brion Vibber]

Will post updated patch once I've tested it on 10.2 & x86 build...

Comment 20

[Brion Vibber]

Attachment: Demo HTML file showing mac-specific CSS colors, including the two added ones

Comment 21

[Brion Vibber]

Attachment: Updated patch to trunk, 10.2 compatible

Conditionally defines kThemeBrushAlternatePrimaryHighlightColor on older SDK, as recommended in comment 18.

Tested with universal build (10.2 SDK for PPC, 10.4u SDK for Intel); resulting binary appears to run correctly on 10.4/ppc, 10.4/intel, and 10.2/ppc.

Comment 22

[Stefan [:stefanh]]

(In reply to comment #21)
> Created an attachment (id=218397) [edit]
> Updated patch to trunk, 10.2 compatible
> 
> Conditionally defines kThemeBrushAlternatePrimaryHighlightColor on older SDK,
> as recommended in comment 18.
> 
> Tested with universal build (10.2 SDK for PPC, 10.4u SDK for Intel); resulting
> binary appears to run correctly on 10.4/ppc, 10.4/intel, and 10.2/ppc.
> 

Please ask for review (josh or mento) - otherwise it will take another two years ;). That said, I'm not sure you need the fallback color, since we don't support anything below 10.2 (also, difference between new/old highlight is that use of the new highlight color have to be combined with a change of text-color). But I'm absolutely not in the position to comment on this, so you might want to hear the reviewers comments first.

Comment 23

[Brion Vibber]

Comment on attachment 218397 [details] [diff] [review]
Updated patch to trunk, 10.2 compatible

This version does include an extra line for fallback on 10.1, which is no longer necessary since 10.1 support was dropped. I can remove it if desired, but it shouldn't hurt anything to be there, in case someone wants 10.1 support back. ;)

Comment 24

[Josh Aas]

Brion - I'm not ignoring this bug, it is just going to take me a little bit to get to it. Thanks for looking into it!

Comment 25

[Josh Aas]

Comment on attachment 218397 [details] [diff] [review]
Updated patch to trunk, 10.2 compatible

looks fine to me

Comment 26

[Stefan [:stefanh]]

Comment on attachment 218397 [details] [diff] [review]
Updated patch to trunk, 10.2 compatible

We probably need sr from a layout peer for this. Trying roc.

Comment 27

[Stefan [:stefanh]]

Comment on attachment 218397 [details] [diff] [review]
Updated patch to trunk, 10.2 compatible

Requesting approval-branch-1.8.1. Minor & safe fix that will make it possible to use native styled selection colors in the Mac themes(Pinstripe/Classic).

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 218397 [details] [diff] [review]
Updated patch to trunk, 10.2 compatible

This changes the nsILookAndFeel interface, but not in any way that could affect anyone, as far as I can tell.

Comment 29

[Stefan [:stefanh]]

Josh,

Can you check in Brion's patch on trunk/1.8.1 branch, please?

Comment 30

[Josh Aas]

landed on branch and trunk, if anyone gets upset about the interface change don't blame the messenger :)

Comment 31

[Stefan [:stefanh]]

Attachment: Pinstripe fix

Thanks for the check-ins, Josh :)

I'll attach a patch for Pinstripe here that will make selection colors in listboxes/trees configurable via OS X's system preferences. Thunderbird's folder pane unaffected. Kevin, do you have the time to look at this?

Comment 32

[Asaf Romano (gone)]

Comment on attachment 225433 [details] [diff] [review]
Pinstripe fix

a181=mano.

Comment 33

[Stefan [:stefanh]]

Comment on attachment 225433 [details] [diff] [review]
Pinstripe fix

Mano, can you check in the patch (trunk/1.8.1), please?

Comment 34

[Karsten Düsterloh]

Comment on attachment 225433 [details] [diff] [review]
Pinstripe fix

Checked in on trunk and MOZILLA_1_8_BRANCH.

Comment 35

[Stefan [:stefanh]]

Attachment: Screenshot of previous/new look of folder pane

For the record, this actually do change the look of selected items in Thunderbirds left folder pane. But I think the behaviour before the patch wasn't correct. Before the patch, upon selection, a darker blue/lighter gray area could be seen around the folder titles. That area is now gone.

Comment 36

[Stefan [:stefanh]]

Attachment: Additional pinstripe fix

Hmm, I forgot some things. The style rules for 'seltype="cell"' etc should also be changed. We don't seem to use it in our tree, but afaik those attributes (cell, text) are implemented so one could create a tree with selectable cells (and navigate between cells in different rows). I'm making them look the same as for the whole row for now. If someone else has any idea of what color to use... I also changed the colors on the "alternatingbackground" stuff at the  end. Those are not in use either, but if someone  wants to use them...

Comment 37

[Stefan [:stefanh]]

Comment on attachment 226072 [details] [diff] [review]
Additional pinstripe fix

This is an additional fix to Pinstripe that should have been in the original patch.

Comment 38

[Stefan [:stefanh]]

Attachment: 1.8 branch diff of tree.css

The tree.css part of attachment # 225433 [details] [diff] [review] might not apply on branch - bug 296040 (cell-based selection in trees) wasn't fixed on the 1.8 branch. So, here's a 1.8 diff of tree.css.

Comment 39

[Asaf Romano (gone)]

Comment on attachment 226942 [details] [diff] [review]
1.8 branch diff of tree.css

Checking in toolkit/themes/pinstripe/global/tree.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/global/tree.css,v  <--  tree.css
new revision: 1.8.8.4; previous revision: 1.8.8.3
done

Comment 40

[:Gavin Sharp [email: [e-mail]]]

Checked in attachment 226072 [details] [diff] [review] on the trunk.
mozilla/toolkit/themes/pinstripe/global/tree.css 	1.12