Bug 12145 - Color parsing is too relaxed in strict mode
Summary: Color parsing is too relaxed in strict mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL: http://www.bjoernsworld.de/test/color...
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-06 16:36 PST by Sam Weinig
Modified: 2007-01-09 14:47 PST (History)
0 users

See Also:


Attachments
testcase (strict mode) (343 bytes, text/html)
2007-01-06 18:22 PST, Sam Weinig
no flags Details
testcase (quirks mode) (246 bytes, text/html)
2007-01-06 18:23 PST, Sam Weinig
no flags Details
First attempt (28.05 KB, patch)
2007-01-09 14:02 PST, Rob Buis
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.