RESOLVED DUPLICATE of bug 150853 39140
Add support for 4 and 8 hexit CSS hexcolor values
https://bugs.webkit.org/show_bug.cgi?id=39140
Summary Add support for 4 and 8 hexit CSS hexcolor values
Tab Atkins
Reported 2010-05-14 15:04:43 PDT
Feature is proposed for CSS3 Color, worst case will be in CSS4 Color.
Attachments
Patch with tests (6.01 KB, patch)
2010-05-14 15:12 PDT, Tab Atkins
darin: review-
darin: commit-queue-
Patch with tests (6.47 KB, patch)
2010-05-14 16:28 PDT, Tab Atkins
no flags
Patch with tests (6.64 KB, patch)
2010-05-14 17:16 PDT, Tab Atkins
no flags
Patch with tests (6.67 KB, patch)
2010-05-14 17:47 PDT, Tab Atkins
eric: review-
Tab Atkins
Comment 1 2010-05-14 15:12:51 PDT
Created attachment 56115 [details] Patch with tests
Darin Adler
Comment 2 2010-05-14 15:22:56 PDT
Comment on attachment 56115 [details] Patch with tests The patch has tabs in it, so it can't be landed. What's the compatibility implication of this? What do existing browsers do when confronted with colors in these formats? Can we prove to ourselves that there are not some sites unknowingly depending on this? Have any other browsers added support for this yet. > + if (length == 6) { > + alpha = 0xFF; > + } > + if (length == 3) { > + alpha = 0xF; > + } WebKit coding style does not use braces around single line if statement bodies. > +var testElement = document.createElement('div'); > +for (var i = 0; i < inputs.length; ++i) { > + testElement.style.color = inputs[i]; > + shouldBeEqualToString('testElement.style.color', expected[i]); > +} This is not a great way to write a test, because the test output won't contain any indication of what's being tested. Instead, I suggest writing this: shouldBeEqualToString('testElement.style.color = "' + inputs[i] + '"; testElement.style.color", expected[i]); Then the test output will show what's being tested. Or you can make it even cleaner by using a function: shouldBeEqualToString('testColorRoundTrip("' + inputs[i] + '")", expected[i]); There also was something wrong with the expected test result you attached here. Not sure what went wrong exactly. review- because of the tabs. Please consider my other comments.
Tab Atkins
Comment 3 2010-05-14 16:28:59 PDT
Created attachment 56120 [details] Patch with tests Corrected tab issues, addressed other stylistic comments. Testing shows that current Opera, Firefox, and Webkit all ignore 4 and 8 hexit colors. IE8 ignores 8 hexit colors, but treats 4 hexit colors as white.
Dumitru Daniliuc
Comment 4 2010-05-14 17:09:50 PDT
(In reply to comment #3) > Corrected tab issues i think you still have some tabs in 4-or-8-hexit-colors.js.
Tab Atkins
Comment 5 2010-05-14 17:16:19 PDT
Created attachment 56126 [details] Patch with tests Fixed final tab issues, and made a manual check through the patch for further tabs. I think the issues with my expected output file are due to me working on a windows machine but working through cygwin. The file, as it exists, appears to be what's required for tests to pass on my machine. I can renormalize the linebreaks to unix-style if necessary, though.
Darin Adler
Comment 6 2010-05-14 17:17:14 PDT
(In reply to comment #3) > Testing shows that current Opera, Firefox, and Webkit all ignore 4 and 8 hexit colors. IE8 ignores 8 hexit colors, but treats 4 hexit colors as white. The fact that current browsers ignore these colors means that a site author might accidentally specify one of these, and it will be harmless until we start supporting it. In the past, such changes have resulted in websites that worked in all other browsers but not in Safari. I'm not sure what the best way to mitigate this risk is.
Michael Nordman
Comment 7 2010-05-14 17:26:01 PDT
Comment on attachment 56120 [details] Patch with tests > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 59490) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,16 @@ > +2010-05-14 Tab Atkins <tabatkins@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Adds support for 4 and 8 hexit CSS hexcolors > + https://bugs.webkit.org/show_bug.cgi?id=39140 > + > + Tested by fast/css/4-or-8-hexit-colors.html > + > + * css/tokenizer.flex: > + * platform/graphics/Color.cpp: > + (WebCore::Color::parseHexColor): > + > 2010-05-14 Eric Seidel <eric@webkit.org> > > Unreviewed, rolling out r59489. > Index: WebCore/platform/graphics/Color.cpp > =================================================================== > --- WebCore/platform/graphics/Color.cpp (revision 59365) > +++ WebCore/platform/graphics/Color.cpp (working copy) > @@ -129,21 +129,36 @@ > bool Color::parseHexColor(const String& name, RGBA32& rgb) > { > unsigned length = name.length(); > - if (length != 3 && length != 6) > + if (length != 3 && length != 4 && length != 6 && length != 8) > return false; > unsigned value = 0; > + unsigned alpha = 0; > for (unsigned i = 0; i < length; ++i) { > if (!isASCIIHexDigit(name[i])) > return false; > value <<= 4; > value |= toASCIIHexValue(name[i]); > } > - if (length == 6) { > - rgb = 0xFF000000 | value; > + > + if (length == 8) { > + alpha = 0xFF & value; > + value >>= 8; > + } > + if (length == 6) > + alpha = 0xFF; > + if (length == 4) { > + alpha = 0xF & value; > + value >>= 4; > + } > + if (length == 3) > + alpha = 0xF; > + > + if (length == 6 || length == 8) { > + rgb = alpha << 24 | value; > return true; > } > - // #abc converts to #aabbcc > - rgb = 0xFF000000 > + // #abc converts to #aabbcc, #abcd converts to #aabbccdd > + rgb = alpha << 28 | alpha << 24 > | (value & 0xF00) << 12 | (value & 0xF00) << 8 > | (value & 0xF0) << 8 | (value & 0xF0) << 4 > | (value & 0xF) << 4 | (value & 0xF); > Index: WebCore/css/tokenizer.flex > =================================================================== > --- WebCore/css/tokenizer.flex (revision 59365) > +++ WebCore/css/tokenizer.flex (working copy) > @@ -13,7 +13,7 @@ > nmchar [_a-zA-Z0-9-]|{nonascii}|{escape} > string1 \"([\t !#$%&(-~]|\\{nl}|\'|{nonascii}|{escape})*\" > string2 \'([\t !#$%&(-~]|\\{nl}|\"|{nonascii}|{escape})*\' > -hexcolor {h}{3}|{h}{6} > +hexcolor {h}{3}|{h}{4}|{h}{6}|{h}{8} > > ident -?{nmstart}{nmchar}* > name {nmchar}+ > Index: LayoutTests/fast/css/script-tests/4-or-8-hexit-colors.js > =================================================================== > --- LayoutTests/fast/css/script-tests/4-or-8-hexit-colors.js (revision 0) > +++ LayoutTests/fast/css/script-tests/4-or-8-hexit-colors.js (revision 0) > @@ -0,0 +1,32 @@ > +description( > +'This test checks if CSS 4-hexit and 8-hexit color values are handled correctly..' > +); > + > +var inputs = ["#0000ffff", > + "#0000ff00", > + "#00ff", > + "#00f0", > + "#f00f", > + "#0f0f", > + "#ff0000ff", > + "#00ff00ff", > + "#0000ff80", > + "#00f8"]; > +var expected = ["rgb(0, 0, 255)", > + "rgba(0, 0, 255, 0)", > + "rgb(0, 0, 255)", > + "rgba(0, 0, 255, 0)", > + "rgb(255, 0, 0)", > + "rgb(0, 255, 0)", > + "rgb(255, 0, 0)", > + "rgb(0, 255, 0)", > + "rgba(0, 0, 255, 0.5)", > + "rgba(0, 0, 255, 0.53125)"]; > + > +var testElement = document.createElement('div'); > +for (var i = 0; i < inputs.length; ++i) { > + testElement.style.color = inputs[i]; > + shouldBeEqualToString('testElement.style.color = "' + inputs[i] + '"; testElement.style.color', expected[i]); > +} > + > +successfullyParsed = true; > Index: LayoutTests/fast/css/4-or-8-hexit-colors.html > =================================================================== > --- LayoutTests/fast/css/4-or-8-hexit-colors.html (revision 0) > +++ LayoutTests/fast/css/4-or-8-hexit-colors.html (revision 0) > @@ -0,0 +1,13 @@ > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > +<html> > +<head> > +<link rel="stylesheet" href="../js/resources/js-test-style.css"> > +<script src="../js/resources/js-test-pre.js"></script> > +</head> > +<body> > +<p id="description"></p> > +<div id="console"></div> > +<script src="script-tests/4-or-8-hexit-colors.js"></script> > +<script src="../js/resources/js-test-post.js"></script> > +</body> > +</html> > Index: LayoutTests/fast/css/4-or-8-hexit-colors-expected.txt > =================================================================== > --- LayoutTests/fast/css/4-or-8-hexit-colors-expected.txt (revision 0) > +++ LayoutTests/fast/css/4-or-8-hexit-colors-expected.txt (revision 0) > @@ -0,0 +1,38 @@ > +This test checks if CSS 4-hexit and 8-hexit color values are handled correctly.. + > + + > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". + > + + > + + > +PASS testElement.style.color = "#0000ffff"; testElement.style.color is "rgb(0, 0, 255)" + > +PASS testElement.style.color = "#0000ff00"; testElement.style.color is "rgba(0, 0, 255, 0)" + > +PASS testElement.style.color = "#00ff"; testElement.style.color is "rgb(0, 0, 255)" + > +PASS testElement.style.color = "#00f0"; testElement.style.color is "rgba(0, 0, 255, 0)" + > +PASS testElement.style.color = "#f00f"; testElement.style.color is "rgb(255, 0, 0)" + > +PASS testElement.style.color = "#0f0f"; testElement.style.color is "rgb(0, 255, 0)" + > +PASS testElement.style.color = "#ff0000ff"; testElement.style.color is "rgb(255, 0, 0)" + > +PASS testElement.style.color = "#00ff00ff"; testElement.style.color is "rgb(0, 255, 0)" + > +PASS testElement.style.color = "#0000ff80"; testElement.style.color is "rgba(0, 0, 255, 0.5)" + > +PASS testElement.style.color = "#00f8"; testElement.style.color is "rgba(0, 0, 255, 0.53125)" + > +PASS successfullyParsed is true + > + + > +TEST COMPLETE + > + + > Index: LayoutTests/ChangeLog > =================================================================== > --- LayoutTests/ChangeLog (revision 59492) > +++ LayoutTests/ChangeLog (working copy) > @@ -1,3 +1,14 @@ > +2010-05-14 Tab Atkins <tabatkins@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Tests for 4 and 8 hexit CSS hexcolors. > + https://bugs.webkit.org/show_bug.cgi?id=39140 > + > + * fast/css/4-or-8-hexit-colors-expected.txt: Added. > + * fast/css/4-or-8-hexit-colors.html: Added. > + * fast/css/script-tests/4-or-8-hexit-colors.js: Added. > + > 2010-05-14 Eric Seidel <eric@webkit.org> > > Unreviewed, rolling out r59489. WebCore/platform/graphics/Color.cpp:154 + alpha = 0xF; Since only one of these branches will get taken, I'd use a construct that doesn't test for all of the conditions once that one branch is found. Either an if else or a switch maybe?
Tab Atkins
Comment 8 2010-05-14 17:47:23 PDT
Created attachment 56127 [details] Patch with tests @Micheal: Ah, now I feel silly. I have no idea why I went with serial ifs over a switch. Hopefully I got the webkit style with regards to switch indentation. @Darin: I'm running some tests right now to see if there is any significant usage of 4 or 8 hexit colors on the web. Hopefully I'll see some results over the weekend.
Darin Adler
Comment 9 2010-05-14 19:09:34 PDT
(In reply to comment #5) > I think the issues with my expected output file are due to me working on a windows machine but working through cygwin. The file, as it exists, appears to be what's required for tests to pass on my machine. I can renormalize the linebreaks to unix-style if necessary, though. You are going to need to do something along those lines. Otherwise the test will fail on all the buildbots and all other machines besides yours!
Dave Hyatt
Comment 10 2010-05-15 16:13:25 PDT
I'm a bit nervous about making syntactic changes to color parsing when this is not even in any draft yet. This can't be prefixed, so we need to be careful here. I'd prefer to see this in a draft before we make this change.
Eric Seidel (no email)
Comment 11 2010-05-16 00:40:38 PDT
Comment on attachment 56127 [details] Patch with tests r- based on Hyatt's above comment. This is sensitive code we're dealing with here. :)
Eric Seidel (no email)
Comment 12 2010-05-16 00:41:52 PDT
The course forward (to answer Hyatt's comment) is mostly to wait. To get this into some sort of editor draft or implemented by some other browser(s) before we modify these core color parsing routines.
Boris Zbarsky
Comment 13 2010-05-20 18:29:59 PDT
> Testing shows that current Opera, Firefox, and Webkit all ignore 4 and 8 > hexit colors. Tab, I don't think that's true. Simple testcase: data:text/html,<font color="%23000000ff">This is blue</font> The text is interoperably blue in Webkit and Gecko (with and without the '#') and black in Opera. With your patch it would render black, right? The key here, at least in Gecko, is quirks mode. Webkit seems to do the same thing (blue) in both modes. Another testcase: data:text/html,<font color="0000">This is black</font> The text is solid black in Webkit/Gecko/Opera; in Webkit and Opera this would even happen in standards mode. With your patch it would be transparent, right?
Tab Atkins
Comment 14 2010-05-21 08:51:11 PDT
Right, we fall back to other routines when parsing colors out of HTML. To avoid any compat issues in that direction, I'd have to find and patch the area where we look for HTML color values to either use a different, HTML-specific function, or to fail early and switch over to the legacy-parsing function immediately.
Andras Becsi
Comment 15 2011-03-23 11:31:55 PDT
*** Bug 56461 has been marked as a duplicate of this bug. ***
Simon Pieters (:zcorpan)
Comment 16 2012-10-08 03:51:14 PDT
(dataset: http://dotnetdotcom.org/ ) $ grep -aPic "(background|color|outline|border)\s*:\s*#([0-9a-f]{4}){1,2}($|\s|;|,|})" web200904 177 This is a relatively low number, though the search does not include cases where the color is not the first token in the value (e.g. border:1px #f000 solid), nor does it include external style sheets. IMHO this syntax isn't clear and people get the wrong number of digits at times. It also doesn't work together with keywords. Using a separator character might be better (e.g. color: orange * 0.7).
Ian Thomas (thelem)
Comment 17 2015-11-23 07:40:22 PST
Dean Jackson
Comment 18 2016-08-03 13:23:42 PDT
*** This bug has been marked as a duplicate of bug 150853 ***
Note You need to log in before you can comment on or make changes to this bug.