Bug 15360 - Invalid color #{predefined colorName} is accepted by the CSS parser.
Summary: Invalid color #{predefined colorName} is accepted by the CSS parser.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Andras Becsi
URL: http://english.china.com/
Keywords:
Depends on:
Blocks:
 
Reported: 2007-10-03 08:06 PDT by Anantha Keesara
Modified: 2011-04-07 00:46 PDT (History)
11 users (show)

See Also:


Attachments
color:#{predefined colorName} is okay in Safari (179 bytes, text/html)
2007-10-03 08:07 PDT, Anantha Keesara
no flags Details
Potential patch for bug 15360 (4.36 KB, patch)
2008-08-13 09:01 PDT, Glenn Wilson
no flags Details | Formatted Diff | Diff
Potential patch with x-platform layout test (2.46 KB, patch)
2008-08-20 15:57 PDT, Glenn Wilson
no flags Details | Formatted Diff | Diff
Potential fix to bug 15360 with no tabs (4.54 KB, patch)
2008-08-21 14:49 PDT, Glenn Wilson
eric: review-
Details | Formatted Diff | Diff
A color test html page (688 bytes, text/html)
2008-08-22 11:02 PDT, Glenn Wilson
no flags Details
proposed patch (8.28 KB, patch)
2011-04-05 10:41 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anantha Keesara 2007-10-03 08:06:57 PDT
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
Comment 1 Anantha Keesara 2007-10-03 08:07:50 PDT
Created attachment 16521 [details]
color:#{predefined colorName} is okay in Safari
Comment 2 Eric Seidel (no email) 2008-01-14 00:25:14 PST
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.
Comment 3 Glenn Wilson 2008-08-13 09:01:04 PDT
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.
Comment 4 Mark Rowe (bdash) 2008-08-19 21:59:24 PDT
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.
Comment 5 Mark Rowe (bdash) 2008-08-20 15:17:37 PDT
Glenn, see my comment above.
Comment 6 Glenn Wilson 2008-08-20 15:57:38 PDT
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".
Comment 7 Glenn Wilson 2008-08-21 14:49:29 PDT
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!
Comment 8 Eric Seidel (no email) 2008-08-21 16:18:52 PDT
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.
Comment 9 Eric Seidel (no email) 2008-08-21 16:20:51 PDT
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.
Comment 10 Darin Adler 2008-08-22 09:57:57 PDT
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?
Comment 11 Glenn Wilson 2008-08-22 11:02:26 PDT
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)
Comment 12 Darin Adler 2008-08-24 15:16:51 PDT
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.
Comment 13 Mark Rowe (bdash) 2008-09-02 23:51:51 PDT
Landed in r36050.  Please try and avoid using tabs in new layout tests!
Comment 14 mitz 2008-09-04 16:27:57 PDT
Rolled out the patch in <http://trac.webkit.org/changeset/36103> because it broke svg/custom/invalid-fill-hex.svg.
Comment 15 Eric Seidel (no email) 2008-10-06 18:24:07 PDT
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.)
Comment 16 Andras Becsi 2011-04-05 10:41:08 PDT
Created attachment 88275 [details]
proposed patch
Comment 17 Darin Adler 2011-04-05 15:47:11 PDT
Comment on attachment 88275 [details]
proposed patch

What kind of compatibility research have you done on this?
Comment 18 Darin Adler 2011-04-05 15:47:43 PDT
(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.
Comment 19 Darin Adler 2011-04-05 15:49:20 PDT
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?
Comment 20 Andras Becsi 2011-04-06 06:33:44 PDT
(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.
Comment 21 Darin Adler 2011-04-06 07:08:26 PDT
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.
Comment 22 Darin Adler 2011-04-06 07:09:51 PDT
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.
Comment 23 Andras Becsi 2011-04-06 07:14:52 PDT
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.
Comment 24 Andras Becsi 2011-04-06 07:20:08 PDT
(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.
Comment 25 WebKit Review Bot 2011-04-06 08:10:56 PDT
http://trac.webkit.org/changeset/83046 might have broken WinCE Release (Build)
Comment 26 Andras Becsi 2011-04-07 00:44:47 PDT
(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.
Comment 27 Andras Becsi 2011-04-07 00:45:54 PDT
Comment on attachment 88275 [details]
proposed patch

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

Clearing flags.
Comment 28 Andras Becsi 2011-04-07 00:46:58 PDT
All reviewed patches landed, closing bug.