[Bug]: Asset Combiner prevents usage of CSS `var()`

CupNoodles commented 3 years ago

What happened?

Version is 3.1.1

Anywhere in a theme's scss, put in a block like

$sass_var: 100px;
padding-top: var($sass_var);

And then compile to CSS with the method of your choice. You should see that the compiled css (in case of orange theme at assets/css/app.css) the $sass_var has been compiled into

padding-top: var(100px);

Now, make sure that when 'enableAssetCombiner' => FALSE in config/system.php, your site will correctly load app.css and you'll see the padding-top: var(100px); line in your css.

Turn on asset combiner so that 'enableAssetCombiner' => TRUE and load the same page, which will now point to a minified version of your css that is specifically missing padding-top: var(100px);

What did you expect to happen?

var() is valid CSS as per https://developer.mozilla.org/en-US/docs/Web/CSS/var() and I think should be allowed to exist here?

Version

Other (please detail)

What browser are you seeing the problem on?

Firefox

Relevant log output

No response

sampoyigi commented 2 years ago

I'm unable to reproduce the error, works fine for me on the frontend and admin. Could be caching issue?

sampoyigi commented 2 years ago

Oh, sorry for the delay in reviewing. :)

CupNoodles commented 2 years ago

I'm pretty certain it's not a browser-side caching issue but it may be a Laravel caching issue that I'm not aware of.

I've made a fresh installation to test it at https://www.tasty-testing.com/ to make sure it's not a side effect of my other plugins - you can currently see that at the bottom of https://www.tasty-testing.com/_assets/1452465ec47c99502dae464829a8b2a5-1637093243.css, there is .bug_testing{padding-bottom:100px} whereas I've added

.bug_testing{
$sass_var: 100px;
padding-top: var($sass_var);
padding-bottom: 100px;
}

to the bottom of themes/tastyigniter-orange/assets/src/scss/app.scss (and then hit save on the template in the admin ui in order to compile sass). The fact that padding-bottom: 100px; shows up tells me that it's compiling and that it's getting through the cache.

I'm sorry to be pointing you at such an esoteric and probably-inconsequential bug! I'm happy to try and help but I frankly don't have any idea how the asset combiner works, and it's a bit above my level of understanding of Laravel.

sampoyigi commented 2 years ago

There's no need to apologise. The asset combiner was adapted from Symfony's Assetic Manager and it seems the issue is coming from minifying the CSS, disabling minify outputs the CSS as expected. We were using an outdated CSSMin package, I've replaced it with one adapted from octobercms.

https://github.com/tastyigniter/flame/commit/7d8f5b9a4ee82f417c01f093effe1a4887b9f46c https://github.com/tastyigniter/TastyIgniter/commit/1db10760e626072ecab88c027f247a3bd9a4c7b3

tastyigniter / TastyIgniter