Skip to content

[BUGFIX] Allow comma-separated arguments in selectors #1292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ Please also have a look at our

### Fixed

- Selector functions (like `:not`) with comma-separated arguments are now
parsed correctly (#1292)
- Allow comma in selectors (e.g. `:not(html, body)`) (#1293)
- Insert `Rule` before sibling even with different property name
(in `RuleSet::addRule()`) (#1270)
Expand Down
39 changes: 36 additions & 3 deletions src/RuleSet/DeclarationBlock.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,23 @@ public static function parse(ParserState $parserState, ?CSSList $list = null): ?
$comments = [];
$result = new DeclarationBlock($parserState->currentLine());
try {
$selectors = [];
$selectorParts = [];
$stringWrapperCharacter = null;
$functionNestingLevel = 0;
$consumedNextCharacter = false;
do {
$selectorParts[] = $parserState->consume(1)
. $parserState->consumeUntil(['{', '}', '\'', '"'], false, false, $comments);
if (!$consumedNextCharacter) {
$selectorParts[] = $parserState->consume(1);
}
$selectorParts[] = $parserState->consumeUntil(
['{', '}', '\'', '"', '(', ')', ','],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this easier to understand, I propose moving this array either to a class constant or a well-named local variable. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a local variable, as it's only applicable to this method. It also matches all the cases in the switch statement. ({ and } could be handled as switch cases rather than by the while, but it's clearer that these are loop end cases the way it's currently written.)

false,
false,
$comments
);
$nextCharacter = $parserState->peek();
$consumedNextCharacter = false;
switch ($nextCharacter) {
case '\'':
// The fallthrough is intentional.
Expand All @@ -58,9 +69,31 @@ public static function parse(ParserState $parserState, ?CSSList $list = null): ?
}
}
break;
case '(':
if (!isset($stringWrapperCharacter)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can convert all those to is_string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I forgot to update this after making the change in the pre-PR.

++$functionNestingLevel;
}
break;
case ')':
if (!isset($stringWrapperCharacter)) {
if ($functionNestingLevel <= 0) {
throw new UnexpectedTokenException('anything but', ')');
}
--$functionNestingLevel;
}
break;
case ',':
if (!isset($stringWrapperCharacter) && $functionNestingLevel === 0) {
$selectors[] = \implode('', $selectorParts);
$selectorParts = [];
$parserState->consume(1);
$consumedNextCharacter = true;
}
break;
}
} while (!\in_array($nextCharacter, ['{', '}'], true) || \is_string($stringWrapperCharacter));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isset($stringWrapperCharacter) and \is_string($stringWrapperCharacter) are both checking the same thing, right? If so, maybe settle on using one of the two.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was changed to is_string in a pre-PR, but I forgot to update it in this.

$result->setSelectors(\implode('', $selectorParts), $list);
$selectors[] = \implode('', $selectorParts); // add final or only selector
$result->setSelectors($selectors, $list);
if ($parserState->comes('{')) {
$parserState->consume(1);
}
Expand Down
68 changes: 68 additions & 0 deletions tests/Unit/RuleSet/DeclarationBlockTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@
use Sabberworm\CSS\CSSElement;
use Sabberworm\CSS\CSSList\CSSListItem;
use Sabberworm\CSS\Position\Positionable;
use Sabberworm\CSS\Parsing\ParserState;
use Sabberworm\CSS\Property\Selector;
use Sabberworm\CSS\RuleSet\DeclarationBlock;
use Sabberworm\CSS\Settings;
use TRegx\PhpUnit\DataProviders\DataProvider;

/**
* @covers \Sabberworm\CSS\RuleSet\DeclarationBlock
Expand Down Expand Up @@ -48,4 +52,68 @@ public function implementsPositionable(): void
{
self::assertInstanceOf(Positionable::class, $this->subject);
}

/**
* @return array<non-empty-string, array{0: non-empty-string}>
*/
public static function provideSelector(): array
{
return [
'type' => ['body'],
'class' => ['.teapot'],
'id' => ['#my-mug'],
'`not`' => [':not(#your-mug)'],
'`not` with multiple arguments' => [':not(#your-mug, .their-mug)'],
];
}

/**
* @test
*
* @param non-empty-string $selector
*
* @dataProvider provideSelector
*/
public function parsesAndReturnsSingleSelector(string $selector): void
{
$subject = DeclarationBlock::parse(new ParserState($selector . '{}', Settings::create()));

$resultSelectorStrings = \array_map(
static function (Selector $selectorObject): string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have this two times now - so I'd like to propose we move this to a well-named private method.

return $selectorObject->getSelector();
},
$subject->getSelectors()
);
self::assertSame([$selector], $resultSelectorStrings);
}

/**
* @return DataProvider<non-empty-string, array{0: non-empty-string, 1: non-empty-string}>
*/
public static function provideTwoSelectors(): DataProvider
{
return DataProvider::cross(self::provideSelector(), self::provideSelector());
}

/**
* @test
*
* @param non-empty-string $firstSelector
* @param non-empty-string $secondSelector
*
* @dataProvider provideTwoSelectors
*/
public function parsesAndReturnsTwoCommaSeparatedSelectors(string $firstSelector, string $secondSelector): void
{
$joinedSelectors = $firstSelector . ',' . $secondSelector;
$subject = DeclarationBlock::parse(new ParserState($joinedSelectors . '{}', Settings::create()));

$resultSelectorStrings = \array_map(
static function (Selector $selectorObject): string {
return $selectorObject->getSelector();
},
$subject->getSelectors()
);
self::assertSame([$firstSelector, $secondSelector], $resultSelectorStrings);
}
}