Skip to content

[BUGFIX] Render rules in line and column number order #1058

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[BUGFIX] Render rules in line and column number order #1058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ Please also have a look at our

### Fixed

- Render rules in line and column number order (#1058)
- Don't render `rgb` colors with percentage values using hex notation (#803)
- Parse `@font-face` `src` property as comma-delimited list (#790)

Expand Down
28 changes: 13 additions & 15 deletions src/RuleSet/RuleSet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,22 +277,20 @@ protected function renderRules(OutputFormat $outputFormat)
$result = '';
$isFirst = true;
$nextLevelFormat = $outputFormat->nextLevel();
foreach ($this->rules as $rules) {
foreach ($rules as $rule) {
$renderedRule = $nextLevelFormat->safely(static function () use ($rule, $nextLevelFormat): string {
return $rule->render($nextLevelFormat);
});
if ($renderedRule === null) {
continue;
}
if ($isFirst) {
$isFirst = false;
$result .= $nextLevelFormat->spaceBeforeRules();
} else {
$result .= $nextLevelFormat->spaceBetweenRules();
}
$result .= $renderedRule;
foreach ($this->getRules() as $rule) {
$renderedRule = $nextLevelFormat->safely(static function () use ($rule, $nextLevelFormat): string {
return $rule->render($nextLevelFormat);
});
if ($renderedRule === null) {
continue;
}
if ($isFirst) {
$isFirst = false;
$result .= $nextLevelFormat->spaceBeforeRules();
} else {
$result .= $nextLevelFormat->spaceBetweenRules();
}
$result .= $renderedRule;
}

if (!$isFirst) {
Expand Down
35 changes: 35 additions & 0 deletions tests/Unit/RuleSet/DeclarationBlockTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

declare(strict_types=1);

namespace Sabberworm\CSS\Tests\Unit\RuleSet;

use PHPUnit\Framework\TestCase;
use Sabberworm\CSS\Parsing\ParserState;
use Sabberworm\CSS\RuleSet\DeclarationBlock;
use Sabberworm\CSS\OutputFormat;
use Sabberworm\CSS\Settings;

/**
* @covers \Sabberworm\CSS\RuleSet\DeclarationBlock
*/
final class DeclarationBlockTest extends TestCase
{
/**
* @test
*/
public function rendersRulesInOrderProvided(): void
{
$css = '
.test {
background-color:transparent;
background:#222;
background-color:#fff;
}';
$expectedRendering = 'background-color: transparent;background: #222;background-color: #fff;';

$declarationBlock = DeclarationBlock::parse(new ParserState($css, Settings::create()));

self::assertStringContainsString($expectedRendering, $declarationBlock->render(new OutputFormat()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please let's not use render in unit tests (I'll try to do this for the existing code in #1057). Instead, please let's check for what getSelectors returns.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(That's because we want to check the parsing here, not the rendering, and particularly not both at the same time.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

When we mix parsing and rendering in one test, it's really hard to see what the test is about when reading it. And when the test fails, it's near impossible to find out if the parsing of the rendering is broken.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We actually want to check the rendering here (that was what was broken).

To set up the construct whose rendering is to be tested without using the parser would require quite of lot of code. It would need to use the various constructors, setters, adders, etc. to construct this:

Sabberworm\CSS\RuleSet\DeclarationBlock::__set_state(array(
   'selectors' => 
  array (
    0 => 
    Sabberworm\CSS\Property\Selector::__set_state(array(
       'selector' => '.test',
    )),
  ),
   'rules' => 
  array (
    'background-color' => 
    array (
      0 => 
      Sabberworm\CSS\Rule\Rule::__set_state(array(
         'rule' => 'background-color',
         'value' => 'transparent',
         'isImportant' => false,
         'lineNumber' => 3,
         'columnNumber' => 94,
         'comments' => 
        array (
        ),
      )),
      1 => 
      Sabberworm\CSS\Rule\Rule::__set_state(array(
         'rule' => 'background-color',
         'value' => 
        Sabberworm\CSS\Value\Color::__set_state(array(
           'sName' => 'rgb',
           'aComponents' => 
          array (
            'r' => 
            Sabberworm\CSS\Value\Size::__set_state(array(
               'fSize' => 255.0,
               'sUnit' => NULL,
               'bIsColorComponent' => true,
               'lineNumber' => 5,
            )),
            'g' => 
            Sabberworm\CSS\Value\Size::__set_state(array(
               'fSize' => 255.0,
               'sUnit' => NULL,
               'bIsColorComponent' => true,
               'lineNumber' => 5,
            )),
            'b' => 
            Sabberworm\CSS\Value\Size::__set_state(array(
               'fSize' => 255.0,
               'sUnit' => NULL,
               'bIsColorComponent' => true,
               'lineNumber' => 5,
            )),
          ),
           'sSeparator' => ',',
           'lineNumber' => 5,
        )),
         'isImportant' => false,
         'lineNumber' => 5,
         'columnNumber' => 176,
         'comments' => 
        array (
        ),
      )),
    ),
    'background' => 
    array (
      0 => 
      Sabberworm\CSS\Rule\Rule::__set_state(array(
         'rule' => 'background',
         'value' => 
        Sabberworm\CSS\Value\Color::__set_state(array(
           'sName' => 'rgb',
           'aComponents' => 
          array (
            'r' => 
            Sabberworm\CSS\Value\Size::__set_state(array(
               'fSize' => 34.0,
               'sUnit' => NULL,
               'bIsColorComponent' => true,
               'lineNumber' => 4,
            )),
            'g' => 
            Sabberworm\CSS\Value\Size::__set_state(array(
               'fSize' => 34.0,
               'sUnit' => NULL,
               'bIsColorComponent' => true,
               'lineNumber' => 4,
            )),
            'b' => 
            Sabberworm\CSS\Value\Size::__set_state(array(
               'fSize' => 34.0,
               'sUnit' => NULL,
               'bIsColorComponent' => true,
               'lineNumber' => 4,
            )),
          ),
           'sSeparator' => ',',
           'lineNumber' => 4,
        )),
         'isImportant' => false,
         'lineNumber' => 4,
         'columnNumber' => 134,
         'comments' => 
        array (
        ),
      )),
    ),
  ),
   'lineNumber' => 1,
   'comments' => 
  array (
  ),
))

If a test has been broken by a change to the parsing, then that's likely to be the culprit. Ditto for the rendering. I agree if it's an update to a library (or PHP) it's more difficult.

On the parsing side, we can have tests that confirm specific things have been parsed, and a test for DeclarationBlock parsing doesn't need to check beyond the Selector and Rules (since we would already have tests for Rule parsing the property name and Value).

So maybe a more viable approach would be to use the parser for the rendering tests, but also have corresponding parsing tests that don't use the renderer. That way we'd also be able to tell which side was broken.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the rendering test, I've given it a go at #1059. What do you think?

}
}