Bug 15360 – Invalid color #{predefined colorName} is accepted by the CSS parser.

• Component: CSS
• Status: RESOLVED FIXED

[Anantha Keesara]

I .Steps:

1. Go to: http://english.china.com/
2. Click on "Rainstorm in Southern China" on left menu

II.Issue:

Notice under the highlights section that the link is in a different color

III. Other browsers:

IE, FF, Opera: ok
Safari: not ok

Attached is the reduction

[Anantha Keesara]

Created attachment 16521 [details]
color:#{predefined colorName} is okay in Safari

[Eric Seidel (no email)]

I think this occurs because of these two things:

  | '#' maybe_space { $$.id = 0; $$.string = ParseString(); $$.unit = CSSPrimitiveValue::CSS_RGBCOLOR; } /* Handle error case: "color: #;" */

in CSSGrammar.y

And this, in CSSParser::parseColorFromValue:

    } else if (value->unit == CSSPrimitiveValue::CSS_RGBCOLOR ||
                value->unit == CSSPrimitiveValue::CSS_IDENT ||
                (!strict && value->unit == CSSPrimitiveValue::CSS_DIMENSION)) {
        if (!CSSParser::parseColor(domString(value->string), c, strict && value->unit == CSSPrimitiveValue::CSS_IDENT))
            return false;

CSSParser::parseColor is multi-use and doesn't know that the value being passed in is an RGBCOLOR instead of an IDENT.  Still should rather easy to fix with a little bit of thought.

[Glenn Wilson]

Created attachment 22774 [details]
Potential patch for bug 15360

In the bison template CSSGrammar.y, "hexcolor" was defined as both "HEX maybe_space" OR "IDENT maybe_space".  This caused identifiers not fitting the appropriate hex format, but preceeded by a '#', to be interpreted as a valid color (CSSPrimitiveValue::CSS_PARSER_HEXCOLOR), when it was really just an ignorable token.

To correct this, "IDENT maybe_space" was removed from "hexcolor" and added under "term" as "'#' IDENT maybe_space", which is then processed as a CSSPrimitiveValue::CSS_STRING instead of CSSPrimitiveValue::CSS_PARSER_HEXCOLOR.  Thus, the value will still be processable in CSSParser in the future, but as a CSS_STRING and not a CSS_PARSER_HEXCOLOR or CSS_PARSER_IDENT.

"IDENT maybe_space" is already being matched earlier in "term", and "'#' maybe_space" is matched after, so both edge cases are already being matched.

The expected value for the layout test is added under platform/win right now, but I'd also like to be able to add the expected value for platform/mac next.

[Mark Rowe (bdash)]

Comment on attachment 22774 [details]
Potential patch for bug 15360

It should be possible to make the layout test result portable by changing the test to dumpAsText and using getComputedStyle to determine whether the link was styled as red.

[Mark Rowe (bdash)]

Glenn, see my comment above.

[Glenn Wilson]

Created attachment 22905 [details]
Potential patch with x-platform layout test

Rookie mistake on my part :)  Thanks for the tip!

Here's the patch with a layout test that's much simpler, using "dumpAsText" and "getComputedStyle".

[Glenn Wilson]

Created attachment 22925 [details]
Potential fix to bug 15360 with no tabs

The previous patch had tabs in it, and the changelogs got omitted.  This patch should be correct (let me know if I missed any tabs in there...)

Thanks!

[Eric Seidel (no email)]

Comment on attachment 22925 [details]
Potential fix to bug 15360 with no tabs

The change looks fine.  The test could still be "better"

<p id="test" style="color:#red">FAIL - testing that #color is ignored as a color name, bug 15360</p>
<script>
if (window.layoutTestController)
   layoutTestController.dumpAsText();
var test = document.getElementById("test");
if (window.getComputedStyle(test).color == "rgb(0,0,0)") {
    test.innerText = "PASS";
}
</script>

would be more concise.  and shows only PASS/FAIL (to make it hopefully easier to understand at a glance.

Even better would be to use a .js test, but that's a bit trickier for your first patch (due to our lack of documentation on the subject, fast/js/resources has a bunch of examples, make-js-test-wrappers is the script which generates the .html files to run the .js tests from the TEMPLATE.html files).  We'll talk about .js tests some time over IRC. :)

This is fine to land as-is though.

[Eric Seidel (no email)]

Comment on attachment 22925 [details]
Potential fix to bug 15360 with no tabs

I wonder though if there are any hex colors this could break?

Setting back to r=? for a second opinion.

[Darin Adler]

Comment on attachment 22925 [details]
Potential fix to bug 15360 with no tabs

I believe this change was originally made to match behavior in other browsers. They seem to interpret strings as hex even if they have non-hex characters in them.

Have you tested the behavior of IE and Firefox on these?

[Glenn Wilson]

Created attachment 22943 [details]
A color test html page

Here is a test page I cooked up which contains both valid and invalid colors in both hex and predefined values.

Testing on WinXP SP2, with IE7, FF3, and Safari, the only difference is that Safari interprets "color:#red" where the other browsers do not. 

Also, invalid hex does not seem to be interpreted in any browser.  That is, "#FF00RR", "FF00RR", "FF0000RR" and "#FF0000RR" are all ignored and the color remains the default color.    

Running the layout / reduction on WinXPSP2:
IE7:  style="color:#red;" is ignored (Layout test text remains black)
FF3:  style="color:#red;" is ignored (Layout test text remains black)
FF2:  style="color:#gray;" is ignored (test reduction link not gray)
Safari: style="color:#red;" is interpreted (Layout test text is red)

[Darin Adler]

Comment on attachment 22925 [details]
Potential fix to bug 15360 with no tabs

r=me

By the way, at this point there's no need to have the hexcolor production.

[Mark Rowe (bdash)]

Landed in r36050.  Please try and avoid using tabs in new layout tests!

Rolled out the patch in <http://trac.webkit.org/changeset/36103> because it broke svg/custom/invalid-fill-hex.svg.

[Eric Seidel (no email)]

Comment on attachment 22925 [details]
Potential fix to bug 15360 with no tabs

Marking r- since this was rolled out.  (That removes it from our commit queue.)

[Andras Becsi]

Created attachment 88275 [details]
proposed patch

[Darin Adler]

Comment on attachment 88275 [details]
proposed patch

What kind of compatibility research have you done on this?

[Darin Adler]

(In reply to comment #17)
> What kind of compatibility research have you done on this?

Oh, I see that’s already answered a long time ago.

[Darin Adler]

Comment on attachment 88275 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88275&action=review

> Source/WebCore/css/tokenizer.flex:50
> +"#"{hexnum}             {yyTok = HEX; return yyTok;}
>  "#"{ident}              {yyTok = IDSEL; return yyTok;}

Will this break the use of selectors that are also valid hex numbers by putting the HEX production first? For example, does #a still work as a selector?

[Andras Becsi]

(In reply to comment #19)
> (From update of attachment 88275 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88275&action=review
> 
> > Source/WebCore/css/tokenizer.flex:50
> > +"#"{hexnum}             {yyTok = HEX; return yyTok;}
> >  "#"{ident}              {yyTok = IDSEL; return yyTok;}
> 
> Will this break the use of selectors that are also valid hex numbers by putting the HEX production first? For example, does #a still work as a selector?

This case is handled by the specifier production in CSSGrammar.y, which besides of IDSELs has a rule for HEX tokens, too. So the given example still works as a selector.

[Darin Adler]

Comment on attachment 88275 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88275&action=review

It’s kind of wasteful that the CSS parser already knows it’s hex in the hexcolor case, but still does so much work to process other color formats. Not new, of course, but could be so much more efficient given the lexer has already told us it's a hex number.

> Source/WebCore/css/tokenizer.flex:49
> +"#"{hexnum}             {yyTok = HEX; return yyTok;}

I think {h}+ would read just as well as {hexnum} if not better, so I don’t think we really need hexnum.

[Darin Adler]

Comment on attachment 88275 [details]
proposed patch

I am concerned that this changes error behaves. After all, we have a production that explicitly allows "#" with no digits after it as part of handling an error case. So I wonder if error case handling is changed now because some kinds of strings that begin with "#" can't even be parsed any more. While I don’t fully understand error handling in the parser and lexer, I suspect this does change error handling behavior.

[Andras Becsi]

Comment on attachment 88275 [details]
proposed patch

Thanks Darin for the quick review. Setting cq- to rename {hexnum} to {h}+, and landing it by hand.

[Andras Becsi]

(In reply to comment #22)
> (From update of attachment 88275 [details])
> I am concerned that this changes error behaves. After all, we have a production that explicitly allows "#" with no digits after it as part of handling an error case. So I wonder if error case handling is changed now because some kinds of strings that begin with "#" can't even be parsed any more. While I don’t fully understand error handling in the parser and lexer, I suspect this does change error handling behavior.

I'll watch the bots when landing. SVG depends heavily on the CSS parser's error handling so when there are regressions, this should show up on bots. At least on the Qt env I did not see any regressions, but of course Qt doesn't have full coverage yet.

[WebKit Review Bot]

http://trac.webkit.org/changeset/83046 might have broken WinCE Release (Build)

[Andras Becsi]

(In reply to comment #25)
> http://trac.webkit.org/changeset/83046 might have broken WinCE Release (Build)

False alarm, this was caused by an svn exception.

[Andras Becsi]

Comment on attachment 88275 [details]
proposed patch

Landed in http://trac.webkit.org/changeset/83046.

Clearing flags.

[Andras Becsi]

All reviewed patches landed, closing bug.