Implement parser for :lang pseudo class selector arguments with wildcard '*' subtags
Created attachment 242133 [details] Patch
Created attachment 242141 [details] Patch
Created attachment 242214 [details] Patch
Comment on attachment 242214 [details] Patch I'll try to review this tomorrow, I am not doing too well. Some comments form a quick reading: -I don't think '*' is valid language range for CSS. The specification says that '*' can be the first character only when followed by an hyphen: "consist of an asterisk (* U+002A) immediately followed by an identifier beginning with an ASCII hyphen (U+002D)". -The terminals LANGRANGE and IDENT alone have no action, that is suspicious but that may be correct. -The production for the rule "lang_range" is more complex than I'd like. The more complex a production is, the harder it is to verify its correctness. I'll need to look at the parser/lexer side of things to see if lang_range can be simplified. Hopefully I'll do that tomorrow.
(In reply to comment #4) > Comment on attachment 242214 [details] > Patch > > I'll try to review this tomorrow, I am not doing too well. > Ah, get well soon :). > Some comments form a quick reading: > -I don't think '*' is valid language range for CSS. The specification says > that '*' can be the first character only when followed by an hyphen: > "consist of an asterisk (* U+002A) immediately followed by an identifier > beginning with an ASCII hyphen (U+002D)". Is valid language range for CSS != language range defined in http://www.ietf.org/rfc/rfc3066.txt (*) section 2.5? (*) In that rfc3066, the language range is denoted as: The following definition of language-range is derived from HTTP/1.1 [RFC 2616]. language-range = language-tag / "*" That is, a language-range has the same syntax as a language-tag, or is the single character "*". > -The terminals LANGRANGE and IDENT alone have no action, that is suspicious > but that may be correct. > -The production for the rule "lang_range" is more complex than I'd like. The > more complex a production is, the harder it is to verify its correctness. Yes it is, I tempted to use the `goto` statement, but I'm not sure if that path will lead us to simpler production. > > I'll need to look at the parser/lexer side of things to see if lang_range > can be simplified. Hopefully I'll do that tomorrow. Thanks!
(In reply to comment #5) > > Some comments form a quick reading: > > -I don't think '*' is valid language range for CSS. The specification says > > that '*' can be the first character only when followed by an hyphen: > > "consist of an asterisk (* U+002A) immediately followed by an identifier > > beginning with an ASCII hyphen (U+002D)". > > Is valid language range for CSS != language range defined in > http://www.ietf.org/rfc/rfc3066.txt (*) section 2.5? > > (*) In that rfc3066, the language range is denoted as: > The following definition of language-range is derived from HTTP/1.1 [RFC > 2616]. > > language-range = language-tag / "*" > > That is, a language-range has the same syntax as a language-tag, or is the > single character "*". It looks like the specification of CSS is more restrictive than the definition of range in rfc3066 (http://dev.w3.org/csswg/selectors4/#the-lang-pseudo). As far as I know, the CSS working group has given no rationale for this.
Comment on attachment 242214 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242214&action=review Looking at the parser, the problem is that you are interpreting the range as defined by BCP47. Here we are in a much more restricted context. Since we are parsing a stylesheet, we are expected to parse using the rules of CSS, then try to follow BCP 47 as closely as possible for interpreting the range. CSS Level 4 defines the valid input of :lang() as (http://dev.w3.org/csswg/selectors4/#the-lang-pseudo): "Each language range in :lang() must be a valid CSS identifier [CSS21] or consist of an asterisk (* U+002A) immediately followed by an identifier beginning with an ASCII hyphen (U+002D) for the selector to be valid.". This implies we should use all the rules of CSS Identifiers for interpreting the input. For example, with CSS, you can escape characters with \. A range with a star would be encoded in CSS as :lang(foo-\*-bar). You can even include parenthesis in the range by escaping them, something like :lang(foo-\(bar\)) probably already work today. I think CharacterAsterisk + "currentCharacter<SrcCharacterType>() == '-'" is a good start to get the new token. But then I would try to take advantage of parseIdentifier() to get the remaining characters. > Source/WebCore/css/CSSParser.cpp:11531 > + String range; Using String::append() does a new memory allocation for every call. When creating String from a series of parts, we use StringBuilder to reduce the allocation impact.
Sorry it took me so long to do the first round of review.
Created attachment 242384 [details] Patch
Comment on attachment 242384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242384&action=review You removed a bunch of tests between the last two iterations. You had many great tests in the previous iteration, it is sad to lose them. What I would do is: -Create a new test parsing-css-lang.html -Add all the tests you had previously, just change the expected results to your new parser. -Duplicate some of the tests you had previously and use escape characters in there. You can check the tests parsing-matches-X.html, parsing-not-X.html, parsing-nth-child-X.html. Those are extensive parsing tests. Appart from that, the new grammar looks much simpler, I like it. > Source/WebCore/css/CSSParser.cpp:11501 > + result++; I would do result = currentCharacter<SrcCharacterType>(); ...it seems clearer. > Source/WebCore/css/CSSParser.cpp:11502 > + parseIdentifier(result, yylval->string, hasEscape); I find it confusing to use yylval->string here, I would use a temporary variable, then yylval->string.init(parsedLangRange.toString()); later initialize the yylval. I think you will hit an assertion if you use this code with "*--".
I have a question, related to the spec and escaped '*'. For example we have: :lang(*-foo-\*), after parsing, it can be then serialized as :lang(*-foo-*), which has '*' that is not at the beginning of the language range. Should it be treated as invalid range? Since in the spec http://dev.w3.org/csswg/selectors4/#the-lang-pseudo EXAMPLE 31, there is a note: "Note that asterisks are not allowed anywhere else in :lang()'s language range syntax: they only have meaning, and are therefore only allowed, at the beginning." Thanks!
(In reply to comment #11) > I have a question, related to the spec and escaped '*'. > > For example we have: > > :lang(*-foo-\*), after parsing, it can be then serialized as :lang(*-foo-*), > which has '*' that is not at the beginning of the language range. Should it > be treated as invalid range? Since in the spec > http://dev.w3.org/csswg/selectors4/#the-lang-pseudo EXAMPLE 31, there is a > note: > > "Note that asterisks are not allowed anywhere else in :lang()'s language > range syntax: they only have meaning, and are therefore only allowed, at the > beginning." That's a very good question. That line in Example 31 is ambiguous. I think that sentence is wrong, or was referring to the raw input. In any case, I have asked the CSS Working Group to clarify: http://lists.w3.org/Archives/Public/www-style/2014Dec/0046.html
Created attachment 242479 [details] Patch
(In reply to comment #12) > (In reply to comment #11) > That's a very good question. That line in Example 31 is ambiguous. > > I think that sentence is wrong, or was referring to the raw input. In any > case, I have asked the CSS Working Group to clarify: > http://lists.w3.org/Archives/Public/www-style/2014Dec/0046.html Thanks for posting the question to the www-style group. I know we are still waiting for the clarification of http://lists.w3.org/Archives/Public/www-style/2014Dec/0046.html, but I guess with this #4 patch I'm trying to grab your idea, especially for the tests.
Created attachment 242482 [details] Patch
Created attachment 242487 [details] Patch
Comment on attachment 242487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242487&action=review I like the patch and the test. If we do not get feedback from the CSS WG tomorrow, let's move forward and review it. > LayoutTests/fast/css/css-selector-text-expected.txt:323 > +PASS parseThenSerializeRule(':lang(*-foo-\\0072 aisin) { }') is ':lang(*-foo-raisin) { }' > +PASS parseThenSerializeRule(':lang(*-foo-\\0062 \\0061 r) { }') is ':lang(*-foo-bar) { }' Interesting.
Comment on attachment 242487 [details] Patch The patch looks great. Very thorough testing, I like it. Let's land it
Comment on attachment 242487 [details] Patch Clearing flags on attachment: 242487 Committed r176902: <http://trac.webkit.org/changeset/176902>
All reviewed patches have been landed. Closing bug.
This cause 2 crashes in the following tests due to assertion failures on debug builds: fast/css/parsing-css-lang.html fast/css/css-selector-text.html See: https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK2%20%28Tests%29/builds/806 https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r176902%20(806)/results.html The tests have been skipped in r176908: <http://trac.webkit.org/r176908>. Please un-skip them when the issue has been fixed. Thanks.
Wouldn't it be more appropriate to roll out? It doesn't seem good to have these failures in the tree for any extended period of time (and it's already been more than a day).
(In reply to comment #22) > Wouldn't it be more appropriate to roll out? It doesn't seem good to have > these failures in the tree for any extended period of time (and it's already > been more than a day). I'll have a look. We can also revert without any impact, this is an experimental feature (CSS_SELECTORS_LEVEL4).
Patch for review: https://bugs.webkit.org/show_bug.cgi?id=139379
It seems that it was reopened due to failure, which were fixed in separate bug as per Comment 24 rather than rolling back this patch (at least as per discussion). If there is nothing much left here, can this be marked as "RESOLVED FIXED" or accordingly? Thanks!
<rdar://problem/97407945>