329144 - [Mac] Classic: Selected items should use darker system highlight color

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Stefan [:stefanh]]

We should re-style classic's tree.css a bit. Selected treerows should be blue. I'm basically going to rip what's needed from toolkit's pinstripe theme here...

Comment 1

[Stefan [:stefanh]]

Attachment: Fix tree.css styles

Comment 2

[Karsten Düsterloh]

>Index: themes/classic/global/mac/tree.css
>===================================================================
> tree {
>-  -moz-appearance: treeview;
>   margin: 0px 4px;
>-  border: 2px solid;
>-  -moz-border-top-colors: ThreeDShadow ThreeDDarkShadow;
>-  -moz-border-right-colors: ThreeDHighlight ThreeDLightShadow;
>-  -moz-border-bottom-colors: ThreeDHighlight ThreeDLightShadow;
>-  -moz-border-left-colors: ThreeDShadow ThreeDDarkShadow;
...
> treechildren::-moz-tree-row {
>-  -moz-appearance: treeitem;

> treechildren::-moz-tree-separator {
>-  -moz-appearance: separator;
>-  border-top: 1px solid ThreeDShadow;
>-  border-right: 1px solid ThreeDHighlight;
>-  border-bottom: 1px solid ThreeDHighlight;
>-  border-left: 1px solid ThreeDShadow;

I'm not at all comfortable with these. Why are you removing the native look and replacing it by arbitrary colour values? -moz-appearance and ThreeD* are there for a reason...

Comment 3

[[e-mail]]

Currently active tree items paint in the "Highlight" colour, which (on the Mac) appears to be the same as the text selection background and the menu hover. If this is incorrect then mac/nsLookAndFeel.cpp should be corrected instead.

Comment 4

[Stefan [:stefanh]]

In reply to comment #2)
.
.
.
.
> I'm not at all comfortable with these. Why are you removing the native look and
> replacing it by arbitrary colour values? -moz-appearance and ThreeD* are there
> for a reason...
> 

"-moz-appearance" overrules border styles. I guess the borders are still there because noone bothered to removed them when the appearance styles where introduced. Fwiw, we also have an issue with native-styled trees, see bug 301060. Fwiw, I don't really trust the mac native styling either - some stuff origins from the pre-OS X days...

> treechildren::-moz-tree-row {
>-  -moz-appearance: treeitem;

This might have been an overkill, though. Hmm, I know see that it might look better if I could use an outline instead of a border for the focusring - there's a 2px gap (left/right) on selected rows when the tree doesn't have focus. 

Comment 5

[Stefan [:stefanh]]

(In reply to comment #3)
> Currently active tree items paint in the "Highlight" colour, which (on the Mac)
> appears to be the same as the text selection background and the menu hover. If
> this is incorrect then mac/nsLookAndFeel.cpp should be corrected instead.
> 

Well, I fear that noone will do that. See bug 301364. Josh doesn't want to fix anything more it seems (https://bugzilla.mozilla.org/show_bug.cgi?id=301364#c6).

Comment 6

[Stefan [:stefanh]]

Attachment: Screenshot of tree with ThreeD* border colors and without "-moz-appearance: treeview;"

Comment 7

[Stefan [:stefanh]]

I've done some investigation regarding the CSS "Highlight" colour:

Currently, the mac "Highlight" colour is the one that appears on < 10.3. What we want is the Appearance Manager constant "kThemeBrushAlternatePrimaryHighlightColor". However, since we're building against the 10.2.8 sdk (and thus using the 10.2.8 Appearance.h) that won't work. The 10.2.8 Appearance.h only has "kThemeBrushPrimaryHighlightColor" - and that returns the old colour.

If you build without the 10.2.8 sdk or use the 10.4u sdk (intel builds) it will work, though.

One could of course use the old colour as a fallback in mac/nsLookAndFeel.cpp. One problem with that might be that "Highlight" on 10.2 doesn't imply that the textcolour change - but it does on 10.3 and higher.

Comment 8

[Brion Vibber]

(In reply to comment #7)
> Currently, the mac "Highlight" colour is the one that appears on < 10.3. What
> we want is the Appearance Manager constant
> "kThemeBrushAlternatePrimaryHighlightColor". However, since we're building
> against the 10.2.8 sdk (and thus using the 10.2.8 Appearance.h) that won't
> work. The 10.2.8 Appearance.h only has "kThemeBrushPrimaryHighlightColor" - and
> that returns the old colour.

According to Apple's release notes this brush is available on 10.2 but wasn't documented until 10.3:
http://developer.apple.com/releasenotes/Carbon/HIToolboxOlderNotes.html

So if you need to use it with the 10.2.8 SDK, you could hardcode the value (-5) or #define it onesself.

Comment 9

[Stefan [:stefanh]]

Attachment: Hardcode brush color (test)

> So if you need to use it with the 10.2.8 SDK, you could hardcode the value (-5)
> or #define it onesself.

Ah, right. This seems to work - not tested on 10.2, though. Hmm, a change like this is a rather large change in the ui. You'll need to tweak some css files, since this affects not just trees...

Comment 10

[Stefan [:stefanh]]

Attachment: patch for autocomplete/history items

I'm just attaching a patch that will change the selection highlight color for autocomplete/history items - I might incorporate this one with the forthcoming tree/listbox patch.

Comment 11

[Stefan [:stefanh]]

New patch coming up in a few days...

Comment 12

[Stefan [:stefanh]]

OK, so the search box with the "Search x for y" is styled in navigator.css...
navigator.css:
------------------------------------------------------
264 .autocomplete-search-engine[menuactive="true"] {
265   background-color: Highlight;
266   color: HighlightText;
267 }
------------------------------------------------------

Hmm...

Comment 13

[Stefan [:stefanh]]

> OK, so the search box with the "Search x for y" is styled in navigator.css...
> navigator.css:
> ------------------------------------------------------
> 264 .autocomplete-search-engine[menuactive="true"] {
> 265   background-color: Highlight;
> 266   color: HighlightText;
> 267 }
> ------------------------------------------------------
> 
> Hmm...
> 

Neil, I suppose there is no way for me to overrule this someplace in global/mac? I need to use background-color: -moz-mac-alternateprimaryhighlight; and white color for the text...

Comment 14

[[e-mail]]

I guess we could always move the rule to autocomplete.css unless someone (jag?) has any better ideas...

Comment 15

[Stefan [:stefanh]]

Attachment: Another problem...

While I'm at it... As you can see in the image I would rather prefer "GrayText" or something.

navigator.css:
246 .autocomplete-treebody::-moz-tree-cell-text(comment) {
247   color: ThreeDShadow;
248 }

Comment 16

[Stefan [:stefanh]]

Ah, I always wondered why there is a white border at the top of the search box... Hmm, those style rules (in navigator.css) doesn't work for mac either:
-------------------------------------------------------
.autocomplete-search-box {
  border-top: 2px groove -moz-Dialog;
  background-color: -moz-Dialog;
  color: ButtonText;
}

.autocomplete-result-popup[nomatch] > .autocomplete-search-box {
  border-top: 1px solid ThreeDHighlight;
}
-------------------------------------------------------
Both of the border colours seems to produce a white (1px) border at the top of the search box.

I can only explain the last one: widget/src/mac/nsLookAndFeel.cpp#268 returns a white colour for ThreeDHighlight... (I don't know if the fallback colour is used or not).

Anyway, the solution here seems to be to move those style rules as well...

Comment 17

[Stefan [:stefanh]]

Attachment: screenshots - white gap/border at top of search box (1.8.0.2)

Just to show you what I mean. Note that this is with a 1.8.0.2 build (some changes might have been made, but the border remains).

Comment 18

[Stefan [:stefanh]]

Attachment: Patch - change highlight colors etc

This patch will change all the selection highlight colors and fix some issues in autocomplete (I had to move 4 style rules from navigator.css to autocomplete.css).

1: New top border on the search box

2: I decided to not use any border on the search box when there is no result to show in the search box (ie "nomatch"). The surrounding autocomplete-result-popup border will work as a "top" border here instead.

3 & 4: New mac highlight colors.

Comment 19

[Stefan [:stefanh]]

Attachment: Screenshots of "new" autocomplete look

Comment 20

[[e-mail]]

Comment on attachment 228597 [details] [diff] [review]
Patch - change highlight colors etc

Should you move Modern's styles for consistency?

Comment 21

[Stefan [:stefanh]]

(In reply to comment #20)
> (From update of attachment 228597 [details] [diff] [review] [edit])
> Should you move Modern's styles for consistency?
> 

Are the themes consistent now?

Comment 22

[[e-mail]]

(In reply to comment #21)
>(In reply to comment #20)
>>Should you move Modern's styles for consistency?
>Are the themes consistent now?
Well noow that I look at the patch again, I think it would be best if you moved all of those autocomplete styles into autocomplete.css in both themes.

Comment 23

[Stefan [:stefanh]]

(In reply to comment #22)
> (In reply to comment #21)
> >(In reply to comment #20)
> >>Should you move Modern's styles for consistency?
> >Are the themes consistent now?
> Well noow that I look at the patch again, I think it would be best if you moved
> all of those autocomplete styles into autocomplete.css in both themes.
> 

OK, but I don't think we can get rid of all the autocomplete rules in navigator.css, since there are cases where the same class is styled differently in resp file.

Comment 24

[Stefan [:stefanh]]

Fwiw, Neil thought it was ok (on irc)to not move any more style rules.

Comment 25

[Asaf Romano (gone)]

Comment on attachment 228597 [details] [diff] [review]
Patch - change highlight colors etc

r=mano

Comment 26

[Stefan [:stefanh]]

Comment on attachment 228597 [details] [diff] [review]
Patch - change highlight colors etc

Neil, can you check in the patch, please?

Comment 27

[Stefan [:stefanh]]

Checked in by Neil (thanks).

Comment 28

[Stefan [:stefanh]]

Comment on attachment 228597 [details] [diff] [review]
Patch - change highlight colors etc

I would like this for 1.1a - another small ui enhancement for users of mac classic...

Comment 29

[Robert Kaiser]

Comment on attachment 228597 [details] [diff] [review]
Patch - change highlight colors etc

sound good. a=me for 1.1a

Comment 30

[Stefan [:stefanh]]

Attachment: Branch-friendly diff of tree.css

The trunk diff of tree.css doesn't apply on branch (because of bug 296040). Here's a branch-friendly diff of tree.css. That said, any chance you could check in the approved patch on 1.8 branch, KaiRo?

Comment 31

[Karsten Düsterloh]

Checked in attachments #228597 and #232927 on MOZILLA_1_8_BRANCH.