Closed Bug 329144 Opened 15 years ago Closed 14 years ago

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

Categories

(SeaMonkey :: General, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey1.1alpha

People

(Reporter: stefanh, Assigned: stefanh)

References

Details

(Keywords: fixed-seamonkey1.1a, fixed1.8.1, Whiteboard: ETA: seamonkey1.1a)

Attachments

(6 files, 3 obsolete files)

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...
Attached patch Fix tree.css styles (obsolete) — Splinter Review
Attachment #213789 - Flags: review?(mnyromyr)
>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...
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.
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. 
(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).
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.
Attachment #213789 - Flags: review?(mnyromyr)
(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.
Attached patch Hardcode brush color (test) (obsolete) — Splinter Review
> 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...
Depends on: 240320
Summary: [Mac] Classic: Selected treerows should be blue → [Mac] Classic: Selected items should use darker system highlight color
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.
New patch coming up in a few days...
Status: NEW → ASSIGNED
Whiteboard: ETA: seamonkey1.1a
Target Milestone: --- → mozilla1.8.1
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...
> 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...
I guess we could always move the rule to autocomplete.css unless someone (jag?) has any better ideas...
Attached image 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 }
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...
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).
Component: Themes → General
Product: Core → Mozilla Application Suite
Target Milestone: mozilla1.8.1 → seamonkey1.1alpha
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.
Attachment #228597 - Flags: superreview?(neil)
Attachment #228597 - Flags: review?(bugs.mano)
Attachment #213789 - Attachment is obsolete: true
Attachment #214362 - Attachment is obsolete: true
Attachment #226022 - Attachment is obsolete: true
Comment on attachment 228597 [details] [diff] [review]
Patch - change highlight colors etc

Should you move Modern's styles for consistency?
Attachment #228597 - Flags: superreview?(neil) → superreview+
(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?
(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.
(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.
Fwiw, Neil thought it was ok (on irc)to not move any more style rules.
Comment on attachment 228597 [details] [diff] [review]
Patch - change highlight colors etc

r=mano
Attachment #228597 - Flags: review?(bugs.mano) → review+
Comment on attachment 228597 [details] [diff] [review]
Patch - change highlight colors etc

Neil, can you check in the patch, please?
Checked in by Neil (thanks).
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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...
Attachment #228597 - Flags: approval-seamonkey1.1a?
Comment on attachment 228597 [details] [diff] [review]
Patch - change highlight colors etc

sound good. a=me for 1.1a
Attachment #228597 - Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+
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?
Checked in attachments #228597 and #232927 on MOZILLA_1_8_BRANCH.
You need to log in before you can comment on or make changes to this bug.