Bug 21472 – escaped characters cause parse errors in idents (for example in @namespace rules)

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Anne van Kesteren]

@import is not ignored despite it coming after @namespace.

http://dev.w3.org/csswg/css3-namespace/#syntax

[Simon Hollingshead]

Proof of issue shown at w3 testsuite case http://www.w3.org/Style/CSS/Test/CSS3/Namespace/20090210/syntax-006.xml

[Alexey Proskuryakov]

*** Bug 38587 has been marked as a duplicate of this bug. ***

[Daniel Glazman]

Ok I found the root of the problem and it's NOT related to @import, it's related to "url(" !!!

The test http://www.w3.org/Style/CSS/Test/CSS3/Namespace/20090210/syntax-006.xml
contains the following stylesheet:

   @namespace x u\00072l("test");
   @import url("support/fail.css");
   @namespace url("test2");
   x|test { background:lime }
   test { background:red }

and the first line fails because of u\00072l("test") !!!
So the second line, the @import rule, is applied when it should not.

Fundamentally, this bug should have its title changed. @import and @namespace
and even url() all are implemented correctly. This bug exists because escaped
characters FAIL in most idents. As an example,

  @i\mport "foo.css";

will always fail and never import the foo.css stylesheet...

[Daniel Glazman]

Created attachment 55223 [details]
proposed fix, tested

[Cedric Vivier]

(In reply to comment #4)
> Created an attachment (id=55223) [details]
> proposed fix, tested

Shouldn't the tokenizer lazily unescape escaped characters so that it does not potentially complexify/slow down the processing of all stylesheets to handle this (very?) rare use case?

Something like:

ESCAPED_CHAR    \\(0{0,4}[0-9][0-9](\r\n|[ \t\r\n\f])?) | [a-z])
{
    //unput is a flex function to set the next character to be scanned.
    //unescape is a function to implement (or likely rather reuse from somewhere) that returns an unescaped character.

    /*insert unescaped character */
    unput(unescape(yytext)); 

    /*eat up escaped sequence*/
}

[Darin Adler]

Comment on attachment 55223 [details]
proposed fix, tested

This patch has tabs in it. Please make a new one without tabs.

How did you test the effect of this change on CSS parsing performance? What was the effect?

[Darin Adler]

(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=55223) [details] [details]
> > proposed fix, tested
> 
> Shouldn't the tokenizer lazily unescape escaped characters so that it does not
> potentially complexify/slow down the processing of all stylesheets to handle
> this (very?) rare use case?
> 
> Something like:
> 
> ESCAPED_CHAR    \\(0{0,4}[0-9][0-9](\r\n|[ \t\r\n\f])?) | [a-z])
> {
>     //unput is a flex function to set the next character to be scanned.
>     //unescape is a function to implement (or likely rather reuse from
> somewhere) that returns an unescaped character.
> 
>     /*insert unescaped character */
>     unput(unescape(yytext)); 
> 
>     /*eat up escaped sequence*/
> }

That will probably work better, yes.

[Alexey Proskuryakov]

See also: bug 32045.

[Daniel Glazman]

(In reply to comment #6)

> (From update of attachment 55223 [details])
> This patch has tabs in it. Please make a new one without tabs.
> 
> How did you test the effect of this change on CSS parsing performance? What was
> the effect?

I did not. This is my very first patch in the WebKit world and I'm not yet
familiar with the environment (10 years of mozillianism is barely an excuse;-)

I agree the proposed fix could be a wrong thing to do on a perf basis. But
the diagnosis of the bug is at least the right one. There are two issues here:

  - escaped chars in tokens
  - WebCore::CSSParser::text assuming a fixed length for functional notations

[Darin Adler]

(In reply to comment #9)
> But the diagnosis of the bug is at least the right one.

Yes, agreed. Good work on that! Lets find an efficient fix. I think Cedric Vivier’s suggestion may be the best way to approach the tokenizer half of the problem.

[Dave Hyatt]

Yeah, I think the current patch uglifies the tokenizer too much, and as a result will be minusing it.  I think what Cedric is suggesting will keep the tokenizer side much cleaner.

[Daniel Glazman]

(In reply to comment #11)

> Yeah, I think the current patch uglifies the tokenizer too much, and as a result will be minusing it.  I think what Cedric is suggesting will keep the tokenizer side much cleaner.

Aaaaah :-) I missed hyatt's r- ;-)

BTW, this bug is the last outstanding bug on the CSS 3 Namespaces radar blocking the move to PR. See

  http://disruptive-innovations.com/zoo/csswg/NAMESPACES_IMP_REPORT/

[Alexey Proskuryakov]

>   http://disruptive-innovations.com/zoo/csswg/NAMESPACES_IMP_REPORT/

What do the green "fail" boxes in Safari column mean? I can't find it explained on this page.

[Daniel Glazman]

(In reply to comment #13)
> >   http://disruptive-innovations.com/zoo/csswg/NAMESPACES_IMP_REPORT/
> 
> What do the green "fail" boxes in Safari column mean? I can't find it explained on this page.

D'oh. Thanks for the heads up, I updated the classes on the table cells
but forgot the prose. Fixed now.

[Alexey Proskuryakov]

See also: bug 54838.

[Ahmad Saleem]

Safari pass all tests in CSS Namespace - https://wpt.fyi/results/css/css-namespaces?label=master&label=experimental&aligned&q=namespace

Is anything further needed here? Thanks!

Safari even pass on Comment 03.

[Brent Fulgham]

This was probably fixed with the CSS Parser import, @Antti, can you confirm and close?

[Ryosuke Niwa]

Most certainly fixed by the new CSS parser.