Skip to content
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.

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

Fix custom-property-no-missing-var-function false positives for properties that can contain author-defined identifiers #5742

Closed
CyanSalt opened this issue Nov 26, 2021 · 8 comments · Fixed by #7478
Labels
status: wip is being worked on by someone type: bug a problem with a feature or rule

Comments

@CyanSalt
Copy link

CyanSalt commented Nov 26, 2021

What is the problem you're trying to solve?

Thanks to the CSS Houdini API, the property name of a custom property could appear in the value of some declarations, such as transition (or more precisely, transition-property). However, the current custom-property-no-missing-var-function rule will report errors for these custom property names that are not wrapped in var().

Online demo

What solution would you like to see?

One possibility is to not report transition, etc. by default, but this could also produce breaking changes.

It is also possible to add options to the rule to ignore certain property names, as we did in property-no-vendor-prefix.

module.exports = {
  rules: {
    'custom-property-no-missing-var-function': [true, { ignoreProperties: ['transition', 'transition-property'] }],
  }
}
@ybiquitous
Copy link
Member

@CyanSalt Thanks for using the template!

Not reporting access without var() by default looks good to me, but honestly, I'm not sure about the spec. Can you provide a link to the spec allowing not to use var(), please?

@ybiquitous ybiquitous added the status: needs clarification triage needs clarification from author label Nov 26, 2021
@CyanSalt
Copy link
Author

CyanSalt commented Nov 29, 2021

Sorry for taking so long to reply.

As far as I know, CSS Houdini is still in draft form and only Blink supports the relevant features. But as we can see in the https://stylelint.io/user-guide/rules/list/custom-property-no-missing-var-function/ documentation there are also examples of @property at-rule, and these features are already being used a lot.

As I understand the spec, transition-property actually supports the <custom-ident> syntax, which is allowed in the definition of CSS Values to pass any legal CSS identifier. This is also explicitly mentioned in the CSS Houdini draft.

For this, you can see the first example in the CSS Houdini draft, which shows exactly how this syntax is used in transition.

References:

  1. https://www.w3.org/TR/css-transitions-1/#transition-property-property
  2. https://www.w3.org/TR/css-values-4/#custom-idents
  3. https://drafts.css-houdini.org/css-properties-values-api-1/#animation-behavior-of-custom-properties
  4. https://drafts.css-houdini.org/css-properties-values-api-1/#examples

@ybiquitous
Copy link
Member

@CyanSalt Thank you for the detailed explanation! SGTM 👍🏼

@ybiquitous ybiquitous added status: ready to implement is ready to be worked on by someone and removed status: needs clarification triage needs clarification from author labels Nov 29, 2021
@ybiquitous
Copy link
Member

I've labeled the issue as ready to implement. Please consider contributing if you have time.

@ybiquitous ybiquitous added the type: enhancement a new feature that isn't related to rules label Nov 29, 2021
@CyanSalt
Copy link
Author

As a side note, I found that almost all user-defined at-rules in the CSS standard support the <custom-ident> identifier, such as @keyframes and @counter-style, which leads to a number of properties in addition to transition-property that can actually pass custom property names that are not wrapped in var().

As a possibly incomplete list, the properties I've gathered so far are as follows:

  • animation, exactly animation-name
  • counter-reset and counter-increment
  • grid-row-start, grid-row-end, grid-column-start and grid-column-end. This also means that grid-row and grid-column
  • list-style, exactly list-style-type
  • transition, exactly transition-property
  • will-change, as a closely related one to transition-property

For these properties, I think the more common use case is probably still custom properties wrapped in var(). So I suspect that ignoring them by default may not be appropriate, and perhaps an option approach would be a better choice.

As a rough design, perhaps we could provide { "ignoreCustomIdent": true } as an option, but it would be more difficult to implement. Providing ignoreProperties is probably the most cost effective option.

@ybiquitous
Copy link
Member

Providing ignoreProperties is probably the most cost effective option.

@CyanSalt Thank you so much! The idea above sounds good to me, too. 👍🏼

@jeddy3 Any thoughts?

@jeddy3 jeddy3 changed the title custom-property-no-missing-var-function with transition Fix false positives for properties that can contain author-defined identifiers in custom-property-no-missing-var-function Feb 8, 2022
@jeddy3 jeddy3 added type: bug a problem with a feature or rule and removed type: enhancement a new feature that isn't related to rules labels Feb 8, 2022
@jeddy3
Copy link
Member

jeddy3 commented Feb 8, 2022

@jeddy3 Any thoughts?

Sorry, I missed this.

As a side note, I found that almost all user-defined at-rules in the CSS standard support the <custom-ident> identifier, such as @keyframes and @counter-style, which leads to a number of properties in addition to transition-property that can actually pass custom property names that are not wrapped in var().

Yes, that was the original reason for limiting this rule to defined custom properties. However, we didn't consider the scenario where a custom property is defined but intentionally not used in a var().

Thanks for drawing up the list in #5742 (comment). Ideally, we'd ignore just the <custom-ident> slot of these properties, but that's tricky to do with our current parser. I suggest we ignore the properties outright to avoid these false positives and then document this limitation, e.g. "This rule does not check properties which can contain author-defined identifiers".

@jeddy3 jeddy3 changed the title Fix false positives for properties that can contain author-defined identifiers in custom-property-no-missing-var-function Fix custom-property-no-missing-var-function false positives for properties that can contain author-defined identifiers Sep 28, 2022
@ybiquitous
Copy link
Member

I'm revisiting this old issue, but still valid. I'll address it according to the suggestion on #5742 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone type: bug a problem with a feature or rule
3 participants