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

Optimizing properties of level 2 removes hsl values #1181

Open
Vatoth opened this issue Aug 24, 2021 · 12 comments
Open

Optimizing properties of level 2 removes hsl values #1181

Vatoth opened this issue Aug 24, 2021 · 12 comments

Comments

@Vatoth
Copy link

Vatoth commented Aug 24, 2021

Precheck

Using 2 level, color values specified in hsl format are removed.

Similar to #1156

Environment

  • clean-css version - 5.1.5
  • node.js version - 14.17.5
  • operating system - Linux 5.13.12

Configuration options

var CleanCSS = require('clean-css');
new CleanCSS({ inline: false, level: 2 })

Input CSS

.zone.contacts .size_1_3 > a {
      border: 3px solid hsl(0deg 0 85%);
}

Actual output CSS

.zone.contacts .size_1_3>a{border:3px solid}

Expected output CSS

.zone.contacts .size_1_3>a{border:3px solid hsl(0deg 0 85%)}
@jakubpawlowicz
Copy link
Collaborator

Hey @Vatoth - I've only tested in latest Firefox and Chrome but hsl(0deg 0 85%) seems to be an invalid color value, hence the reason clean-css removes them. Is it a valid color value in any case?

@Vatoth
Copy link
Author

Vatoth commented Aug 30, 2021

Hello, @jakubpawlowicz. Yeah nice catch!

I'm trying the lib without the deg and with hsl(0 0 85%), but it doesn't appear to work either :(.

I get it working by adding a comma to hsl(0, 0, 85%), which produce #d8d8d8 as expected but not with the spaces :/

I'm using the hsl with "space-separated functional notation"

https://caniuse.com/mdn-css_types_color_space_separated_functional_notation

It's not supported by IE11, unfortunately :/

Thank you very much for your time.

@jakubpawlowicz
Copy link
Collaborator

It's fixed on main branch, due in 5.3 today.

@boris-petrov
Copy link

@jakubpawlowicz - are you sure that's fixed? Using 5.3.0 and the same example as in the original post I get the same result as him - hsl is removed. Adding commas also doesn't help.

@jakubpawlowicz
Copy link
Collaborator

@boris-petrov 5.3.0 is the version used on https://clean-css.github.io/ - there it works just fine with the original bug report example. Is your case any different? Could be an edge case.

@boris-petrov
Copy link

@jakubpawlowicz - you can also reproduce the issue there - but be sure to enable level 2 optimizations (which aren't on by default).

@jakubpawlowicz
Copy link
Collaborator

My bad, you are right. Will look into it.

@jakubpawlowicz
Copy link
Collaborator

It turned out clean-css.github.io didn't have 5.3.0 deployed. Now it works there. Tests also pass fine so it could be an edge case.

@boris-petrov
Copy link

@jakubpawlowicz not sure if the problem is on my side but I still get the same result on https://clean-css.github.io/ . Also, it still says it's using version 5.1.3, perhaps some caching issue? I did try with multiple browsers though.

I also tried clean-css-cli - installed version 5.6.0 (not sure which clean-css it uses underneath) but it gives the same result. Trying with the example on the original post:

foo.css:

.zone.contacts .size_1_3 > a {
      border: 3px solid hsl(0deg 0 85%);
}
% cleancss -O2 foo.css
.zone.contacts .size_1_3>a{border:3px solid}

@jakubpawlowicz
Copy link
Collaborator

I see what's the problem: consider "hsl(0deg 0 85%)" vs "hsl(0deg 0% 85%)". Is hsl color supposed to work when second value is not given a percentage? Could it be fine only when the value is zero?

@boris-petrov
Copy link

boris-petrov commented May 11, 2022

Not sure about that, probably it is OK to not use percentage as in most places CSS accepts also zero without a unit.

Also, here are more examples that are removed:

.foo {
  border-top: 0.063rem solid hsl(210deg, 10%, 24%);
}

.foo {
  border-top: 0.063rem solid hsl(210deg, 10.8108108108%, 24.649859944%);
}

@jakubpawlowicz
Copy link
Collaborator

Thanks, I'll dig into it.

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

No branches or pull requests

3 participants