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
Created attachment 16521 [details] color:#{predefined colorName} is okay in Safari
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.
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 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.
Glenn, see my comment above.
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".
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 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 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 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?
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 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.
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.
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.)
Created attachment 88275 [details] proposed patch
Comment on attachment 88275 [details] proposed patch What kind of compatibility research have you done on this?
(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 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?
(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 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 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 on attachment 88275 [details] proposed patch Thanks Darin for the quick review. Setting cq- to rename {hexnum} to {h}+, and landing it by hand.
(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.
http://trac.webkit.org/changeset/83046 might have broken WinCE Release (Build)
(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 on attachment 88275 [details] proposed patch Landed in http://trac.webkit.org/changeset/83046. Clearing flags.
All reviewed patches landed, closing bug.