Conversation
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
phax
left a comment
There was a problem hiding this comment.
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 |
ph-css/src/main/java/com/helger/css/decl/CSSNestedDeclarations.java
Outdated
Show resolved
Hide resolved
| * {@link com.helger.css.decl.visit.DefaultCSSVisitor DefaultCSSVisitor} which is a good basis for | ||
| * your own implementations. | ||
| * | ||
| * @author Philip Helger |
There was a problem hiding this comment.
Quick question, do you care about the @author being present? Personally I think the relevant info is in the commit history already.
There was a problem hiding this comment.
@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 |
ph-css/src/main/java/com/helger/css/handler/CSSNodeToDomainObject.java
Outdated
Show resolved
Hide resolved
|
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? |
Yes, I want to change as little behavior as possible.
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 What I find easy to work with is: Consider e.g. Up until now, the implementation of And a .foo {
.bar {} /* Newline required because this is not the last nested rule */
color: blue;
}(Yes, we can debate how we want to format If you really want, I can try and make it work with the previous behavior, but imo it makes more sense this way.
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 ; )
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 : ) |
|
@blutorange Thanks for the second explanation - my brain was still a bit foggy when processing this the first time. |
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:
.foo { .bar { } }) (https://drafts.csswg.org/css-nesting-1/#syntax).foo { @media print { }) (https://drafts.csswg.org/css-nesting-1/#conditionals).foo { &.bar { ... } }) (https://drafts.csswg.org/css-nesting-1/#nest-selector).foo { > .bar { ... } }) (https://drafts.csswg.org/css-nesting-1/#syntax)As mentioned in this comment, I added the interface
ICSSNestedRuleand the classCSSNestedDeclarationsto represent nested rule, analogous to the CSSOM.I also:
getAsCSSStringlogic to account for nested rules.getAsCSSStringimplementations added leading or trailing spaces / newlines. If these should be added may depend on the context, which only the container (e.g. theCSSStyleRulewith the declarations and rules) knows about.getAsCSSStringdoes not add trailing spaces / newlines.CSSVisitorto account for nested declarations and rulestestfiles/css30/good/issue-gc-13.csscss3-modsel-156*.&is now a valid selector member. The WPT test suite also changed the tests to use%instead of &, which is still invalid.