429188 - bug 420726 added system color implemented on only one platform

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

bug 420726 added a system color but implemented it on only one platform -- that shouldn't have been done.

I'm also not too happy with the name.  And I'm a little uncomfortable with the assumption it makes that the even rows should be -moz-field (is that what they should be?) but the odd rows have a special color.  It seems like if we have a constant for one of the two we really ought to have a constant for the other.

The patch in question should have had Style System module owner or peer review, I think.

Comment 1

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

Yeah, sorry.

Comment 2

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

Stefan, can you address these issues immediately? If not, I think we should back out bug 420726.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that I filed bug 429214 on having an automated test for this.

Comment 4

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

How do you suggest he address the comments? Change the name to make it mac-only? How do we deal with things where we want to use system-native colours for UI elements that aren't actually cross-platform? I suppose the better thing to do here would be return -moz-field on platforms that don't implement it?

The patch is definitely optimized for OSX, where all alternating row UI goes back and forth between textfield background & the colour that it's pulling. But perhaps what you're saying is we should have names for both colours, and then as the systems do those natively, so we should return them?

So:
  Windows: -moz-field and -moz-field
  OSX    : -moz-field and the queried colour
  Linux  : ??

Comment 5

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

cc'ing Michael Monreal who might know if there's a way to implement this for Linux.

Comment 6

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

(and ventnor, who apparently fixed a linux bug to get alternating row colouring working)

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

Related bug on linux was bug 427979

Comment 8

[Stefan [:stefanh]]

(In reply to comment #2)
> Stefan, can you address these issues immediately? If not, I think we should
> back out bug 420726.

The re-naming should be trivial, just need a suggestion of a new name. evenrow shouldn't be hard to do either. Apple actually has a constant for this, but it returns the same as -moz-field and for that reason I felt it was an overkill to use evenrow. That decision might have been wrong, though.

Unfortunately, I just have access to a mac, so any win/nix stuff will require a lot of hand-holding here. We can change the name to be mac-only, but it looks like that would require bug 427979 to be backed out :-/

FYI, I'm writing this from work, will be home at 1700 (UTC+2)

Comment 9

[Michael Ventnor]

GTK has a constant for evenrow too, but the vast majority of the time it also returns -moz-field.

Why not just make the windows code (and any other platform code that hasn't implemented oddrow) return the same as the tree background for Fx3?

Comment 10

[Stefan [:stefanh]]

So, following Michaels suggestion, on platforms that doesn't have oddrow, I'll return the same as the tree background. By reading the gtk file, I assume that the new name should be "-moz-oddcellbackground"?

dbaron, does this sounds reasonable to you? Are you still feeling uncomfortable with the evenrow assumptions or are you ok with not implementing "-moz-evencellbackground" (assuming that's the correct name)?

Comment 11

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

(In reply to comment #9)
> Why not just make the windows code (and any other platform code that hasn't
> implemented oddrow) return the same as the tree background for Fx3?

Why the tree background? I was assuming that what we'd do is fallback to -moz-field if unimplemented (which should be "the background colour of a textfield", which is safe for places where one would be doing alternate row colour, and f.e. on Windows is what we're doing anyway) and then specify evenrow and oddrow as per platform.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

System colors in general don't use the word 'background' in them -- they use 'text' to indicate that something is a foreground and the lack of it to indicate a background.

I think we should have -moz-eventreerow and -moz-oddtreerow, which, on most platforms, are both the same as -moz-field.  It should be documented that the appropriate text color to use on them is -moz-fieldtext.

(I'm assuming -moz-field and -moz-fieldtext are the correct colors for trees based on the above discussion, though I might be wrong.

Comment 13

[Stefan [:stefanh]]

Attachment: almost finished patch

I'm just attaching a wip version, in case anyone wonders what I'm doing...

This does the following:

1) -moz-eventreerow and -mozoddtreerow for all OS. For eventreerow, I used the same color as -moz-Field everywhere except for mac, where I used the OS constant (which returns the same color as -moz-Field). For gtk, I think it should be quite easy for someone who knows about the code/platform to get the theme color (if it exists).

2) Updates nsXPLookAndFeel::sColorPrefs - there where a few colors missing there

3) Updates the themes to the new color name.

I'm just going to look a bit more into the -moz-fieldtext/-moz-field thing and add some documentation.

Comment 14

[Stefan [:stefanh]]

Attachment: Final version

I think this should address everything. It's basically the same as attachment #316112 [details] [diff] [review], except for added/changed comments and a correction of a mistake in my ordering of the evenrow color.

Mano, can you please look at the toolkit files (just a name change)?

Comment 15

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

I think you can simplify this patch for the platforms that return the same as -moz-field, by just putting your 'case ...:' before the -moz-field case in the nsLookAndFeel switch.

Comment 16

[Stefan [:stefanh]]

(In reply to comment #15)
> I think you can simplify this patch for the platforms that return the same as
> -moz-field, by just putting your 'case ...:' before the -moz-field case in the
> nsLookAndFeel switch.
>

Yeah, of course - sorry.

Comment 17

[Stefan [:stefanh]]

Attachment: Simplified version

There where some css3 comments just above the -moz-field case that I felt had to be removed, also changed the comment in the cocoa file a bit.

Comment 18

[Stefan [:stefanh]]

(In reply to comment #13)

> 1) -moz-eventreerow and -mozoddtreerow for all OS. For eventreerow, I used the
> same color as -moz-Field everywhere except for mac, where I used the OS
> constant (which returns the same color as -moz-Field).

Right, oddrow --> -moz-field for all OS that hasn't been implemented before this

Comment 19

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

so some of those sColorPrefs have been broken for a long time and no-one noticed? Maybe we should just remove them completely.

Comment 20

[Damon Sicore (:damons)]

Roc, any way to get someone other than dbaron to review this?  He's out.

Comment 21

[Reed Loden [:reed]]

(In reply to comment #20)
> Roc, any way to get someone other than dbaron to review this?  He's out.

I'm not roc, but bz and glazou are both style peers. Maybe one of them?
http://www.mozilla.org/owners.html#style-system

Comment 22

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

This is not a good time to bother Boris. I'll just give it my review and if David doesn't like it later, we can fix it or change it.

Comment 23

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

Comment on attachment 316450 [details] [diff] [review]
Simplified version

looks like this is ready to go

Comment 24

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

Comment on attachment 316450 [details] [diff] [review]
Simplified version

a1.9=beltzner

Comment 25

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

I checked this in assuming that Stefan doesn't have CVS access. Thanks!!!