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

Fraction optimizer (level 1) breaks image-set URLs containing zeros and dots #1183

Closed
squiddy opened this issue Sep 9, 2021 · 3 comments
Closed

Comments

@squiddy
Copy link

squiddy commented Sep 9, 2021

@tomyo and I noticed an issue where running clean-css over our CSS would sometimes mangle image URLs that contain zeros and dots, e.g. assets containing a hash generated by webpack.

For example category_book_medicine@2x.90ab2594.png would get changed to category_book_medicine@2x.9ab2594.png (zero removed).

Environment

  • clean-css version - master / 5.1.5:
  • node.js version - 12.21.0:
  • operating system: Pop!_OS 21.04

Configuration options

var CleanCSS = require('clean-css');
new CleanCSS({
  level: 1
})

Input CSS

div { 
  background: image-set(url('foo.10.png') 1x); 
}

Actual output CSS

div{background:image-set(url('foo.1.png') 1x)}

Note how 10 was changed to 1. This can also observed with URLs such as 10.10.png, which get's changed to 10.1.png.

Expected output CSS

div{background:image-set(url('foo.10.png') 1x)}

Culprit

We could trace this issue back to the optimizations happening in fraction.js. Disabling just this one optimization returns the expected result. There are a couple other optimizations that return early when their input is considered an URL (startsAsUrl), however in the case of image-set that function returns false.

@squiddy
Copy link
Author

squiddy commented Sep 10, 2021

Looking at the code, a level 1 optimizer only operates on single properties/values, so an easy fix would probably be to skip the optimization if the value is either

  • an url
  • an image-set

If that is fine I'm happy to provide a PR.

I was hesitating with that approach because I thought that long-term this library probably wants to apply the same optimizations to url inside image-set as it does to plain url values. Looks to be a bigger task though.

@jakubpawlowicz
Copy link
Collaborator

Hey @squiddy - the fix you suggested is an easy workaround.

However we clearly shouldn't be interpreting any values inside url() declaration no matter where they appear, and I'm not 100% sure url and image-set are the only cases. We recently had a fix for 'zero units' which can appear within linear-gradient and similar, see d53fe2f - could be a good place to start.

The choice is yours, I'll be happy with whatever fixes this. If that's the ease fix we'll open an issue for a better fix in the future.

@jakubpawlowicz
Copy link
Collaborator

Fixed on master, due in 5.3 tomorrow.

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

No branches or pull requests

2 participants