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

CSS Compressor: Incorrect optimization for transition property #72

Closed
miroskow opened this issue Jun 23, 2022 · 16 comments
Closed

CSS Compressor: Incorrect optimization for transition property #72

miroskow opened this issue Jun 23, 2022 · 16 comments
Assignees
Labels
bug 🏆 workaround A workaround has been provided
Milestone

Comments

@miroskow
Copy link

miroskow commented Jun 23, 2022

Used resources-optimizer-maven-plugin v2.3.8

Before optimization:

.transition-test {
	transition: all 0s ease 0s;
	transition-delay: 0s;
	transition-duration: 0s;
	transition-delay: 0ms;
	transition-duration: 0ms;
}

After optimization:

.transition-test{transition:all 0 ease 0;transition-delay:0;transition-duration:0;transition-delay:0;transition-duration:0}

Removing the units of measure s, ms for a value of 0 is invalid for the transition property.

https://developer.mozilla.org/en-US/docs/Web/CSS/time
"0 Although unitless zero is allowed for [length's], it's invalid for [time's]."

@melloware
Copy link
Member

melloware commented Jun 23, 2022

OK here looks like the original issue from the YUI Compressor: yui/yuicompressor#256

Can you try my suggestion in that ticket just for now...

.transition-test {
	transition: all 0.00s ease 0.00s;
	transition-delay: 0.00s;
	transition-duration: 0.00s;
	transition-delay: 0.00ms;
	transition-duration: 0.00ms;
}

Here is another suggestion: yui/yuicompressor#342 using unset

transition-delay: unset

@miroskow
Copy link
Author

miroskow commented Jun 23, 2022

The workaround works, but it does not resolve the issue.
Thanks.

After optimization:

.transition-test{transition:all .00s ease .00s;transition-delay:.00s;transition-duration:.00s;transition-delay:.00ms;transition-duration:.00ms}

With one decimal place, units are also removed.
As you said, we need to add two decimal places.

@jepsar
Copy link
Member

jepsar commented Jun 23, 2022

The YUI compressor project looks pretty dead to me...

@melloware
Copy link
Member

melloware commented Jun 23, 2022

its totally dead that is why I copied the code into this project and patched it myself instead of using their lib like it used to. So I have CssCompressor.java and I had patched like 5 PR's from YUI that have been open for years.

@melloware
Copy link
Member

Did you try unset as well?

@miroskow
Copy link
Author

I chose the first workaround.

@melloware
Copy link
Member

Oh I thought you said "but it does not resolve the issue." like that didn't fix it.

@melloware melloware added the 🏆 workaround A workaround has been provided label Jun 23, 2022
@miroskow
Copy link
Author

miroskow commented Jun 23, 2022

Before that, I used closure-compiler-maven-plugin v2.22.0
But your resources-optimizer-maven-plugin works faster :)
By the way, I wanted to use css optimization.
And this problem has arisen.

Maybe someone will improve it in the future.

@melloware
Copy link
Member

OK I am modifying that guy's PR which had some issues but hopefully it will fix this issue for 2.3.9!

@melloware melloware added the bug label Jun 23, 2022
@melloware melloware added this to the 2.3.9 milestone Jun 23, 2022
@melloware
Copy link
Member

If I push 2.3.9 to Maven Central would you try it @miroskow ??

@miroskow
Copy link
Author

I'm looking forward to version 2.3.9 :)

@melloware
Copy link
Member

ok let me do a build I will let you know when its in Maven Central.

@melloware
Copy link
Member

OK 2.3.9 is now in Maven Central. Give it a shot.

@melloware melloware self-assigned this Jun 23, 2022
@miroskow
Copy link
Author

Test resources-optimizer-maven-plugin v2.3.9

Before optimization:

.transition-test {
	transition: all 0s ease 0s;
	transition-delay: 0s;
	transition-duration: 0s;
	transition-delay: 0ms;
	transition-duration: 0ms;
}

.transition-test-one-decimal-place {
	transition: all 0.0s ease 0.0s;
	transition-delay: 0.0s;
	transition-duration: 0.0s;
	transition-delay: 0.0ms;
	transition-duration: 0.0ms;
}

.transition-test-two-decimal-places {
	transition: all 0.00s ease 0.00s;
	transition-delay: 0.00s;
	transition-duration: 0.00s;
	transition-delay: 0.00ms;
	transition-duration: 0.00ms;
}

.rotating-test {
	-webkit-animation: loadpspin 0s infinite linear;
	-moz-animation: loadpspin 0.0s infinite linear;
	-ms-animation: loadpspin 0.00s infinite linear;
	animation: loadpspin 1.0s infinite linear;
	transform-origin: 50% 50%;
}

After optimization:

.transition-test{transition:all 0s ease 0s;transition-delay:0s;transition-duration:0s;transition-delay:0ms;transition-duration:0ms}
.transition-test-one-decimal-place{transition:all 0s ease 0s;transition-delay:0s;transition-duration:0s;transition-delay:0ms;transition-duration:0ms}
.transition-test-two-decimal-places{transition:all .00s ease .00s;transition-delay:.00s;transition-duration:.00s;transition-delay:.00ms;transition-duration:.00ms}
.rotating-test{-webkit-animation:loadpspin 0s infinite linear;-moz-animation:loadpspin 0s infinite linear;-ms-animation:loadpspin .00s infinite linear;animation:loadpspin 1s infinite linear;transform-origin:50% 50%}

Looks good
Many thanks @melloware

@melloware
Copy link
Member

Awesome thanks for testing!!!!

@melloware
Copy link
Member

I added a battery of unit tests for CSS compression and included your new test case as well!

https://github.com/primefaces-extensions/resources-optimizer-maven-plugin/blob/master/src/test/resources/transition.css

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🏆 workaround A workaround has been provided
Projects
None yet
Development

No branches or pull requests

3 participants