Bug 65739 – Mishandling trailing backslash in style attribute

• Component: CSS
• Status: RESOLVED INVALID

[Kenichi Ishibashi]

Created attachment 103026 [details]
testcase

See the attached html file to reproduce the problem. the font-family of the element should not be '}'.

[Kenichi Ishibashi]

Created attachment 103027 [details]
Patch V0

[WebKit Review Bot]

Comment on attachment 103027 [details]
Patch V0

Attachment 103027 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9317204

[Kenichi Ishibashi]

Created attachment 103039 [details]
Patch V1(fix mac build failure)

[Alexey Proskuryakov]

What makes the new behavior correct? Is that what HTML5 says?

[Kenichi Ishibashi]

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,

[Kenichi Ishibashi]

Created attachment 118188 [details]
Patch

[Kenichi Ishibashi]

Hi Sam, Simon, Zoltan,

I'd appreciate if you could review this patch.

Thanks,

[Eric Seidel (no email)]

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?

[Kenichi Ishibashi]

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?

[Ojan Vafai]

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 [e-mail] to clarify.

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

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.

[Alexey Proskuryakov]

Comment on attachment 118188 [details]
Patch

> This probably deserves an email to [e-mail] 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.

[Ojan Vafai]

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.