420726 - Use system background color for odd tree rows

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Stefan [:stefanh]]

I don't see any reason not using system colors for our "striped" trees/listboxes. Apple actually have constants for even/odd rows and they're supported from 10.4 and up. Using those system colors will also make, for instance, DOMi look more decent imo.

Comment 1

[Stefan [:stefanh]]

Attachment: Use system colors for odd/even rows

Mano, I'd like you to look at this first. Perhaps we can just use one system color here (oddrow) since the even row color is white anyway?

Comment 2

[Stefan [:stefanh]]

I should of course say here that the bluish system color for odd rows do match the Finder ui, but not Safari. So far, I've found 3 different colors:

Safari (237, 243, 254) -  edf3fe
Finder (236, 243, 254) -  ecf3fe
iTunes (241, 245, 250) -  f1f5fa

However, the difference between the first 2 is really small - you hardly notice it. Besides, having color management on will probably make the current hardcoded  "Safari" color differ anyway :-)

Comment 3

[Stefan [:stefanh]]

Attachment: testcase showing color differences

Here's a testcase that shows how the colors differ. Only the iTunes color stands out, the two other look pretty much the same.

Comment 4

[Stefan [:stefanh]]

Comment on attachment 307050 [details] [diff] [review]
Use system colors for odd/even rows

I'm going to attach a new patch (that just adds the oddrow color) and let beltzner have a first look at it.

Comment 5

[Stefan [:stefanh]]

Attachment: New version, use system bg color on odd rows

Comment 6

[Stefan [:stefanh]]

Attachment: Screenshot of before/after (Page Info & DOMi)

Here's a screenshot showing Page Info (Media tab) and DOMi before/after attachment #308904 [details] [diff] [review]

Comment 7

[Stefan [:stefanh]]

Comment on attachment 308904 [details] [diff] [review]
New version, use system bg color on odd rows

Beltzner, see comment #2 and attachment #307053 [details] for the color difference. The most visible effect of this can be seen in attachment #308906 [details] (where we had gray rows before we'll now have light blue ones). Note also that our current gray rows appear to be 1px less in height due to a white top-border.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 308904 [details] [diff] [review]
New version, use system bg color on odd rows

Not sure if roc or josh is the right person to review the nsLookAndFeel.mm bits - probably josh.

Comment 9

[Stefan [:stefanh]]

Comment on attachment 308904 [details] [diff] [review]
New version, use system bg color on odd rows

Mano, can you please look at "your" files here?

Comment 10

[Stefan [:stefanh]]

Comment on attachment 308904 [details] [diff] [review]
New version, use system bg color on odd rows

I surely need sr as well.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 308904 [details] [diff] [review]
New version, use system bg color on odd rows

r=mano (browser & toolkit).

Comment 12

[Stefan [:stefanh]]

(mid-air)

(tree.css)

 391 /* ::::: editable tree ::::: */
 392 
 393 treechildren::-moz-tree-row(selected, editing) {
 394   background-color: transparent;
 395 }
 396 
 397 treechildren::-moz-tree-cell-text(selected, editing) {
 398   color: inherit;
 399 }
 400 
 401 .tree-input {
 402   -moz-appearance: none;
 403    border: 1px solid Highlight;
 404   -moz-border-top-colors: Highlight;
 405   -moz-border-bottom-colors: Highlight;
 406   -moz-border-left-colors: Highlight;
 407   -moz-border-right-colors: Highlight;
 408   margin: 0 0 0 -3px;
 409   padding: 1px;
 410 }

I was actually playing a little bit with this. I think the odd/alternate rows
where forgotten here (393, 394). Even if I would add the styling for
odd/alternate rows, I think this needs some additional thoughts - it doesn't
looks that "macish". Since I'm making the top (row) border transparent, the
.tree-input will now appear to have an extra top border with the same color. In
the end, I think we would want some native textfield styling of the .tree-input
here. Anyway, I'll file a follow-up on this.

Comment 13

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

Comment on attachment 308904 [details] [diff] [review]
New version, use system bg color on odd rows

Why not make this -moz-oddrowbackground? I seem to recall GTK2 could use this. You just have to rename things, GTK2 support is not required.

Comment 14

[Stefan [:stefanh]]

(In reply to comment #13)
> (From update of attachment 308904 [details] [diff] [review])
> Why not make this -moz-oddrowbackground? I seem to recall GTK2 could use this.
> You just have to rename things, GTK2 support is not required.
> 

Yeah, I'll rename and do some re-ordering once josh have looked at this.

+    case eColor__moz_mac_oddrowbackground:
+      // Background color of odd list rows. Note that Apples row index is different from ours.

I'll change to "Apple's" as well.

Comment 15

[Stefan [:stefanh]]

Attachment: New version using -moz-oddrowbackground

Final version. Changes made:

1) "-moz-oddrowbackground" instead of "-moz-mac-oddrowbackground" as suggested by roc
2) Some re-ordering (since the new color name starts with "o" instead of "m")
3) Apples --> Apple's (comment in widget/src/cocoa/nsLookAndFeel.mm)

Transfering reviews (where possible).

Comment 16

[Josh Aas]

Comment on attachment 309996 [details] [diff] [review]
New version using -moz-oddrowbackground

+      res = GetMacBrushColor(kThemeBrushListViewEvenRowBackground, aColor, NS_RGB(0xFF,0xFF,0xFF));

If the hex at the end of that line is a default, shouldn't we pick a more reasonable default? Perhaps one of the hex values that we're removing from the theme? Otherwise this looks fine.

Comment 17

[Josh Aas]

Comment on attachment 309996 [details] [diff] [review]
New version using -moz-oddrowbackground

You don't need to manually carry over r/sr flags to updated patches. Just makes it look like you incorrectly marked them yourself.

Comment 18

[Stefan [:stefanh]]

Attachment: Final version with new default color in cocoa/nsLookAndFeel.mm

I picked the gray color that can be seen in Page Info: Media atm.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 312074 [details] [diff] [review]
Final version with new default color in cocoa/nsLookAndFeel.mm

a=beltzner

Is the new OSX theme already calling this? I'm expecting we'll see the improvements immediately?

Comment 20

[Stefan [:stefanh]]

> Is the new OSX theme already calling this? I'm expecting we'll see the
> improvements immediately?

The patch fixes the theme code as well. So, yes - when I've landed this, you'll see the improvements immediately.

Comment 21

[Stefan [:stefanh]]

Checking in browser/themes/pinstripe/browser/places/organizer.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/places/organizer.css,v  <--  organizer.css
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/themes/pinstripe/global/tree.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/global/tree.css,v  <--  tree.css
new revision: 1.16; previous revision: 1.15
done
Checking in toolkit/themes/pinstripe/mozapps/downloads/downloads.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/downloads/downloads.css,v  <--  downloads.css
new revision: 1.26; previous revision: 1.25
done
Checking in layout/style/nsCSSKeywordList.h;
/cvsroot/mozilla/layout/style/nsCSSKeywordList.h,v  <--  nsCSSKeywordList.h
new revision: 3.105; previous revision: 3.104
done
Checking in layout/style/nsCSSProps.cpp;
/cvsroot/mozilla/layout/style/nsCSSProps.cpp,v  <--  nsCSSProps.cpp
new revision: 3.166; previous revision: 3.165
done
Checking in widget/public/nsILookAndFeel.h;
/cvsroot/mozilla/widget/public/nsILookAndFeel.h,v  <--  nsILookAndFeel.h
new revision: 1.64; previous revision: 1.63
done
Checking in widget/src/cocoa/nsLookAndFeel.mm;
/cvsroot/mozilla/widget/src/cocoa/nsLookAndFeel.mm,v  <--  nsLookAndFeel.mm
new revision: 1.16; previous revision: 1.15
done

Comment 22

[Stefan [:stefanh]]

(In reply to comment #17)
> (From update of attachment 309996 [details] [diff] [review])
> You don't need to manually carry over r/sr flags to updated patches. Just makes
> it look like you incorrectly marked them yourself.
> 

It's probably a matter of taste (I'm not the only one doing this), I think it makes things clearer.