Escaped newline handling should be limited to strings

RESOLVED FIXED in mozilla6

Status

()

Core
CSS Parsing and Computation
P4
normal
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: Eli Friedman, Assigned: dbaron)

Tracking

(Blocks: 1 bug)

Trunk
mozilla6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Take a rule like the following:

background: r\
gb(0,0,0);

Mozilla accepts this as a background declaration, but according to the CSS rules escaped newlines are only ignored in strings.

This does bring about the interesting side question of whether the following:

<style>
.a\
{color: red}
</style>
<div class="a\">a</div>

should result in a red a. (Opera tosses out the whole rule; in IE, the rule matches class "a" following some logic which doesn't seem completely consistent.)
(Reporter)

Comment 1

10 years ago
Answering myself: I'm pretty sure that backslash followed by a newline outside a string should result in a malformed declaration according to the CSS rules.
(Assignee)

Comment 2

10 years ago
Are other browsers that handle escaped newlines in strings ignoring them outside of strings, or should we just change the spec to match the implementations?
(Reporter)

Comment 3

10 years ago
IE's usually, but not always, treats it as some sort of parse error. However, it sometimes ignores the backslash if it's at the end of a token.

Opera consistently treats a backslash followed by a newline outside of a string as a parse error.

Safari also seems to treat this case as a parse error; I haven't tested thoroughly, though.

I think we're the only implementation that handles backslash newline escapes outside of strings.

Comment 4

9 years ago
Addendum:
gecko removes improperly the \00000a within the string.

see:
http://samples.msdn.microsoft.com/csstestpages/Chapter_4/escaped-newline-001.htm
(Assignee)

Comment 5

9 years ago
(In reply to comment #4)
> Addendum:
> gecko removes improperly the \00000a within the string.

Nothing to do with this bug, and see the comments about white-space defaults on :before and :after on public-css-testsuite and www-style right after Microsoft's tests were contributed. (http://lists.w3.org/Archives/Public/)

Comment 6

9 years ago
> backslash followed by a newline outside
> a string should result in a malformed declaration according to the CSS rules

This is also the conclusion we reached here:

[CSS2.1] Clarification on section 4.1.3 and the application of escaped newlines to identifiers
http://lists.w3.org/Archives/Public/www-style/2008Nov/0035.html

Zack Weinberg, Boris Zbarsky and fantasai participated into such discussion.

> DIV
> {
> background-color: gre\
> en;
> }

> There is a note that only FireFox currently supports this behavior, 
> which I believe makes them non-compliant.

"Yeah, the Gecko behavior here is just wrong.  We'll fix it." B. Zbarsky
(Assignee)

Comment 7

9 years ago
The working group discussed this today, and I don't think things were so clear-cut.
(Assignee)

Updated

7 years ago
Blocks: 605520
(Assignee)

Updated

6 years ago
Summary: Escaped newlines ignored in identifiers → Escaped newline handling should be limited to strings
(Assignee)

Updated

6 years ago
Duplicate of this bug: 647917
(Assignee)

Updated

6 years ago
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
OS: Windows XP → All
Priority: -- → P4
Hardware: x86 → All
Target Milestone: --- → mozilla6
(Assignee)

Comment 9

6 years ago
Created attachment 529382 [details] [diff] [review]
patch 1: pass in-string boolean to ParseAndAppendEscape
Attachment #529382 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

6 years ago
Created attachment 529383 [details] [diff] [review]
patch 2: handle failure of GatherIdent and fix up ParseRef
Attachment #529383 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

6 years ago
Created attachment 529384 [details] [diff] [review]
patch 3: introduce failure from ParseAndAppendEscape in the EOF cases, and handle that failure
Attachment #529384 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

6 years ago
Created attachment 529385 [details] [diff] [review]
patch 4: only allow escaped newlines within strings (string token and string in url() token)
Attachment #529385 - Flags: review?(bzbarsky)
(Assignee)

Comment 13

6 years ago
Created attachment 529386 [details] [diff] [review]
patch 5: fix the remaining todo in test_parse_rule.html, a test error
Attachment #529386 - Flags: review?(bzbarsky)
Comment on attachment 529382 [details] [diff] [review]
patch 1: pass in-string boolean to ParseAndAppendEscape

r=me
Attachment #529382 - Flags: review?(bzbarsky) → review+
Comment on attachment 529383 [details] [diff] [review]
patch 2: handle failure of GatherIdent and fix up ParseRef

>Handle failure of GatherIdent, which can happen (starting with the next patch) fail

  s/happen //

or otherwise fix the grammar.

r=me with that.
Attachment #529383 - Flags: review?(bzbarsky) → review+
Comment on attachment 529384 [details] [diff] [review]
patch 3: introduce failure from ParseAndAppendEscape in the EOF cases, and handle that failure

r=me
Attachment #529384 - Flags: review?(bzbarsky) → review+
Comment on attachment 529385 [details] [diff] [review]
patch 4: only allow escaped newlines within strings (string token and string in url() token)

Hmm.  Why do you not need to push back ch in the !aInString case?  And won't this tokenize escaped newline as DELIM for the '\\' and then whitespace?
Comment on attachment 529386 [details] [diff] [review]
patch 5: fix the remaining todo in test_parse_rule.html, a test error

r=me
Attachment #529386 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 19

6 years ago
(In reply to comment #17)
> Comment on attachment 529385 [details] [diff] [review] [review]
> patch 4: only allow escaped newlines within strings (string token and string in
> url() token)
> 
> Hmm.  Why do you not need to push back ch in the !aInString case?  And won't
> this tokenize escaped newline as DELIM for the '\\' and then whitespace?

Er, right, I do need a Pushback() there; without that I'd fail to tokenize the whitespace.  I'm not sure how the difference is testable, though, since we still generate the delim token (per patch 3), and that should cause any parser caller I can think of to fail.
(Assignee)

Comment 20

6 years ago
Created attachment 529796 [details] [diff] [review]
patch 4: only allow escaped newlines within strings (string token and string in url() token)
Attachment #529385 - Attachment is obsolete: true
Attachment #529796 - Flags: review?(bzbarsky)
Attachment #529385 - Flags: review?(bzbarsky)
Comment on attachment 529796 [details] [diff] [review]
patch 4: only allow escaped newlines within strings (string token and string in url() token)

r=me
Attachment #529796 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 22

6 years ago
https://hg.mozilla.org/mozilla-central/rev/c7aa244b92af
https://hg.mozilla.org/mozilla-central/rev/16da10dde7a5
https://hg.mozilla.org/mozilla-central/rev/5a70248eb4ac
https://hg.mozilla.org/mozilla-central/rev/4dd57526d5f6
https://hg.mozilla.org/mozilla-central/rev/fde62ad9cd75
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.