Skip to content

Fix declaration-property-value-keyword-no-deprecated false positives for arguments of unknown functions#8579

Closed
Mouvedia wants to merge 2 commits into
stylelint:mainfrom
Mouvedia:8575-dpvknd-function
Closed

Fix declaration-property-value-keyword-no-deprecated false positives for arguments of unknown functions#8579
Mouvedia wants to merge 2 commits into
stylelint:mainfrom
Mouvedia:8575-dpvknd-function

Conversation

@Mouvedia

Copy link
Copy Markdown
Member

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

Closes #8575

Is there anything in the PR that needs further explanation?

N/A

@changeset-bot

changeset-bot Bot commented May 16, 2025

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 298a282

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Patch

Not sure what this means? Click here to learn what changesets are.

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

@github-actions

github-actions Bot commented May 16, 2025

Copy link
Copy Markdown
Contributor

This PR is packaged and the instant preview is available (298a282). View the demo website.

Install it locally:

npm i -D https://pkg.pr.new/stylelint@298a282

@Mouvedia Mouvedia force-pushed the 8575-dpvknd-function branch 2 times, most recently from 7390f1c to 679b463 Compare May 16, 2025 23:41
@Mouvedia Mouvedia marked this pull request as ready for review May 16, 2025 23:45
@Mouvedia Mouvedia requested review from jeddy3 and ybiquitous May 16, 2025 23:45
@Mouvedia Mouvedia marked this pull request as draft May 16, 2025 23:50
@Mouvedia Mouvedia force-pushed the 8575-dpvknd-function branch from 679b463 to 6626a7a Compare May 17, 2025 00:20

@Mouvedia Mouvedia May 17, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@@ -17,11 +17,14 @@ export const camelCaseFunctions = new Set([
export const colorFunctions = new Set([

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note

This set is used by color-named.

!hasColorFunction(declValue) &&

@Mouvedia Mouvedia force-pushed the 8575-dpvknd-function branch from 6626a7a to 05c4644 Compare May 17, 2025 00:26
@Mouvedia Mouvedia force-pushed the 8575-dpvknd-function branch from 05c4644 to f92f068 Compare May 17, 2025 00:33
'contrast-color',
'device-cmyk',
'hsl',
'hsla',

@Mouvedia Mouvedia May 17, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@Mouvedia Mouvedia marked this pull request as ready for review May 17, 2025 00:39

@romainmenke romainmenke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for working on this @Mouvedia

Comment thread lib/reference/functions.mjs
Comment thread lib/rules/declaration-property-value-keyword-no-deprecated/index.mjs Outdated
return;
}

if (node.type === 'function' && !colorFunctions.has(node.value.toLowerCase())) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it correct that we actually want to check if background is intended as a <color> or as some unrelated argument to a function?

a {
  /* background as color */
  color: rgb(from background r g b);
}

a {
  /* background as color */
  color: var(--foo, background);
}

a {
  /* background as color */
  color: rgb(from var(--foo, background) r g b);
}

a {
  /* background as color */
  color: rgb(from if(style(--foo: bar): background; else: white) r g b);
}

a {
  /* background as a random keyword */
  color: if(style(--foo: background): red; else: white);
}

a {
  /* background as a random keyword */
  color: --custom-function(background);
}

Demo

This would also be easier to keep track of with the state feature of @csstools/css-parser-algorithms.

When entering a function like var(), if(), rgb(), ccolor-mix(), contrast-color(), ... is encountered the checks for values like background would run.

When entering unrelated functions like style(), --custom-function(), ... the checks would skip.

@Mouvedia Mouvedia May 17, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Out of the properties/values supported only the color functions are relevant.
Also the user would expect the rule to pick the deprecated keyword within those functions.

e.g.
calc-size doesn't support intrinsic AFAIK so it doesn't need an exception because it would be invalid in the first place. Now the actual wording in the specification is not really precise:

The production matches any intrinsic size keywords allowed in the context.

so I might be wrong.


Granted, it would be much simpler to ignore all functions.

@Mouvedia Mouvedia marked this pull request as draft May 17, 2025 10:21
@Mouvedia Mouvedia marked this pull request as ready for review May 17, 2025 11:49
@Mouvedia Mouvedia requested a review from romainmenke May 17, 2025 11:49

@Mouvedia Mouvedia May 17, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

off topic
l132

Ignored keywords may erroneously not be ignored in some cases.
i.e. it should be done inside parsedValue.walk((node) => { … }); too
It won't be fixed in this PR though.

@Mouvedia

Copy link
Copy Markdown
Member Author

@romainmenke concerning #8579 (comment)
Is the recursive approach that I went with sufficient for you?
If not, you can try your hand using @csstools packages instead since Iv allowed edits by maintainers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Fix declaration-property-value-keyword-no-deprecated false positives for function arguments

2 participants