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 false positives for hex colours in color-function-notation #5638

Closed
m4thieulavoie opened this issue Oct 22, 2021 · 13 comments · Fixed by #5650
Closed

Fix false positives for hex colours in color-function-notation #5638

m4thieulavoie opened this issue Oct 22, 2021 · 13 comments · Fixed by #5650
Labels
status: ready to implement is ready to be worked on by someone syntax: scss relates to SCSS and SCSS-like syntax type: bug a problem with a feature or rule

Comments

@m4thieulavoie
Copy link
Contributor

Clearly describe the bug

After upgrading to the latest major, I experienced an issue with color-function-notation, and turns out it wasn't an issue per-say, but I think stylelint could've made a better job at telling me the issue.

So I had this rule prior to the upgrade

background-color: rgba(#fff, 0.85);

After running stylelint, I had the warning I should use color-function-notation (nice!). So what I did is the following

background-color: rgba(#fff / 0.85);

After that, stylelint passed. However, it was a false-positive as when I built my SCSS, it actually failed. I discovered that hex colors aren't allowed. The correct syntax is the following

background-color: rgba(255 255 255 / 0.85);

Which rule, if any, is the bug related to?

color-function-notation

What code is needed to reproduce the bug?

a {
  background-color: rgba(#fff / 0.85);
}

What Stylelint configuration is needed to reproduce the bug?

Default one or the following

{
  "rules": {
    "color-function-notation": "modern"
  }
}

Which version of Stylelint are you using?

"stylelint": "14.0.0",

How are you running stylelint: CLI, PostCSS plugin, Node.js API?

npx stylelint

Does the bug relate to non-standard syntax (e.g. SCSS, Less etc.)?

I don't think so?

What did you expect to happen?

A warning from stylelint directly in my code (with a tooltip)

What actually happened (e.g. what warnings or errors did you get)?

✅ from stylelint when it actually was invalid code

@alexander-akait
Copy link
Member

/ is evil in SCSS 😄

@lachieh
Copy link
Contributor

lachieh commented Oct 23, 2021

This is not really an issue with stylelint as this relates to the fact that SCSS is interpreting the #fff / 0.85 as an operation, rather than standard CSS syntax.

The Sass website has a page dedicated to the fact that SCSS is using slash as division. There is also an issue on their GitHub that is related: sass/sass#2565

There are really two options here:

  1. Don't use color-function-notation alongside SCSS for the time being
  2. Make sure you're using dart-sass@1.33.0 or higher and use the migration guide to stop using slash as division. It's worth noting that Sass is making the change to remove slash as division so this will eventually be the way forward.

@hudochenkov
Copy link
Member

I think bug report is correct. background-color: color-function-notation should not show a problem with invalid CSS rgba(#fff, 0.85);.

@lachieh
Copy link
Contributor

lachieh commented Oct 24, 2021

That's a good point. If this gets labeled up, I'd be keen to fix this issue.

@m4thieulavoie
Copy link
Contributor Author

This is not really an issue with stylelint as this relates to the fact that SCSS is interpreting the #fff / 0.85 as an operation, rather than standard CSS syntax.

The Sass website has a page dedicated to the fact that SCSS is using slash as division. There is also an issue on their GitHub that is related: sass/sass#2565

There are really two options here:

  1. Don't use color-function-notation alongside SCSS for the time being
  2. Make sure you're using dart-sass@1.33.0 or higher and use the migration guide to stop using slash as division. It's worth noting that Sass is making the change to remove slash as division so this will eventually be the way forward.

Thanks for the context! I think for now I can live with doing 2. knowing this will eventually be fixed by sass. I think it'd still be nice for stylelint to let the user know that this kind of operation is going to fail at the sass step. Sounds like a false-positive when stylelint passes but sass doesn't 😄

@lachieh
Copy link
Contributor

lachieh commented Oct 24, 2021

@m4thieulavoie no worries! It still seems like this is a bug as @hudochenkov pointed out because stylelint should not have suggested the change to the new color function syntax since background-color: rgba(#fff, 0.85); is invalid standard css syntax 👍🏻

@jeddy3
Copy link
Member

jeddy3 commented Oct 25, 2021

Let's add an isStandardSyntaxColorFunction util that has heuristics for this. Probably pass in the function node and check if the nested first word node is a hex colour. And add a comment to that conditional saying it's for these sass functions.

@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone syntax: scss relates to SCSS and SCSS-like syntax type: bug a problem with a feature or rule labels Oct 25, 2021
@jeddy3 jeddy3 changed the title [color-function-notation] Add extra warnings for hex colors Fix false positives for hex colours in color-function-notation Oct 25, 2021
@lachieh
Copy link
Contributor

lachieh commented Oct 25, 2021

I'll pick this one up 👍🏻

@lachieh
Copy link
Contributor

lachieh commented Oct 25, 2021

Let's add an isStandardSyntaxColorFunction util that has heuristics for this. Probably pass in the function node and check if the nested first word node is a hex colour. And add a comment to that conditional saying it's for these sass functions.

@jeddy3 Should this check for all possible color functions by name? I have a version working that does as you say above—find first word node and check if it is valid hex—but wanted to check if it should be more like isMathFunction.

@jeddy3
Copy link
Member

jeddy3 commented Oct 25, 2021

Should this check for all possible color functions by name?

Nope. We just need the minimal amount of heuristics in it to identify non-standard syntax.

We could make it nieve like isStandardSyntaxMathFunction, but I think that'll introduce false negatives for comments (e.g. rgba(/* #aaa */ 23, 23, 23. 0.1)). Probably best to reach for the value parser for a more robust util.

find first word node and check if it is valid hex

No need to check if it's a valid hex... just if it's a word starting with #.

@lachieh
Copy link
Contributor

lachieh commented Oct 25, 2021

Thanks, makes sense. PR submitted!

@cascornelissen
Copy link

cascornelissen commented Oct 29, 2021

@lachieh, I tried out main from GitHub directly as the PR isn't in a release yet and am seeing that for example the following is still a false positive, I think at least?

@use 'sass:color';

$color: rgba(color.mix(#000, #fff, 35%), 0.6);

@jeddy3
Copy link
Member

jeddy3 commented Oct 29, 2021

@cascornelissen Yes, it won't catch that. Can create a new issue as we'll need discussion how, and even if, we can fix it for that pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready to implement is ready to be worked on by someone syntax: scss relates to SCSS and SCSS-like syntax type: bug a problem with a feature or rule
Development

Successfully merging a pull request may close this issue.

6 participants