Created attachment 103026 [details] testcase See the attached html file to reproduce the problem. the font-family of the element should not be '}'.
Created attachment 103027 [details] Patch V0
Comment on attachment 103027 [details] Patch V0 Attachment 103027 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9317204
Created attachment 103039 [details] Patch V1(fix mac build failure)
What makes the new behavior correct? Is that what HTML5 says?
Hi Alexey, (In reply to comment #4) > What makes the new behavior correct? Is that what HTML5 says? I glanced http://www.w3.org/TR/CSS21/syndata.html but couldn't find how we should treat it. According to the section 4.1.1, backslash could use to escape characters. So I think we could treat the trailing backslash as escaping empty character. For the first time, I've tried to fix this by inserting space like: setupParser("@-webkit-decls{", string, " } "); However, this involves font-family: ' ' for the testcase and I think this behavior isn't appropriate. I also check how Firefox treat it. Firefox rejects the declaration, which is the same behavior of the patch introduces. Thanks,
Created attachment 118188 [details] Patch
Hi Sam, Simon, Zoltan, I'd appreciate if you could review this patch. Thanks,
Comment on attachment 118188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118188&action=review > Source/WebCore/css/CSSParser.cpp:581 > + if (string.length() > 0 && string[string.length() - 1] == '\\') > + setupParser("@-webkit-decls{", string.substring(0, string.length() - 1), "} "); > + else > + setupParser("@-webkit-decls{", string, "} "); Could we just set string = string.substring(0, string.length() - 1); in the if and then only have one setupParser call?
Comment on attachment 118188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118188&action=review Hi Eric, Thank you for review! I have a question about your comment. >> Source/WebCore/css/CSSParser.cpp:581 >> + setupParser("@-webkit-decls{", string, "} "); > > Could we just set string = string.substring(0, string.length() - 1); in the if and then only have one setupParser call? Since the string is passed as a const variable, we always need a copy of it if we want to have only one setupParser call. Should we do that?
http://dev.w3.org/csswg/css-style-attr/ specs the style attribute and refers to http://www.w3.org/TR/CSS21/syndata.html for the parsing, which includes the following: "In CSS 2.1, a backslash (\) character can indicate one of three types of character escape. Inside a CSS comment, a backslash stands for itself, and if a backslash is immediately followed by the end of the style sheet, it also stands for itself (i.e., a DELIM token). First, inside a string, a backslash followed by a newline is ignored (i.e., the string is deemed not to contain either the backslash or the newline). Outside a string, a backslash followed by a newline stands for itself (i.e., a DELIM followed by a newline)." It's not clear whether the end of the style attribute should be considered the end of the stylesheet, a newline outside of a string or as a newline inside a string. My intuition is that it should be treated like the end of the stylesheet, which is WebKit's current behavior. This probably deserves an email to www-style@w3.org to clarify.
Comment on attachment 118188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118188&action=review >>> Source/WebCore/css/CSSParser.cpp:581 >>> + setupParser("@-webkit-decls{", string, "} "); >> >> Could we just set string = string.substring(0, string.length() - 1); in the if and then only have one setupParser call? > > Since the string is passed as a const variable, we always need a copy of it if we want to have only one setupParser call. Should we do that? You should be able to do: const String& stringWithoutTrailingBackslash = (string.length() > 0 && string[string.length() - 1] == '\\') ? string.substring(0, string.length() - 1) : string; setupParser("@-webkit-decls{", stringWithoutTrailingBackslash, "} "); > LayoutTests/fast/css/declaration-with-trailing-backslash.html:1 > +<p>This page checks that an CSS declaration with trailing backslash is parsed correctly.</p> No DOCTYPE?
Comment on attachment 118188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118188&action=review I'd say r+. The patch looks landable. Please consider making the change I suggested in the previous comment. > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). This line should appear before the description bug after the bug title and the bug url.
Comment on attachment 118188 [details] Patch > This probably deserves an email to www-style@w3.org to clarify. Did someone send this e-mail, as suggested by Ojan? Firefox compatibility alone is not a sufficient reason for changing WebKit. r- since this change lacks rationale.
Comment on attachment 118188 [details] Patch I'd really like to see this discussed on www-style before we land it. Or, at the very least, an analysis of what the latest versions of IE/Firefox/Opera do with this.