css nesting : increase support#345
Conversation
- handle nested at rules - improve parsing behaviour when a rule doesn't start with an ident
romainmenke
force-pushed
the
css-nesting-1--handle-at-rules--courteous-komodo-dragon-4379500d15
branch
from
76a2698 to
040b46e
Compare
| // IE star hacks are incompatible with CSS nesting. | ||
| // assertRuleSet('selector { display: inline-block; *display: inline; }', Rules.IEStarHack); | ||
| // assertRuleSet('selector { background: #00f; /* all browsers including Mac IE */ *background: #f00; /* IE 7 and below */ _background: #f60; /* IE 6 and below */ }', Rules.IEStarHack, Rules.IEStarHack); |
There was a problem hiding this comment.
see : #344
IE star hacks aren't compatible with CSS Nesting. * triggers rule parsing.
| let expectedSegment = segments.shift()!; | ||
| let actualSegment = actual.shift()!; | ||
|
|
||
| if (expectedSegment === '...') { | ||
| let nextExpectedSegment = segments[0]; | ||
| let nextActualSegment = actual[0]; | ||
|
|
||
| while (actual.length > 0) { | ||
| if (nextExpectedSegment === nextActualSegment) { | ||
| break; | ||
| } | ||
|
|
||
| actualSegment = actual.shift()!; | ||
| nextActualSegment = actual[0]; | ||
| } | ||
|
|
There was a problem hiding this comment.
This test helper was broken, it didn't actually test anything.
Adding any ... in a expect string would cause the previous algorithm to just skip to the end in case of issues.
This new algorithm is a bit stricter, making it easier to write reliable tests for nested and repeating structures.
|
|
||
| test('RuleSet', function () { | ||
| assertNodes(ruleset, 'selector { prop: value }', 'ruleset,...,selector,...,declaration,...,property,...,expression'); | ||
| assertNodes(ruleset, 'selector { prop; }', 'ruleset,...,selector,...,selector'); |
There was a problem hiding this comment.
This was a false positive from the old test helper.
There is only one selector in this source, but the test still passed.
| if (!this.peek(TokenType.Ident)) { | ||
| return this._parseRuleset(true); | ||
| } | ||
|
|
||
| return this._tryParseRuleset(true) || this._parseDeclaration(); |
There was a problem hiding this comment.
This still isn't fully correct for the CSS specification, but it does work ok in practice.
It is mainly important to have strict rule parsing when the current token isn't an ident. There must not a be a fallback to declaration parsing.
Doing the fallback to declaration parsing causes the wrong auto complete and diagnostics to be shown to users.
To be fully correct for the specification it would be better to do this :
- parse as a declaration
- if this passes, return the declaration
- if this fails, record the diagnostics and reset the parser
- parse as a rule
- if this passes, return the rule
- if this fails, record the diagnostics
- report all diagnostics
It's a bit chicken vs. egg when a user is starting to write something, you can't know what it will be come until they get to the end. So autocomplete and diagnostics should be given for both declarations and rules.
I think it is ok to add this later, given that it is a very large refactor for an edge case.
|
@aeschli friendly ping in case you missed these changes :) |
|
@romainmenke Oh sorry, I missed that. PR look good, thanks a lot! |
romainmenke
deleted the
css-nesting-1--handle-at-rules--courteous-komodo-dragon-4379500d15
branch
|
Thank you @aeschli and @joyceerhl for reviewing this 🙇 |
3 participants
Uh oh!
There was an error while loading. Please reload this page.