[CLEANUP] Clean extra whitespace in css selector #1398

8ctopus · 2025-11-28T13:05:07Z

This PR cleans the css selector from extra whitespace:

$input = <<<CSS
p    >
    small {
font-size:.7em
}

CSS;

$document = (new \Sabberworm\CSS\Parser($input))
    ->parse();

echo $document->render(\Sabberworm\CSS\OutputFormat::createPretty());

without the PR

p    >
    small {
        font-size: .7em;
}

with the PR:

p > small {
        font-size: .7em;
}

coveralls · 2025-11-28T13:08:58Z

coverage: 62.745% (+0.04%) from 62.706%
when pulling 22bee0f on 8ctopus:cleanup-selector
into e7dcce8 on MyIntervals:main.

JakeQZ

Thanks. It's a nice idea to tidy up the whitespace in the selectors.

However, it's not as simple as it seems. We would also need to avoid touching the whitespace within attribute selectors. Although an unlikely real-world example, this change would break a selector like a[title="extra space"].

So I think this would require a more complex regex that handles quoted strings (single and double quotes) to avoid touching them. I'm not sure how a[title=extra space] is supposed to be interpreted; that would need checking up on.

If you're willing to give this a go, we would welcome the tidying functionality, with some tests added to SelectorTest to make sure it reformats only the whitespace it should. I also understand if this now seems like too big a job - instead we could convert this PR to a 'would be nice to have' feature request.

JakeQZ · 2025-11-29T01:12:10Z

If you're willing to give this a go

DeclarationBlock::parse() may be the place to look for removing whitespace, though I'm not sure how.

I also note that setSelectors would fail for a[title="this, and that"], though the parser would process it fine.

@8ctopus, whether or not you proceed with this PR, I thank you for helping us identify some gremlins :/

8ctopus · 2025-12-02T13:27:32Z

@JakeQZ Give me a few days, I'll see what I can come up with as a solution.

JakeQZ · 2025-12-02T19:27:40Z

@JakeQZ Give me a few days, I'll see what I can come up with as a solution.

Thanks. There are some comments in #1324 that may be useful.

8ctopus · 2025-12-05T14:31:41Z

I've come up with a solution for you to look at when you have time. Unfortunately, not elegant, but all tests do pass. I couldn't figure out how to create a regex that only takes two or more spaces that are not within an attribute selector (negative look behind only supports the characters immediately beforehand, therefore not possible to use it).

oliverklee · 2025-12-05T16:51:44Z

tests/Unit/Property/SelectorTest.php

+     */
+    public function cleanupSpacesWithinSelector(): void
+    {
+        $selector = <<<'CSS'


Please don't use heredoc - indented heredoc doesn't work with with PHP 7.2 (though it works with 7.3 upwards), and it makes autoformatting the code more of a hassle.

Instead, could you please switch to a literal, quotes-enclosed string?

Thank you for bringing this to my attention, it's fixed.

JakeQZ

I've come up with a solution for you to look at when you have time. Unfortunately, not elegant, but all tests do pass. I couldn't figure out how to create a regex that only takes two or more spaces that are not within an attribute selector (negative look behind only supports the characters immediately beforehand, therefore not possible to use it).

Yeah, completely solving this with regex may not be straightfoward, nor the long-term way forward.

We have a couple of open issues in this area: #1324, #1325.

What you've done goes some of the way, and the tests you've added will help ensure tightness when we refactor the code.

I've suggested also replacing single 'space' characters (such as newlines or tabs) with actual spaces. Would you be willing to create tests for those cases? And resolve the issues raised by @oliverklee?

Thanks for taking the time to contribute; much appreciated <3.

JakeQZ

I failed to save the inline code comment. Here it is.

JakeQZ · 2025-12-05T23:31:54Z

src/Property/Selector.php

-        $this->selector = \trim($selector);
+        $selector = \trim($selector);
+
+        $this->selector = !str_contains($selector, '[') ? preg_replace('/\s{2,}/', ' ', $selector) : $selector;


The \s needs double-escaping (it does technically work single-escaped, but fails a coding style check).

We could also convert any whitespace to an actual space - e.g. a single newline, tab, etc. - and have a test for that too.

And for better performance (and to avoid catastrophic backtracking in the unlikely case of a selector string with a million spaces being passed), we can use the possessive quantifier (+).

Suggested change

$this->selector = !str_contains($selector, '[') ? preg_replace('/\s{2,}/', ' ', $selector) : $selector;

$this->selector = !str_contains($selector, '[') ? preg_replace('/\\s++/', ' ', $selector) : $selector;

JakeQZ

I've suggested a couple of improvements to the test method names, so that they more accurately describe what the class does (or is expected to do).

JakeQZ · 2025-12-05T23:38:13Z

tests/Unit/Property/SelectorTest.php

+    /**
+     * @test
+     */
+    public function cleanupSpacesWithinSelector(): void


Suggested change

public function cleanupSpacesWithinSelector(): void

public function cleansUpSpacesWithinSelector(): void

JakeQZ · 2025-12-05T23:38:40Z

tests/Unit/Property/SelectorTest.php

+    /**
+     * @test
+     */
+    public function doNotCleanupSpacesWithinAttributeSelector(): void


Suggested change

public function doNotCleanupSpacesWithinAttributeSelector(): void

public function doesNotCleanupSpacesWithinAttributeSelector(): void

8ctopus · 2025-12-06T02:40:54Z

Thank you again for the valuable input, I've made all changes requested, please review when you have the time.

JakeQZ

Thank you again for the valuable input, I've made all changes requested, please review when you have the time.

Thanks. This is looking good now. I like the tests.

There are a couple of further issues to tidy/fix (see specific comments).

Also, could you add an entry to the changelog, probably in the 'Changed' section. (New entries should go at the top of the list.)

JakeQZ · 2025-12-06T06:49:09Z

src/Property/Selector.php

-        $this->selector = \trim($selector);
+        $selector = \trim($selector);
+
+        $this->selector = !str_contains($selector, '[') ? preg_replace('/\\s++/', ' ', $selector) : $selector;


str_contains requires PHP 8, whereas the library supports PHP from 7.2, so I think we'll need to use strpos instead (for now). Also, we're using \ prefix when calling a global function (for clarity).

EDIT: I just realized I originally got the logic the wrong way round in the suggestion. So I think it would be good to use a local variable to make it clear what's happening. Thinking more, a comment explaining why whitespace isn't removed when there's an attribute selector wouldn't go amiss.

Suggested change

$this->selector = !str_contains($selector, '[') ? preg_replace('/\\s++/', ' ', $selector) : $selector;

// Whitespace can't be adjusted within an attribute selector, as it would change its meaning

$hasAttribute = \strpos($selector, '[') !== false;

$this->selector = $hasAttribute ? $selector : preg_replace('/\\s++/', ' ', $selector);

JakeQZ · 2025-12-06T07:00:40Z

tests/Unit/Property/SelectorTest.php

+        $selector = "p
+>
+small";


To improve readability of the code, it would be better to use \n in the string rather than an actual newline (so that we don't have contents of the string indented less than the main code).

Suggested change

$selector = "p

>

small";

$selector = "p\n>\nsmall";

8ctopus · 2025-12-08T12:07:06Z

All changes done, thank you for the feedback.

oliverklee · 2025-12-08T12:21:07Z

@8ctopus Thanks! Now there's a merge conflict in the changelog file. Could you please rebase on main (rebase, please don't merge main into this branch), resolve the merge conflict and repush? Thanks!

8ctopus · 2025-12-08T13:38:43Z

done

JakeQZ

This LGTM now. Thanks for contributing <3.

oliverklee requested a review from JakeQZ November 28, 2025 13:07

oliverklee added this to the 9.2.0: New features, bug fixes and deprecations milestone Nov 28, 2025

oliverklee added the bug label Nov 28, 2025

JakeQZ requested changes Nov 28, 2025

View reviewed changes

JakeQZ added waiting for contributor enhancement and removed bug labels Nov 28, 2025

JakeQZ mentioned this pull request Nov 29, 2025

Refactor DeclarationBlock::parse() #1324

Open

oliverklee reviewed Dec 5, 2025

View reviewed changes

JakeQZ requested changes Dec 5, 2025

View reviewed changes

JakeQZ requested changes Dec 6, 2025

View reviewed changes

8ctopus added 9 commits December 8, 2025 17:36

[CLEANUP] Clean extra whitespace in css selector

6e0fe87

[TASK] Add cleanup selector tests

020d7b5

[BUGFIX] Do not cleanup whitespace within attribute selectors

c491e5d

[TASK] Apply code review selector changes

89abb08

[TASK] Refactor selector spacing tests

a9f1d73

[TASK] Add changelog entry

defa82d

[TASK] Code must be compatible with php 7.2

47cc99c

[TASK] Add code comment

658998b

[TASK] Cleanup test code formatting

22bee0f

8ctopus force-pushed the cleanup-selector branch from b508d8a to 22bee0f Compare December 8, 2025 13:38

JakeQZ approved these changes Dec 8, 2025

View reviewed changes

JakeQZ merged commit 49bf66f into MyIntervals:main Dec 8, 2025
23 checks passed

	$this->selector = !str_contains($selector, '[') ? preg_replace('/\s{2,}/', ' ', $selector) : $selector;
	$this->selector = !str_contains($selector, '[') ? preg_replace('/\\s++/', ' ', $selector) : $selector;

	public function cleanupSpacesWithinSelector(): void
	public function cleansUpSpacesWithinSelector(): void

	public function doNotCleanupSpacesWithinAttributeSelector(): void
	public function doesNotCleanupSpacesWithinAttributeSelector(): void

-        $this->selector = !str_contains($selector, '[') ? preg_replace('/\\s++/', ' ', $selector) : $selector;
+        // Whitespace can't be adjusted within an attribute selector, as it would change its meaning
+        $hasAttribute = \strpos($selector, '[') !== false;
+        $this->selector = $hasAttribute ? $selector : preg_replace('/\\s++/', ' ', $selector);

[CLEANUP] Clean extra whitespace in css selector #1398

[CLEANUP] Clean extra whitespace in css selector #1398

Conversation

8ctopus commented Nov 28, 2025

Uh oh!

coveralls commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JakeQZ left a comment

Choose a reason for hiding this comment

Uh oh!

JakeQZ commented Nov 29, 2025

Uh oh!

8ctopus commented Dec 2, 2025

Uh oh!

JakeQZ commented Dec 2, 2025

Uh oh!

8ctopus commented Dec 5, 2025

Uh oh!

oliverklee Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

oliverklee Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

8ctopus Dec 6, 2025

Choose a reason for hiding this comment

Uh oh!

JakeQZ left a comment

Choose a reason for hiding this comment

Uh oh!

JakeQZ left a comment

Choose a reason for hiding this comment

Uh oh!

JakeQZ Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

JakeQZ left a comment

Choose a reason for hiding this comment

Uh oh!

JakeQZ Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

JakeQZ Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

8ctopus commented Dec 6, 2025

Uh oh!

JakeQZ left a comment

Choose a reason for hiding this comment

Uh oh!

JakeQZ Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakeQZ Dec 6, 2025

Choose a reason for hiding this comment

Uh oh!

8ctopus commented Dec 8, 2025

Uh oh!

oliverklee commented Dec 8, 2025

Uh oh!

8ctopus commented Dec 8, 2025

Uh oh!

JakeQZ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Nov 28, 2025 •

edited

Loading

JakeQZ Dec 6, 2025 •

edited

Loading