RESOLVED FIXED 6129
Incomplete implementation of CSS 2.1 system colors
https://bugs.webkit.org/show_bug.cgi?id=6129
Summary Incomplete implementation of CSS 2.1 system colors
Seth
Reported 2005-12-17 21:16:42 PST
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
Attachments
Color Testing HTML File (4.25 KB, text/html)
2005-12-17 21:18 PST, Seth
no flags
Browser comparison chart and recomended colors (107.28 KB, image/jpeg)
2005-12-17 21:19 PST, Seth
no flags
First attempt (13.83 KB, patch)
2007-07-21 12:55 PDT, Rob Buis
mrowe: review-
Possible patch (294.70 KB, patch)
2007-10-02 05:20 PDT, Andrew Wellington
aroben: review-
Proposed patch 2 (13.64 KB, patch)
2007-11-22 02:41 PST, Andrew Wellington
no flags
Final (hopefully) patch (85.09 KB, patch)
2007-11-28 03:15 PST, Andrew Wellington
darin: review-
Proposed patch (85.05 KB, patch)
2007-12-15 02:39 PST, Andrew Wellington
darin: review+
Seth
Comment 1 2005-12-17 21:18:19 PST
Created attachment 5128 [details] Color Testing HTML File
Seth
Comment 2 2005-12-17 21:19:58 PST
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)
Comment 3 2006-02-15 16:18:11 PST
Confirmed. Good testcase.
mitz
Comment 4 2006-09-26 11:27:47 PDT
*** Bug 11037 has been marked as a duplicate of this bug. ***
Dave Hyatt
Comment 5 2006-09-26 12:21:44 PDT
This should be implemented by RenderTheme (just as I recently did for system fonts for Mac and Win).
Paul
Comment 6 2007-07-17 00:40:35 PDT
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
Comment 7 2007-07-21 12:55:46 PDT
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
Comment 8 2007-07-22 12:25:28 PDT
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 9 2007-08-07 14:01:58 PDT
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
Comment 10 2007-10-02 05:20:27 PDT
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)
Comment 11 2007-10-07 16:33:46 PDT
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
Comment 12 2007-10-08 14:06:19 PDT
Perhaps we should be using the GetThemeColor() API on windows.
Adam Roben (:aroben)
Comment 13 2007-10-27 23:10:17 PDT
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
Comment 14 2007-11-22 02:41:29 PST
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 15 2007-11-24 20:15:48 PST
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
Comment 16 2007-11-28 03:15:05 PST
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 17 2007-11-28 09:59:08 PST
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
Comment 18 2007-11-28 16:56:48 PST
(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?
mitz
Comment 19 2007-11-28 17:57:22 PST
(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 20 2007-12-02 10:31:25 PST
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
Comment 21 2007-12-15 02:39:34 PST
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 22 2007-12-16 10:47:51 PST
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
Comment 23 2007-12-16 11:20:51 PST
Working on landing this.
Darin Adler
Comment 24 2007-12-16 11:26:24 PST
Committed revision 28775.
Note You need to log in before you can comment on or make changes to this bug.