Add support for 4 and 8 hexit CSS hexcolor values

• Component: CSS
• Status: RESOLVED DUPLICATE

[Tab Atkins]

Feature is proposed for CSS3 Color, worst case will be in CSS4 Color.

[Tab Atkins]

Created attachment 56115 [details]
Patch with tests

[Darin Adler]

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]

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]

(In reply to comment #3)
> Corrected tab issues

i think you still have some tabs in 4-or-8-hexit-colors.js.

[Tab Atkins]

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]

(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 on attachment 56120 [details]
Patch with tests

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 59490)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,16 @@
> [phone]  Tab Atkins  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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 @@
> [phone]  Tab Atkins  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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]

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]

(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]

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 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)]

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]

> 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]

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]

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

[Simon Pieters (:zcorpan)]

(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)]

This is now a duplicate of https://bugs.webkit.org/show_bug.cgi?id=150853

[Dean Jackson]

*** This bug has been marked as a duplicate of bug 150853 ***