Add support for modern CSS at-rules @layer, @scope, and @starting-style#1549
Conversation
|
@oliverklee it seems like there's an issue accessing coveralls.io |
There was a problem hiding this comment.
This looks excellent, thanks <3
The tests are fairly comprehensive, but I'd like a few more to confirm what does and doesn't work now.
I think the tests for @scope parsing would be better handled by using a data provider for the argument, so that we can throw whatever we like at it (I suggest the various examples on the MDN documentation page). If some constructs don't work, they can be commented out with a comment in the data provider array indicating that a further fix is needed.
It would also be good to know if @layer statement rules are now also parsed and rendered correctly following this change (as well as @layer block rules, which are covered in the tests added).
It might be helpful to split this PR into three manageable chunks for each at-rule. But if it's a hassle to do that, it's nae bother.
If you rebase, the Coveralls build failure will vanish, following #1551.
| /** | ||
| * @test | ||
| */ | ||
| public function parsesScopeWithSelector(): void | ||
| { | ||
| $css = '@scope (.card) { .title { font-size: 2rem; } }'; | ||
| $contents = (new Parser($css))->parse()->getContents(); | ||
| $atRuleBlockList = $contents[0]; | ||
|
|
||
| self::assertInstanceOf(AtRuleBlockList::class, $atRuleBlockList); | ||
| self::assertSame('scope', $atRuleBlockList->atRuleName()); | ||
| self::assertSame('(.card)', $atRuleBlockList->atRuleArgs()); | ||
|
|
||
| $nestedContents = $atRuleBlockList->getContents(); | ||
| self::assertCount(1, $nestedContents, 'Scope should contain one declaration block'); | ||
| } |
There was a problem hiding this comment.
Does parsing with a scope limit also work following this change? And parsing with a selector list? If so, we should perhaps add tests to confirm this.
We could use a data provider to provide various @scope arguments, perhaps lifted from the MDN examples, so we'd only need one @test method, which would insert the argument into $css, and check it in assertSame( ... atRuleArgs()). This may also work for @scope without an argument.
| /** | ||
| * @test | ||
| */ | ||
| public function parsesLayerWithNamedArgument(): void | ||
| { | ||
| $css = '@layer theme { .button { color: blue; } }'; | ||
| $contents = (new Parser($css))->parse()->getContents(); | ||
| $atRuleBlockList = $contents[0]; | ||
|
|
||
| self::assertInstanceOf(AtRuleBlockList::class, $atRuleBlockList); | ||
| self::assertSame('layer', $atRuleBlockList->atRuleName()); | ||
| self::assertSame('theme', $atRuleBlockList->atRuleArgs()); | ||
|
|
||
| $nestedContents = $atRuleBlockList->getContents(); | ||
| self::assertCount(1, $nestedContents, 'Layer should contain one declaration block'); | ||
| } |
There was a problem hiding this comment.
Does parsing a statement @layer rule like
@layer theme, layout, utilities;also work following this change?
If it does, we should add tests to confirm. If not, that will be for a future PR.
There was a problem hiding this comment.
Nope, it doesn't work currently :(
The parser discards @layer theme, layout, utilities; and returns an empty contents array and renders nothing.
There was a problem hiding this comment.
Nope, it doesn't work currently :(
I feared that would be the case. There are statement at-rules, and block at-rules, and @layer can be either. Some congnitive effort and refactoring will likely be required to support both flavours of @layer (as a future PR).
| /** | ||
| * Since there are more set rules than block rules, | ||
| * we’re whitelisting the block rules and have anything else be treated as a set rule. | ||
| * NOTE: The new line when assigning the value is added to avoid the line length limit of codesniffer. |
There was a problem hiding this comment.
This comment belongs in the PR discussion or commit message, not the code.
| * NOTE: The new line when assigning the value is added to avoid the line length limit of codesniffer. |
| * @internal since 8.5.2 | ||
| */ | ||
| public const BLOCK_RULES = 'media/document/supports/region-style/font-feature-values/container'; | ||
| public const BLOCK_RULES = |
There was a problem hiding this comment.
We need to change this to an array, but that's for another PR.
There was a problem hiding this comment.
This seems like a quick win. Are you sure you want it in a separate PR?
There was a problem hiding this comment.
This seems like a quick win. Are you sure you want it in a separate PR?
Having PRs only addressing one thing makes it easier to backport them or temporarily undo them if a problem is found. So, yes.
Use data providers for AtRules parsing and rendering
JakeQZ
left a comment
JakeQZ
left a comment
There was a problem hiding this comment.
The refactored tests look good. Can we add one for @scope with a 'limit' - or does that not work yet?
Some of the parameters can be declared as non-empty-string but I'm happy to do that as a follow-up PR.
The superfluous DocBlock comment still needs taking out.
| 'scope with selector' => [ | ||
| '@scope (.card) { .title { font-size: 2rem; } }', | ||
| 'scope', | ||
| '(.card)', | ||
| 1, | ||
| ], |
There was a problem hiding this comment.
Would it be possible to have a test for scope with a limt, e.g. @scope (.article-body) to (figure)? Or does that not currently parse correctly?
| /** | ||
| * Since there are more set rules than block rules, | ||
| * we’re whitelisting the block rules and have anything else be treated as a set rule. | ||
| * NOTE: The new line when assigning the value is added to avoid the line length limit of codesniffer. |
There was a problem hiding this comment.
This still needs to be removed.
3 participants
No description provided.