Bug 39140 - Add support for 4 and 8 hexit CSS hexcolor values
: Add support for 4 and 8 hexit CSS hexcolor values
Status: NEW
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-05-14 15:04 PST by
Modified: 2012-10-08 03:51 PST (History)


Attachments
Patch with tests (6.01 KB, patch)
2010-05-14 15:12 PST, Tab Atkins
darin: review-
darin: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch with tests (6.47 KB, patch)
2010-05-14 16:28 PST, Tab Atkins
no flags Review Patch | Details | Formatted Diff | Diff
Patch with tests (6.64 KB, patch)
2010-05-14 17:16 PST, Tab Atkins
no flags Review Patch | Details | Formatted Diff | Diff
Patch with tests (6.67 KB, patch)
2010-05-14 17:47 PST, Tab Atkins
eric: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-05-14 15:04:43 PST
Feature is proposed for CSS3 Color, worst case will be in CSS4 Color.
------- Comment #1 From 2010-05-14 15:12:51 PST -------
Created an attachment (id=56115) [details]
Patch with tests
------- Comment #2 From 2010-05-14 15:22:56 PST -------
(From update of attachment 56115 [details])
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.
------- Comment #3 From 2010-05-14 16:28:59 PST -------
Created an attachment (id=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.
------- Comment #4 From 2010-05-14 17:09:50 PST -------
(In reply to comment #3)
> Corrected tab issues

i think you still have some tabs in 4-or-8-hexit-colors.js.
------- Comment #5 From 2010-05-14 17:16:19 PST -------
Created an attachment (id=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.
------- Comment #6 From 2010-05-14 17:17:14 PST -------
(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.
------- Comment #7 From 2010-05-14 17:26:01 PST -------
(From update of attachment 56120 [details])
> 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?
------- Comment #8 From 2010-05-14 17:47:23 PST -------
Created an attachment (id=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.
------- Comment #9 From 2010-05-14 19:09:34 PST -------
(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!
------- Comment #10 From 2010-05-15 16:13:25 PST -------
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.
------- Comment #11 From 2010-05-16 00:40:38 PST -------
(From update of attachment 56127 [details])
r- based on Hyatt's above comment.  This is sensitive code we're dealing with here. :)
------- Comment #12 From 2010-05-16 00:41:52 PST -------
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.
------- Comment #13 From 2010-05-20 18:29:59 PST -------
> 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?
------- Comment #14 From 2010-05-21 08:51:11 PST -------
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.
------- Comment #15 From 2011-03-23 11:31:55 PST -------
*** Bug 56461 has been marked as a duplicate of this bug. ***
------- Comment #16 From 2012-10-08 03:51:14 PST -------
(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).