Skip to content

Implement CSS Nesting Module #94#123

Merged
phax merged 11 commits intophax:masterfrom
blutorange:issue-94
Apr 7, 2026
Merged

Implement CSS Nesting Module #94#123
phax merged 11 commits intophax:masterfrom
blutorange:issue-94

Conversation

@blutorange
Copy link
Copy Markdown
Contributor

@blutorange blutorange commented Apr 3, 2026

Implements CSS Nesting Module, closes #94

I think I've now got to the point where this should be done, if nobody can find any bugs. What do you think?

Implements:

As mentioned in this comment, I added the interface ICSSNestedRule and the class CSSNestedDeclarations to represent nested rule, analogous to the CSSOM.

I also:

  • Updated the getAsCSSString logic to account for nested rules.
    • It was kinda an issue that some getAsCSSString implementations added leading or trailing spaces / newlines. If these should be added may depend on the context, which only the container (e.g. the CSSStyleRule with the declarations and rules) knows about.
    • So now getAsCSSString does not add trailing spaces / newlines.
    • I don't think that counts as a breaking change -- it's still valid CSS, just slightly different whitespace in some cases.
  • Updated CSSVisitor to account for nested declarations and rules
  • Fixed testfiles/css30/good/issue-gc-13.css
    • The file is not actually good -- it's missing an rbrace!
    • 27cd08b
  • Updated WPT test files css3-modsel-156*.
    • & is now a valid selector member. The WPT test suite also changed the tests to use % instead of &, which is still invalid.
    • e1a2682
  • Adjusted test for different interpretation of missing rbrace in nested rule
    • See commit message for explanation
    • 2ce5343

awa-xima and others added 10 commits March 23, 2026 09:10
Implements:

* Nested style rules (https://drafts.csswg.org/css-nesting-1/#syntax)
* The nesting selector (https://drafts.csswg.org/css-nesting-1/#nest-selector)

TODO / decisions:

* Add support for nested group rules (https://drafts.csswg.org/css-nesting-1/#conditionals)
* How should the domain model be extended?
  * (a) Do we want to preserve the order of interspersed declarations and nested rules?
  * (b) Does it need to be API compatible? Or can there be minor changes?
  * (c) Do we want to preserve the distinction between style rule's "style" and "cssRules" attribute from the CSSOM (https://drafts.csswg.org/css-nesting-1/#nested-declarations)?
  * The easiest way would be to add a list of style rules, a list of media rules etc. to the "CSSStyleRule" class, which fulfills (b) but not (a) and (c).
* A couple of test failures related to parsing errors. Since rules can now be nested, edge cases how invalid CSS is handled differ slightly.
* Make sure nested rules appear in the output of the getCSSAsString method
…ng issues, update getAsCssString

Add CSSNestedDeclarations class to represent nested declarations, like the
CSSOM does. CSSStyleRule now has a list of ICSSNestedRule, which includes
various rules such as CSSStyleRule or CSSMediaRule. See
https://drafts.csswg.org/css-nesting-1/#conditionals

Update CSSNodeToDomainObject to create CSSNestedDeclarations instances where needed.

Add support for parsing nested at-rules, e.g.: .foo { @media print {} }

Fix parsing issue where the parser confused an element selector with a style
declaration, e.g. such as in the following 2 cases:
* .foo { p { color: red } }
* .foo { p: value; }
Add a lookahead that checks if a style declaration follows, and only consume it
in that case. Otherwise, proceed to look for rules and nested declarations.

Add a new parser rule for consuming a style declaration with nested elements.
Not all rules support nesting, e.g. @font-face. For these rules, we do want
to keep the previous behavior where it throws on nested rules.

Minor change to how whitespace is handled: getAsCssString only outputs itself
as a string, it does not output any leading or trailing whitespace or newlines.
Space and lines around an CSS element should be handled by the container that
contains the element, as it may depend on the context of that container.
& is now a valid selector member. The test suite has changed the tests to use %
instead of &, which is still invalid. See:

https://github.com/web-platform-tests/wpt/blob/master/css/selectors/old-tests/css3-modsel-156.xml
support for relative selectors in nested style rules, e.g.

.foo { > .bar { ... } }

When a style rule is nested, it allows a <relativeSelector>, not just a
<selector>. But at the top-level, a <relativeSelector> is not allowed, i.e.
the following is invalid:

> .bar { ... }

To prevent the grammar from blowing up, always allow <relativeSelector> in a
style rule, even at the top-level. Check for disallowed <relativeSelector> at
the top level when mapping the parsed AST to the domain model. Issue a CSS
interpretation error if such an invalid relative selector is encountered.
The file is not actually good -- it's missing an rbrace!
…rule

With the browser compliant flag enabled, the behavior did not change (empty string).

With the browser compliant flag disabled,

.class{color:red;.class{color:green}.class{color:blue}

is now parsed as nothing. Previously, an error occurred at the second ".class", causing the
parser to skip to the next rbrace. Now, the second ".class" is the start of a nested
rule. The parser only encounters an error at the very end, when it finds a missing rbrace.
The error recovery tries looking for the next rbrace, but finds none, only an EOF, causing
it to discard the entire rule.
Use prefix "s" for string variables
Use prefix "m_" for instance fields

As per https://github.com/phax/meta/blob/master/CodingStyleguide.md
Copy link
Copy Markdown
Owner

@phax phax left a comment

Choose a reason for hiding this comment

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

Please find attached my review comments.
I tried to provide clear guidance to the AI what to do - hope it's not a big thing - all minor things

* </ul>
* </ul>
* </ul>
* @author Philip Helger
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Obviously not ;-)

* {@link com.helger.css.decl.visit.DefaultCSSVisitor DefaultCSSVisitor} which is a good basis for
* your own implementations.
*
* @author Philip Helger
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Wasn't me :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Quick question, do you care about the @author being present? Personally I think the relevant info is in the commit history already.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@phax Is see you already merged this, great! But note I didn't change the @author yet. You can either remove the annotation or change it to my name Andre Wachsmuth, both is fine with me ; )

*
* @param <IMPLTYPE>
* Implementation type
* @author Philip Helger
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Wasn't me

@phax
Copy link
Copy Markdown
Owner

phax commented Apr 6, 2026

Thanks alot for your contribution - like it :-)

Can you please do me a favour and explain the reasoning for modifying the new-line handling? As the tests are not really changing there is not much difference in the output - or was that just some AI "noise" on top of it?

@blutorange
Copy link
Copy Markdown
Contributor Author

blutorange commented Apr 7, 2026

As the tests are not really changing there is not much difference in the output

Yes, I want to change as little behavior as possible.

Can you please do me a favour and explain the reasoning for modifying the new-line handling?

I already mentioned it in the opening message, but I'll try again. I found it quite hard to reason about how to generate proper(ly indented) code without clearly defining what each call of getAsCSSString is responsible for in terms of whitespace and indentation.

What I find easy to work with is: getAsCSSString only writes itself, and not any whitespace and indentation before or after.

Consider e.g. CSSStyleRule. It contains (a) declarations and (b) nested rules.

Up until now, the implementation of CSSDeclarationContainer#getAsCSSString ends at the last }, without a newline. But e.g. CSSFontFaceRule#getAsCSSString writes a newline at the end. (inconsistent)

And a CSSNestedDeclarations can't know whether it should add a newline or not. Only the CSSStyleRule that contains the CSSNestedDeclarations knows if a newline should be added. E.g.:

.foo {
  .bar {} /* Newline required because this is not the last nested rule */

  color: blue;
}
css
.foo { .bar {} } /* now newline because ".bar" this is the last nested rule */

(Yes, we can debate how we want to format .foo { .bar {} }. But the point still stands that the container knows better what whitespace / indentation to add then the individual children).

If you really want, I can try and make it work with the previous behavior, but imo it makes more sense this way.

or was that just some AI "noise" on top of it?

Slightly off-topic, but in my experience, AI can be useful as a better autocomplete, but not for making decision on how to implement something. Everything in this PR is (my) intention ; )

Still recovering from sickness and trying to catch up. Please note that I want to wait for !123 to be sorted out before doing this one - I hope you understand.

I hope you get well soon. It's not urgent for us either, we're still in the process of migration everything to Java 17+. But I'm looking forward when we can use the improved parser : )

@phax
Copy link
Copy Markdown
Owner

phax commented Apr 7, 2026

@blutorange Thanks for the second explanation - my brain was still a bit foggy when processing this the first time.
Makes total sense to me and I will try to inherit this for future endeavours :-)

@phax phax merged commit 0fcff58 into phax:master Apr 7, 2026
1 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for CSS nesting module

3 participants