Bug 12145

Summary: Color parsing is too relaxed in strict mode
Product: WebKit Reporter: Sam Weinig <sam>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
URL: http://www.bjoernsworld.de/test/color.html
Attachments:
Description Flags
testcase (strict mode)
none
testcase (quirks mode)
none
First attempt darin: review+

Description Sam Weinig 2007-01-06 16:36:54 PST
The testcase pointed to in the URL field demonstrates a way in which the css color parsing is too relaxed.  In particular, hexidecimal color values without a leading # should be illegal.  This test passes in Firefox 2, IE7 and Opera 9, but not in either shipping Safari or WebKit ToT.
Comment 1 Dave Hyatt 2007-01-06 17:33:11 PST
Try the same test without the doctype (in quirks mode).  Probably a quirk that isn't properly qualified.

Comment 2 Sam Weinig 2007-01-06 18:21:14 PST
(In reply to comment #1)
> Try the same test without the doctype (in quirks mode).  Probably a quirk that
> isn't properly qualified.

You are correct.  In quirks mode Firefox, IE and Opera, the line does get successfully parsed.
Comment 3 Sam Weinig 2007-01-06 18:22:37 PST
Created attachment 12271 [details]
testcase (strict mode)
Comment 4 Sam Weinig 2007-01-06 18:23:18 PST
Created attachment 12272 [details]
testcase (quirks mode)
Comment 5 Rob Buis 2007-01-09 14:02:39 PST
Created attachment 12336 [details]
First attempt

I figured out the Format substitution (http://trac.webkit.org/projects/webkit/changeset/17700) broke
color parsing like color: 33FFFF. Related is handling of FF33333, as shown by the testcases for this bug. This patch should fix both.
Cheers,

Rob.
Comment 6 Darin Adler 2007-01-09 14:15:32 PST
Comment on attachment 12336 [details]
First attempt

Looks fine, r=me.

+    if (Color::parseHexColor(name, rgb) && !strict)
         return true;

It would be nice to not even parse in the strict case. Just reverse the two halves of the &&.

+        static RGBA32 parseColor(const String&, bool = false);

Should have the name strict here, since bool along doesn't make it clear what the parameter is.

+        static bool parseColor(const String&, RGBA32& rgb, bool);

Ditto.
Comment 7 Rob Buis 2007-01-09 14:47:21 PST
Landed in r18722.