Feature/add property rule support

[shagkur]

No description provided.

[blutorange]

I opened #124 before I saw that this exists. I think you're implementation is better 👍 , so #124 should be closed in favor of this.

Small note:

• getAllPropertyRules etc. should be added to AbtractHasTopLevelRules
• I have Implement CSS Nesting Module #94 #123 which implements CSS nesting. It adds "styleDeclarationOrRuleWithNested" to the grammar. The new propertyRule() { errorUnexpectedRule(...) } needs to be added there too, if the PR gets accepted

[blutorange]

There's one issue that relates to #69, which isn't special to this PR, but gets worse as more keywords are added.

Previously, CSS such as

syntax.foo { color: red; }

gets correctly parsed as an element+class selector with a declaration list. Now that syntax is a keyword, parsing fails with

Failed to parse CSS: [1:1] Encountered text 'syntax' corresponding to token "syntax". Was expecting one of: <EOF>, <S>, ...

(same for e.g. .inherits { color: red; })

Again, this issue already exists with other keywords, but I wanted to bring attention to this.

This could be an argument for not adding additional keywords; and parsing the contents of the rule as a general declaration list (like #124). And handling everything else when converting to the parsed AST to the domain model (CSSNodeToDomainObject). Though the "real" solution should be (in my opinion) to get the parser to tokenize differently depending on the context.

[shagkur]

I opened #124 before I saw that this exists. I think you're implementation is better 👍 , so #124 should be closed in favor of this.

Small note:

• getAllPropertyRules etc. should be added to AbtractHasTopLevelRules
• I have Implement CSS Nesting Module #94 #123 which implements CSS nesting. It adds "styleDeclarationOrRuleWithNested" to the grammar. The new propertyRule() { errorUnexpectedRule(...) } needs to be added there too, if the PR gets accepted

The @property rule must not be nested. It must appear at the top level of a stylesheet.

[shagkur]

There's one issue that relates to #69, which isn't special to this PR, but gets worse as more keywords are added.

Previously, CSS such as

syntax.foo { color: red; }

gets correctly parsed as an element+class selector with a declaration list. Now that syntax is a keyword, parsing fails with

Failed to parse CSS: [1:1] Encountered text 'syntax' corresponding to token "syntax". Was expecting one of: <EOF>, <S>, ...

(same for e.g. .inherits { color: red; })

Again, this issue already exists with other keywords, but I wanted to bring attention to this.

This could be an argument for not adding additional keywords; and parsing the contents of the rule as a general declaration list (like #124). And handling everything else when converting to the parsed AST to the domain model (CSSNodeToDomainObject). Though the "real" solution should be (in my opinion) to get the parser to tokenize differently depending on the context.

Yes this can be solved by introducing a dedicated lexer state for the property rule

[blutorange]

The @Property rule must not be nested. It must appear at the top level of a stylesheet

I know, I only meant it should be added to produce a parser error, like it's already in various places in the jjt grammar.

[shagkur]

The @Property rule must not be nested. It must appear at the top level of a stylesheet

I know, I only meant it should be added to produce a parser error, like it's already in various places in the jjt grammar.

Hmm.... i thought i already did that. See mediaRuleList() for instance

[blutorange]

Hmm.... i thought i already did that. See mediaRuleList() for instance

Yes you did. Sorry for the confusion: I added a new production that does not contain propertyRule -- so it needs to be added to either your or my PR, depending on which gets merged first ;)

[phax]

Thanks for taking this on!!! 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.

[shagkur]

@blutorange Do you have an idea why couple of the new unit tests now fail? I already use the build of this branch in a project and there i don't get any errors. Do you have, by any chance, a way to debug?

[shagkur]

Thanks for taking this on!!! 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.

Sure no worries. Meanwhile we use it as a local build for our project. With my PRs i just want to make sure to feed back changes/additions/improvements, i have to make to the parser, to make sure it finds its way back into the main stream