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 declaration-property-value-no-unknown false negatives for typed custom properties #7078

5 changes: 5 additions & 0 deletions .changeset/nice-toes-reflect.md
@@ -0,0 +1,5 @@
---
"stylelint": patch
---

Fixed: `declaration-property-value-no-unknown` false negatives for typed custom properties
103 changes: 102 additions & 1 deletion lib/rules/declaration-property-value-no-unknown/__tests__/index.mjs
@@ -1,3 +1,5 @@
import { stripIndent } from 'common-tags';

import rule from '../index.js';
const { messages, ruleName } = rule;

Expand Down Expand Up @@ -52,12 +54,81 @@ testRule({
code: 'a { font-size: max(1rem, 3rem); }',
},
{
code: `
code: stripIndent`
a { font-weight: bolder; }
@font-face { font-weight: 100 200; }
`,
description: 'at-rule descriptor and property with the same name but different syntaxes',
},
{
code: stripIndent`
a { --foo: red; }

@property --foo {
syntax: "<color>";
inherits: false;
initial-value: #c0ffee;
}
`,
},
{
code: stripIndent`
a { --foo: 10px; }
a { --foo: 10%; }

@property --foo {
syntax: "<length> | <percentage>";
inherits: false;
initial-value: 10px;
}
`,
},
{
code: stripIndent`
a { --foo: bigger; }
a { --foo: BIGGER; }

@property --foo {
syntax: "big | bigger | BIGGER";
inherits: false;
initial-value: big;
}
`,
},
{
code: stripIndent`
a { --foo: 10px; }
a { --foo: 10px 10vh; }

@property --foo {
syntax: "<length>+";
inherits: false;
initial-value: 10px;
}
`,
},
{
code: stripIndent`
a { --foo: red; }
a { --foo: 10px 10vh; }

@property --foo {
syntax: "*";
inherits: false;
initial-value: 10px;
}
`,
},
{
code: stripIndent`
a { --foo: var(--bar); }
@property --foo {
syntax: "<color>";
inherits: false;
initial-value: #c0ffee;
}
`,
},
],

reject: [
Expand Down Expand Up @@ -149,6 +220,36 @@ testRule({
endLine: 1,
endColumn: 30,
},
{
code: stripIndent`
a { --foo: 10px; }
@property --foo {
syntax: "<color>";
inherits: false;
initial-value: #c0ffee;
}
`,
message: messages.rejected('--foo', '10px'),
line: 1,
column: 12,
endLine: 1,
endColumn: 16,
},
{
code: stripIndent`
a { --foo: 10px; }
@property --foo {
syntax: not-a-string;
inherits: false;
initial-value: #c0ffee;
}
`,
message: messages.rejected('syntax', 'not-a-string'),
line: 3,
column: 10,
endLine: 3,
endColumn: 22,
},
],
});

Expand Down
62 changes: 57 additions & 5 deletions lib/rules/declaration-property-value-no-unknown/index.js
@@ -1,7 +1,7 @@
'use strict';

const { isPlainObject } = require('is-plain-object');
const { fork, parse, find } = require('css-tree');
const { fork, parse, find, string } = require('css-tree');

const declarationValueIndex = require('../../utils/declarationValueIndex');
const matchesStringOrRegExp = require('../../utils/matchesStringOrRegExp');
Expand All @@ -13,7 +13,7 @@ const isCustomProperty = require('../../utils/isCustomProperty');
const isStandardSyntaxValue = require('../../utils/isStandardSyntaxValue');
const isStandardSyntaxProperty = require('../../utils/isStandardSyntaxProperty');
const isStandardSyntaxDeclaration = require('../../utils/isStandardSyntaxDeclaration');
const { isAtRule } = require('../../utils/typeGuards');
const { isAtRule, isDeclaration } = require('../../utils/typeGuards');
const { isRegExp, isString } = require('../../utils/validateTypes');
const { nestingSupportedAtKeywords } = require('../../reference/atKeywords');

Expand All @@ -29,6 +29,8 @@ const meta = {
url: 'https://stylelint.io/user-guide/rules/declaration-property-value-no-unknown',
};

const SYNTAX_PROPERTY = /^syntax$/i;

/** @type {import('stylelint').Rule} */
const rule = (primary, secondaryOptions) => {
return (root, result) => {
Expand Down Expand Up @@ -63,9 +65,47 @@ const rule = (primary, secondaryOptions) => {
return valuePattern && matchesStringOrRegExp(value, valuePattern);
};

const propertiesSyntax = (secondaryOptions && secondaryOptions.propertiesSyntax) || {};
const propertiesSyntax = {
// Take a shallow clone as this object will be appended to.
...((secondaryOptions && secondaryOptions.propertiesSyntax) || {}),
};
const typesSyntax = (secondaryOptions && secondaryOptions.typesSyntax) || {};
romainmenke marked this conversation as resolved.
Show resolved Hide resolved

const typedCustomPropertyNames = new Map();
romainmenke marked this conversation as resolved.
Show resolved Hide resolved

root.walkAtRules(/^property$/i, (atRule) => {
const propName = atRule.params.trim();

if (!propName || !atRule.nodes || !isCustomProperty(propName)) return;

for (const node of atRule.nodes) {
if (isDeclaration(node) && SYNTAX_PROPERTY.test(node.prop)) {
const value = node.value.trim();
const unquoted = string.decode(value);

// Only string values are valid.
// We can not check the syntax of this property.
if (unquoted === value) continue;

// Any value is allowed in this custom property.
// We don't need to check this property.
if (unquoted === '*') continue;

// https://github.com/csstree/csstree/pull/256
// We can circumvent this issue by prefixing the property name,
// making it a vendor-prefixed property instead of a custom property.
// No one should be using `-stylelint--` as a property prefix.
//
// When this is resolved `typedCustomPropertyNames` can become a `Set<string>`
// and the prefix can be removed.
const prefixedPropName = `-stylelint${propName}`;

typedCustomPropertyNames.set(propName, prefixedPropName);
propertiesSyntax[prefixedPropName] = unquoted;
}
}
});

const forkedLexer = fork({
properties: propertiesSyntax,
types: typesSyntax,
Expand All @@ -84,10 +124,22 @@ const rule = (primary, secondaryOptions) => {

if (!isStandardSyntaxValue(value)) return;

if (isCustomProperty(prop)) return;
if (isCustomProperty(prop) && !typedCustomPropertyNames.has(prop)) return;

if (isPropIgnored(prop, value)) return;

// https://github.com/mdn/data/pull/674
// `initial-value` has a incorrect syntax definition.
// In reality everything is valid.
if (
/^initial-value$/i.test(prop) &&
decl.parent &&
isAtRule(decl.parent) &&
/^property$/i.test(decl.parent.name)
) {
return;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't cache the regexp's here because this entire block should be removed.

I don't mind optimizing the code anyway.
But I am really hoping that csstree will receive some attention in the near future.


/** @type {import('css-tree').CssNode} */
let cssTreeValueNode;

Expand All @@ -114,7 +166,7 @@ const rule = (primary, secondaryOptions) => {
const { error } =
parent && isAtRule(parent) && !nestingSupportedAtKeywords.has(parent.name.toLowerCase())
? forkedLexer.matchAtruleDescriptor(parent.name, prop, cssTreeValueNode)
: forkedLexer.matchProperty(prop, cssTreeValueNode);
: forkedLexer.matchProperty(typedCustomPropertyNames.get(prop) ?? prop, cssTreeValueNode);

if (!error) return;

Expand Down