Add declaration-property-value-no-unknown

[jeddy3]

Which issue, if any, is this issue related to?

Closes #6376

Is there anything in the PR that needs further explanation?

I figured it'd be a good idea to include this rule in v15. It is a significant change in our direction to offer more validation rules and a change that counterweighs our decision to deprecate most of the stylistic rules. It'll be useful for people to see that in our migration guide. Additionally, the rule is doing a lot and will flag problems for people using values not defined in the CSS specifications. We can help these people by including an entry in the migration guide

As discussed in #6376 (comment), the scoped is limited to checking for mismatched property-value pairs.

Most test cases come from issues people have opened in our tracker over the years.

I've marked as draft as I'm unsure about two things:

• the is-plain-object import
• a TypeScript error

lib/rules/declaration-property-value-no-unknown/index.js:4:9 - error TS2305: Module '"css-tree"' has no exported member 'fork'.

4 const { fork, parse } = require('css-tree');
          ~~~
Found 1 error in lib/rules/declaration-property-value-no-unknown/index.js:4

Any advice?

[changeset-bot]

⚠️ No Changeset found

Latest commit: 2043d67

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[ybiquitous]

@jeddy3 I've pushed a @types/css-tree package patch (b911885) to fix the type errors using patch-package, although I'm unsure whether fork() is a public API and the patched types are correct.

[jeddy3]

This rule is the final piece of the v15 puzzle now that we merged #6561 & #6573.

Let's decide whether we want it in.

As mentioned in #6511, including it now will help communicate to people the type of problems Stylelint can help them with going forward. We've closed two issues (#6591 & #6576) just in the last couple of weeks requesting it, and there's a desire for it in this Twitter thread. People expect and want Stylelint to do this.

However, it comes at the cost of a larger package size and likely some maintenance. For example, CSSTree flags these two:

• font-size: clamp(1rem, 1vw + 1rem, 3rem);
• color: -moz-initial;

Support for clamp (and other math functions) is coming.

Fortunately, the syntax that CSSTree uses can be extended. So, CSSTree can do the heavy lifting in most cases, and we can maintain our own references for things that we can't quickly patch in CSSTree.

There will be edge cases, but we can limit their impact. Regardless, this rule will be beneficial to most people.

What does everyone think?

Unfortunately, I will not be as available as I thought I would be over the coming weeks. If we decide we want this rule in, would someone like to pick it up and get it over the finish line?

[Mouvedia]

Fortunately, the syntax that CSSTree uses can be extended.

We will probably have a regular stream of issues regarding false positives that will end up being upstreamed to csstree.
To avoid the back and forth—PR here waiting for their release and one after to remove the useless extension—someone from the stylelint organisation needs to have at least merge rights on their repo if we intend to collaborate that tightly.
This way we won't have to wait, we will just use a commit SHA in the package.json if their release cycles are too long.

If that's the case I am all for it.

[ybiquitous]

@jeddy3 Sorry for the late reply. I also believe this rule will benefit most people, but I'm a bit concerned about potential false positives and inactivity of the CSSTree repo, as @Mouvedia says.

• font-size: clamp(1rem, 1vw + 1rem, 3rem);

The math functions support like clamp() or max() is not coming yet, so we should avoid false positives by ignoring such functions, for example:

if (/^(clamp|min|max)\(/i.test(value)) return;

Extending via fork() seems not to work in this case.

• color: -moz-initial;

CSSTree seems not to support the legacy keyword for initial and extend keywords via fork(). So we should avoid false positives by just ignoring the keyword, for example:

if (/^-moz-initial$/i.test(value)) return;

Sending a patch to CSSTree may be possible, but this is an edge case, so I believe it is sufficient to add the workaround code to this rule.

---

Additionally, I find this rule may be duplicated with color-no-invalid-hex. These two rules reports for the following CSS code at the same time:

a { color: #4f; }

Unexpected invalid hex color "#4f"                   color-no-invalid-hex
Unexpected unknown value "#4f" for property "color"  declaration-property-value-no-unknown

---

To summarize, despite potential false positives, I believe this rule has a big value for most people, and CSSTree is suitable to achieve it now.

Perhaps, the author of CSSTree and stylelint-validator may collaborate with us because stylelint-validator could be replaced with this rule and future built-in rules using CSSTree.

---

If we decide we want this rule in, would someone like to pick it up and get it over the finish line?

I can do it. ✋🏼

[Mouvedia]

Perhaps, the author of CSSTree and stylelint-validator may collaborate with us because stylelint-validator could be replaced with this rule and future built-in rules using CSSTree.

@lahmatiy what's your opinion on that?

Additionally, I find this rule may be duplicated with color-no-invalid-hex.

Nice catch. If declaration-property-value-no-unknown is added color-no-invalid-hex should be deprecated.

So we should avoid false positives by just ignoring the keyword…

Ill add all the unsupported keywords/functions that I know of.

e.g.
width: min-intrinsic;
height: -webkit-fill-available;

I won't add stuff that I consider out of scope though.
e.g. css expressions, progid, XBL, etc.

[lahmatiy]

@Mouvedia I'm open for a collaboration.

CSSTree is out of my focus currently. That primally affects my ability to work on complex features, I have no enough time for it at the moment. However, I can review simple PRs and publish releases when needed.

For example, CSSTree flags these four:

• font-size: clamp(1rem, 1vw + 1rem, 3rem);
• color: -moz-initial;
  ...
• width: min-intrinsic;
• height: -webkit-fill-available;

Let me start with how syntax matching (a validation is based on it) works. There is no specific code in CSSTree for a property matching/validation, like many other libraries do. Instead, it uses a general solution based on Value Definition Syntax defined in Values and Units Module. Using this approach, you can easily add new properties or change existing by making changes in a dictionary. Mentioned fork() method allows to have a fork of based CSSTree, but with altered dictionaries.

CSSTree uses mdn-data as a primary dictionary of syntax definitions, with some additions and fixes. In most cases, you need to add/fix a definition in mdn-data dictionary. However, mdn-data's releases a pretty rare events, so it may be much faster to make changes in CSSTree as well.

In fact, in my opinion, the dictionary should be a separate project, which will be aimed at providing a dictionary for use in tools. mdn has no such goal, although it is used in tools (with hacks, like in CSSTree) for lack of a better one. But this is a topic for a separate discussion.

4 cases above is about 3 different areas. Last two is most simple, it's about a missing in the dictionary. You can see current definition for width and height properties in CSSTree syntax reference dashboard. The width property is already patched by CSSTree to add some non-standard values:

So you can add missing keywords to these properties the same way.

-moz-initial is a vendor-prefixed CSS-wide keyword. So, that's not a part of any property definition, but a common value for any property. CSSTree has a build-in list of CSS-wide keywords. It can't be altered with fork() method at the moment, but this ability should be easy to add. I'm not sure that -moz-initial should be in the list by default.

clamp(), min(), max() and other math functions are already in the dictionary. These functions, together with calc(), are special because they are not a part of any property, but can replace any dimension, percentage or number value. So processing of these function is different. There is a list of calc functions which can be expanded (and named as math function, with support to alter it with fork() method). The problem is that the contents of such functions will not be validated as well as their position is correct (for example, that a function substituted instead of a number returns a number, not pixels). To add such validation takes time, this is a big feature. However, I think a lack of validation is better than warning on such functions.

Btw, I believe that there are much more missing parts in dictionaries. Some time ago I started working on a project to automate the synchronization of syntaxes with CSS specs. Also worked on a project to collect CSS usage data from real web sites. I believe that such projects can be helpful to fill the gaps.

if (/^(clamp|min|max)\(/i.test(value)) return;

Avoid using fixes like this. clamp/min/max/etc functions can be placed anywhere in a value.

[Mouvedia]

Avoid using fixes like this. clamp/min/max/etc functions can be placed anywhere in a value.

Here's an example of such a case : content: ' (clamp: ' attr(max) ') ';.

CSSTree uses mdn-data as a primary dictionary of syntax definitions, with some additions and fixes. In most cases, you need to add/fix a definition in mdn-data dictionary. However, mdn-data's releases a pretty rare events, so it may be much faster to make changes in CSSTree as well.

I reckon we will need to add around 10 exceptions, at first. @lahmatiy What method would you recommend for that?

• something on our side
• csstree
  • edit patch.json
  • edit the relevant mdn-data dictionaries and/or csstree files
  • a separate stylelint.json that we are responsible for

What we want is to avoid rebuttals/refusals/delays on csstree's side. The goal is to have the lowest number of steps.

However, I can review simple PRs and publish releases when needed.

This is great to hear.

Btw, I believe that there are much more missing parts in dictionaries.

If you have a list for values specifically, please provide it.

[jeddy3]

@lahmatiy Thank you for chiming in and explaining everything so clearly.

It's fantastic that you're open to collaboration. I think we can bring more awareness to your work on dictionaries, usage and syntax as Stylelint is often the entry point to CSS tooling for people. This will hopefully lead to more people contributing to the dictionary (and even the possibility of it becoming a separate project).

I typically use your syntax reference website when proposing new rules for Stylelint and I think it's great!

However, I think a lack of validation is better than warning on such functions.

I agree. Out of curiosity, is var() handled the same way as I noticed that doesn't flag warnings?

It sounds like we can:

• add width: min-intrinsic height: -webkit-fill-available to mdn-data and patch in CSSTree
• workaround -moz-initial in Stylelint until the built-in list of CSS-wide keywords can be altered with fork()
• use fork() on the generic "math function" for clamp() etc

Am I right about the last one? Is that what you meant with:

There is a list of calc functions which can be expanded (and named as math function, with support to alter it with fork() method).

I agree with @ybiquitous when he said:

To summarize, despite potential false positives, I believe this rule has a big value for most people, and CSSTree is suitable to achieve it now.

The rule will benefit most people and we now have an outline for handling future syntaxes and edge cases.

[jeddy3]

Additionally, I find this rule may be duplicated with color-no-invalid-hex.

I touched on this in #6376 (comment). The rule also overlaps with unit-no-unknown and function-no-unknown when checking declarations.

We could either leave the rules in, deprecate them or limit them to at-rules.

(The latter opens up a broader discussion about a new at-rule-name-prelude-no-unknown rule that also uses CSSTree.)

We can leave them in for now to keep things simple. We already have overlapping rules, e.g. all the *-disallowed-list and *-allowed-list ones. Keeping them in gives users a choice of using a more general-purpose property & value pair validating rule or something specific to hex colours, functions, units and so on. We can document this overlapping in the appropriate rule READMEs.

[Mouvedia]

The rule also overlaps with unit-no-unknown and function-no-unknown when checking declarations.

We won't be able to deprecate them right away.

function-no-unknown: because css-functions-list package is more complete (e.g. oklch)
unit-no-unknown: because it has 2 options that won't—and should probably not—be covered by declaration-property-value-no-unknown

IMHO we should keep them as is.

It's fantastic that you're open to collaboration.

👍

[ybiquitous]

Thanks a lot, everyone. It's great to hear about this open collaboration! 🙌🏼
We will be contributing to CSSTree with the help of @lahmatiy in the future.

So now, I'd like to hear your thoughts on whether we should include this declaration-property-value-no-unknown rule in the v15.0.0 release.

Because this collaboration will likely take more time and the rule is duplicated with rules like *-no-unknown (not suitable to put them in stylelint-config-recommended together), I guess it may be good to go toward the v15 release without the rule. The v15 branch has been already long-living and so maintaing main and v15 branches should be a messy job for us.

What do you think? @jeddy3 @Mouvedia

[Mouvedia]

I guess it may be good to go toward the v15 release without the rule.
What do you think?

We usually have at least one release per month; January was release-less.
Id vote for an early release if the release cadence matters.

[ybiquitous]

I've skipped a clamp() test to avoid CI failures via 7e91fc2. If we find a suitable workaround or CSSTree supports it, let's remove the skipping.

[ybiquitous]

What I can do now is finished. Please mention me if more work comes.

[Mouvedia]

Let's wait for csstree/csstree#243 to settle on this.

[Mouvedia]

Iv done the PRs that I was talking about in #6511 (comment).
Let's wait for @lahmatiy review/merge.

The height property will require more work on my part.
There are discrepancies to be resolved.
e.g.

Chrome and Safari supports these keywords on 'height'. Firefox doesn't.

i.e.

1. a PR on mdn/browser-compat-data or mdn/data or w3c/webref
2. a subsequent PR on csstree/csstree
3. a dependency update on stylelint/stylelint

Hence Ill postpone it.
i.e. it won't be ready for this PR

[jeddy3]

Let's favour false negatives over positives.

If we can't use fork() in this instance, let's return early if we find a clamp, min or max function in the value.

[ybiquitous]

Thanks for sharing the nice way to find functions. Fixed via 9317140.

[jeddy3]

@ybiquitous & @Mouvedia Thank you for making commits or pull requests here elsewhere.

I've pushed a commit 5872036 to update the docs to address suggestions in #6511 (comment) & #6511 (comment).

I've made one suggestion. Otherwise, I think we're almost there!

[ybiquitous]

Thanks, everyone. LGTM 👍🏼
I believe we can later apply the patches by @Mouvedia that CSSTree will include.

[ybiquitous]

I'll request this change to the upstream @types/css-tree package later.

[jeddy3]

Thanks, everyone. Looks good enough for me too.

[Mouvedia]

What I was afraid of has happened.
None of the PRs opened on csstree's side were merged.

[ybiquitous]

@lahmatiy What do you think about delegating CSSTree's maintenance to someone passionate? Since CSSTree is a great product, I believe people in the front-end community hope to make CSSTree sustainable.
(maybe this discussion should be open in CSSTree's repo, though)

[Mouvedia]

@ybiquitous in the meantime what about forking the repo and using a github branch as a dependency ?

[ybiquitous]

@Mouvedia Um, it may be better at this point.

using a github branch as a dependency

css-tree requires building, right? If forking it, do we need to publish it to npmjs.com?
Ref: https://github.com/csstree/csstree/blob/612cc5f2922b2304869497d165a0cc65257f7a8b/package.json#L85

[Mouvedia]

css-tree requires building, right?

Now that becomes annoying. It would require to use GitHub actions to build it every time a PR is merged.
I am having second thoughts now.

do we need to publish it to npmjs.com?

Nah that's overkill; I'd prefer waiting for the pending PRs to be merged.