Bug 6129 – Incomplete implementation of CSS 2.1 system colors

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Seth]

WebKit displays incomplete and incorrect colors when CSS 2.1 system colors are specified. OS X 
exposes a serios of named colors in the standard Colors palette which to not align with those displayed 
in a browser. To show this I have linked two files. The first is an image constructed comparing Safari / 
Flock on OS X and IE / Firefox on Windows. The image also includes notes where  The second is a test 
file (HTML with embeded CSS) for testing with development builds.

The image is at the following address:
http://designgods.net/published_pics/webkit/browser-system-colors.jpg

The test file is at the following address:
http://designgods.net/published_pics/webkit/css-color-test.html

[Seth]

Created attachment 5128 [details]
Color Testing HTML File

[Seth]

Created attachment 5129 [details]
Browser comparison chart and recomended colors

Image constructed with screenshots of test HTML file on 17-Dec-2005. Labels
next to recommended colors provided by the OS X Colors palette.

[Joost de Valk (AlthA)]

Confirmed. Good testcase.

*** Bug 11037 has been marked as a duplicate of this bug. ***

[Dave Hyatt]

This should be implemented by RenderTheme (just as I recently did for system fonts for Mac and Win).

[Paul]

This issue is also noticable in Safari 3 from Windows.

Imho severly hampers the ability to create browser apps that look like the user preferences.

On all other major browsers on Windows the System Colors do work properly, allowing you to use them and give your webapp a consistent L&F according to the users OS preferences.

Regards,

Paul

[Rob Buis]

Created attachment 15614 [details]
First attempt

This patch uses the same code basically as Mozilla codebase, old carbon calls. Not surprisingly that it matches FF2 with this test:

http://bugs.webkit.org/attachment.cgi?id=5128&action=view

There are some things to clear up:

- is it ok to use the old Carbon calls? I guess NSColor could workI guess but it needs more lines.
- already since the patch changes ButtonFace value on my system so a lot of regressions are there in the  non-pixel tests, so a lot of tests would need to be updated.
- would we get system color dependent test results after this patch? How to do an added testcase if it would depend on the system the test runs on? I am not sure how much the system colors can be changed by the average user, but at least highlight color can be changed.
Cheers,

Rob.

[Darin Adler]

I'm sure that many of these old Carbon calls are not available in 64-bit, and WebKit compiles for 64-bit, so that's one issue we'll have to resolve.

[Mark Rowe (bdash)]

Comment on attachment 15614 [details]
First attempt

I think it's possible to override the system highlight colours via a user default, so we should be able to force DRT to run with a consistent colour on all machines.

As Darin mentioned, some of the Carbon APIs that you have used are not available for 64-bit.  It would be great to see an updated version of this patching using the equivalent Cocoa APIs, even it does take a little more code.

[Andrew Wellington]

Created attachment 16501 [details]
Possible patch

This patch causes the colour of a button to change -- thus changing a lot of layout test results.

Other than that, all we're doing is fetching the system colours from NSColor, including a kind-of-nasty-but-only-way of getting pattern-based colours (which we can't ask for the appropriate RGB components) by drawing a 1x1 square and getting the colour of that pixel to determine the colour.

[Adam Roben (:aroben)]

We'll want to implement this for all our platforms (attachment 16501 [details] is Mac-only), but that doesn't mean we have to do them all in one checkin.

Note that the colors listed for Windows in attachment 5129 [details] are for the Windows XP Olive Green theme. They should not be treated as constants.

[Sam Weinig]

Perhaps we should be using the GetThemeColor() API on windows. 

[Adam Roben (:aroben)]

Comment on attachment 16501 [details]
Possible patch

+        else if (ident == CSS_VAL_GREY)
+            col = 0xFF808080;

Why not keep grey in the colorValues array?

+        color = [NSColor colorWithCalibratedRed:((float)pixel[0]) / 256.0
+                                          green:((float)pixel[1]) / 256.0
+                                           blue:((float)pixel[2]) / 256.0
+                                          alpha:((float)pixel[3]) / 256.0];
+        [offscreenRep release];
+    } 
+    return Color(static_cast<int>(256.0 * [color redComponent]), static_cast<int>(256.0 * [color greenComponent]), static_cast<int>(256.0 * [color blueComponent]));

I believe you should replace every occurrence of 256.0 with 255.0f.

+void RenderThemeMac::systemColor(int propId, Color& color) const
+{
+    if (m_systemColorCache.contains(propId))
+        color = m_systemColorCache.get(propId);

It would be good to return right here so the long switch statement doesn't have to be in an else block.

r- so the above can be fixed. The design looks good overall. We're also going to have to consider whether this will break existing web sites.

[Andrew Wellington]

Created attachment 17440 [details]
Proposed patch 2

Changed the design to use the lookup table again, decided the design that used the unlabelled values in colorValues wasn't as nice as the proper pairs. This let gray/grey go back into the normal lookup.

The values that are 256 are that specifically because otherwise a number of colours end up rounding in such a way that values are off-by-one. Perhaps we should add a comment to the code to indicate this as it's a little unusual that it's not 255?

Added early return to RenderThemeMac::systemColor.

As to website compatibility, on Mac OS X at least all colours that feature in layout tests in this are the same except that the control grey would change from 0xC0C0C0 to 0xECECEC. Should we go with this change, or just hard code the old value instead of using [NSColor controlColor] for it?

No changelog/layout tests until the above questions are answered (the layout test changes are fairly long with the change from 0xC0C0C0 to 0xECECEC. I'll add them back when we get them answered :-)

[Sam Weinig]

Comment on attachment 17440 [details]
Proposed patch 2

This seems really good.  A few nits though.

I think a comment explain *why* this can fail (and will fail) would be really informative and helpful. 
+    NSColor* color = [theColor colorUsingColorSpaceName:NSCalibratedRGBColorSpace];
+    if (!color) {

Please use c++ style static_casts here and below.  The 256's should also be 256.0f.
+        color = [NSColor colorWithCalibratedRed:((float)pixel[0]) / 256

These should also be 256.0f I think.  I am also surprised that we don't have a constructor for Color that takes an NSColor.
+    return Color(static_cast<int>(256.0 * [color redComponent]), static_cast<int>(256.0 * [color greenComponent]), static_cast<int>(256.0 * [color blueComponent]));

This could use a comment.
+        case CSS_VAL_INFOBACKGROUND:
+            color = 0xFFFBFCC5;


Is returning here ok?  Does this case ever get hit?
+        default:
+            return;

Do you need to check for validity again?  Does RenderTheme::systemColor ever return an invalid color?
+    if (color.isValid())
+        m_systemColorCache.set(propId, color);


I don't think this comment adds anything.
+    // System color
+    virtual void systemColor(int propId, Color&) const;


You need a space between the switch and the (.

+void RenderTheme::systemColor(int propId, Color& color) const
+{
+    switch(propId) {

There is an extra space before color. 
+        case CSS_VAL_WINDOWTEXT:
+             color = 0xFF000000;


I am not thrilled with this line, maybe it should go in colorForCSSValue(ident).
+        if (!col.isValid())
+            theme()->systemColor(ident, col);

[Andrew Wellington]

Created attachment 17571 [details]
Final (hopefully) patch

Although Weinig reviewed the previous version I've made a slight functional change: two colours that show up in the existing layout tests were hard coded back to the old values to ensure website compatibility. We can look at changing them seperately after this patch is done.

Also includes a layout test now :-)

[Adam Roben (:aroben)]

Comment on attachment 17571 [details]
Final (hopefully) patch

 232     return Color(static_cast<int>(256.0 * [color redComponent]), static_cast<int>(256.0 * [color greenComponent]), static_cast<int>(256.0 * [color blueComponent]));

You should be multiplying by 255 instead of 256.0

[Andrew Wellington]

(In reply to comment #17)
> (From update of attachment 17571 [details] [edit])
>  232     return Color(static_cast<int>(256.0 * [color redComponent]),
> static_cast<int>(256.0 * [color greenComponent]), static_cast<int>(256.0 *
> [color blueComponent]));
> 
> You should be multiplying by 255 instead of 256.0
> 

(From comment 14)
The values that are 256 are that specifically because otherwise a number of
colours end up rounding in such a way that values are off-by-one. Perhaps we
should add a comment to the code to indicate this as it's a little unusual that
it's not 255?

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 17571 [details] [edit] [edit])
> >  232     return Color(static_cast<int>(256.0 * [color redComponent]),
> > static_cast<int>(256.0 * [color greenComponent]), static_cast<int>(256.0 *
> > [color blueComponent]));
> > 
> > You should be multiplying by 255 instead of 256.0
> > 
> 
> (From comment 14)
> The values that are 256 are that specifically because otherwise a number of
> colours end up rounding in such a way that values are off-by-one. Perhaps we
> should add a comment to the code to indicate this as it's a little unusual that
> it's not 255?
> 

Perhaps you're looking to multiply by nextafter(256.0, 0.0) (see Color.cpp).

[Darin Adler]

Comment on attachment 17571 [details]
Final (hopefully) patch

     (static_cast<float>(pixel[0])) / 256

Despite the reason you offered, it's not correct to divide by 256. That turns a 0xFF into 255.0/256.0 rather than 1.0. That's the same as the reason you see use of 255.0f in the nsColor function in ColorMac.mm. Same for the multiplication at the end of the function. This should just use the Color functions that already exist as much as possible and should not involve new code. See Mitz's comment for some hints about how you could fix this.

I also think it would be better to convert the pixels directly into a Color rather than making the pixel, then turning the pixel into an NSColor, then extracting the channel values again to make a Color.

+        [NSGraphicsContext saveGraphicsState];

I believe the call to saveGraphicsState is neither necessary nor sufficient. We need to save ad restore the current context. The saveGraphicsState method saves the state *of* the current context. Since we're making a new context, what we need to save is what the current context is, not state within it.

+            // take system default

I don't think you mean "system" here. You mean take the theme-independent default?

+            // Using this value instead of controlColor to avoid changing existing button colors

The verb tense here is confusing. Please write a comment that will make sense in the future, rather than one that describes the current change.

+            // There is no NSColor for this so we use the previous hard coded value

The word previous here won't make sense in the future. Please find another way to word this.

[Andrew Wellington]

Created attachment 17908 [details]
Proposed patch

Use nextafter(256.0, 0.0), fixed the silly rounding issue. Thanks mitz!

Fixed a couple of comments Darin mentioned to be more clear hopeflly.

[NSGraphicsContext saveGraphicsState]; is both necessary and sufficient as far as my reading of its documentation goes. "saveGraphicsState" is documented as such: "This method sends the current graphics context a saveGraphicsState message and pushes the context onto the per-thread stack." To match, "restoreGraphicsState" does the reverse: "Pops a graphics context from the per-thread stack, makes it current, and sends the context a restoreGraphicsState message."

I believe this is necessary (we change the graphics state otherwise), and is sufficient as we push/pop the appropriate graphics context.

[Darin Adler]

Comment on attachment 17908 [details]
Proposed patch

Looks good. I'm saying review+, but there's one issue that needs to be fixed. This will break 64-bit unless we change one use of unsigned to NSUInteger.

Yes, you were right and I was wrong about +[NSGraphicsContext saveGraphicsState].

I don't see any reason for systemColor to take a Color& instead of returning a Color object. These are small objects and need not be returned using out reference parameters.

RenderTheme::systemColor uses a switch statement instead of an array the way CSSStyleSelector.cpp did. The array seems easier to read than the switch. The switch statement is big and it's hard to see which color has which value.

Color objects are simply RGBA values (integers) plus a valid flag. Because of that, I think m_systemColorCache could store RGBA values instead of color objects. We don't really need a way to store an invalid object in the cache.

We normally use a different position of the * for ObjC classes than C++ classes; this patch has things like NSColor* where we use NSColor * -- I'm not a big fan of the rule but I do like to stay consistent.

I think this rect could just be constructed on the line of code where we paint. We don't really need a 0,0 1x1 rect for the bitmap setup.

 203         NSRect offscreenRect = NSMakeRect(0, 0, 1, 1);

We normally use "unsigned" rather than "unsigned int" in code like this. Also, this should be declared further down in the file, right where it's used. Also, this should be NSUInteger, not "unsigned", so it compiles under 64-bit.

 205         unsigned int pixel[4];

There's no reason to first initialize this to nil and then give it a real value two lines later:

 204         NSBitmapImageRep* offscreenRep = nil;

I think these lines could just say "1" -- there's no real value in getting the size out of the rect and I don't think it makes things a lot clearer.

 207                                                                pixelsWide:offscreenRect.size.width
 208                                                                pixelsHigh:offscreenRect.size.height

I don't think it's helpful to use alpha in the NSBitmapImageRep that we use to figure out what the color is. We don't read the alpha value out, so we shouldn't write it in.

 210                                                           samplesPerPixel:4
 211                                                                  hasAlpha:YES

There's no need to include a typecast here. 0 would work fine without it.

 214                                                              bitmapFormat:(NSBitmapFormat)0

This can just be value 4 rather than doing math, and I think it might be clearer.

 215                                                               bytesPerRow:(4 * offscreenRect.size.width)

[Darin Adler]

Working on landing this.

[Darin Adler]

Committed revision 28775.