Fix silent column combinator parsing failure [chromium/src : main]
Byungwoo Lee (Gerrit)
Byungwoo Lee added 1 comment![]( "Open in Gerrit")
Hello Rune, this fixes the silent selector parsing error caused by the column combinator (||) which is not yet supported. (Found this after the CL 8061514 applied). Could you review this as well? Thank you!
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
if (stream.Peek().GetType() == kColumnToken) {Why is this handled differently from other tokens which may become combinators in the future?
Related details
- Byungwoo Lee
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Byungwoo Lee (Gerrit)
Byungwoo Lee added 1 comment![]( "Open in Gerrit")
if (stream.Peek().GetType() == kColumnToken) {Why is this handled differently from other tokens which may become combinators in the future?
I think that CSS column combinator (`||`) cannot be handled as kDelimiterToken since it has two characters. (and the single vertical bar (`|`) is already used for the namespace separator).
Would it be better to have an enclosing switch/case for `stream.Peak().GetType()` so we can collect the cases close to each other?
```cpp
switch (stream.Peek().GetType()) {
case kDelimiterToken:
switch (stream.Peek().Delimiter()) {
case '+':
stream.ConsumeIncludingWhitespace();
return CSSSelector::kDirectAdjacent;
case '~':
stream.ConsumeIncludingWhitespace();
return CSSSelector::kIndirectAdjacent;
case '>':
stream.ConsumeIncludingWhitespace();
return CSSSelector::kChild;
default:
return fallback_result;
}
case kColumnToken:
// TODO(crbug.com/461140887): Support CSS column combinator (||).
failed_parsing_ = true;
[[fallthrough]];
default:
return fallback_result;
}
```
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
But as far as Blink is currently concerned, "||" is not a valid combinator, right? What makes it different from "$$" or "++" or "~~"?
Related details
- Byungwoo Lee
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Or perhaps something that's not currently a delim: "$=" => kSuffixMatchToken.
What if "$=" becomes a combinator in the future?
Byungwoo Lee (Gerrit)
Byungwoo Lee added 1 comment![]( "Open in Gerrit")
Yes correct, `||` is not a valid combinator now.
But it is a valid CSS token. Here is how `CSSTokenizer` produces tokens when it reach `|`, `$`, and `~`:
```cpp
template <bool SkipComments>
CSSParserToken CSSTokenizer::NextToken() {
...
case '$':
if (ConsumeIfNext('=')) {
return CSSParserToken(kSuffixMatchToken);
}
return CSSParserToken(kDelimiterToken, '$');
case '|':
if (ConsumeIfNext('=')) {
return CSSParserToken(kDashMatchToken);
}
if (ConsumeIfNext('|')) {
return CSSParserToken(kColumnToken);
}
return CSSParserToken(kDelimiterToken, '|');
case '~':
if (ConsumeIfNext('=')) {
return CSSParserToken(kIncludeMatchToken);
}
return CSSParserToken(kDelimiterToken, '~');
...
```
Since `||` and `$=` each have a valid token, the selector parser can get `kColumnToken` or `kSuffixMatchToken` at the position where a combinator is expected (`CSSSelectorParser::ConsumeCombinator()`). So if `$=` becomes a combinator later, I think we can return its `RelationType` here, just as we do for `||`.
The `~~` case is different. Since there is no valid token for a double tilde, it is tokenized as two adjacent delimiter tokens of tilde value(`~`). If the first tilde is consumed as a subsequent-sibling combinator, the next should be a compound selector. So when the parser sees an additional subsequent-sibling combinator with no compound selector in between, the parsing fails.
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |