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(colors): convert rgba/hsla to rgb/hsl if opacity is >= 1 #1083

Merged
merged 1 commit into from Nov 12, 2019

Conversation

clshortfuse
Copy link
Contributor

fixes #1082

Fixes:

  • rgba(R,G,B,1) => rgb(R,G,B)
  • rgb(R,G,B,1) => rgb(R,G,B)
  • hsla(H,S,L,1) => hsl(H,S,L)
  • hsl(H,S,L,1) => hsl(H,S,L)
  • Removes case-sensitivity for rgb, rgba, hsl, and hsla

For reference, CSS 4 supports alpha parameter for rgb() and hsl(). That's why the new regex is also optionally looking for a. If it finds it and opacity is 1, then it effectively removes the alpha parameter. That's a plus for backwards compatibility for browsers that don't support rgb(R G B A).

https://developer.mozilla.org/en-US/docs/Web/CSS/color_value

function optimizeColors(name, value, compatibility) {
if (value.indexOf('#') === -1 && value.indexOf('rgb') == -1 && value.indexOf('hsl') == -1) {
if (!value.match(/#|rgb|hsl/gi)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this change may lead to a slight performance degradation, but let's merge it and fix if needed.

Copy link
Collaborator

@jakubpawlowicz jakubpawlowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good otherwise!

@jakubpawlowicz jakubpawlowicz merged commit 58e5ff8 into clean-css:master Nov 12, 2019
@clshortfuse
Copy link
Contributor Author

I haven't looked at this PR in a bit, but the Regex may be incorrect and could cause issues.

The proper Regex for numbers is /[+-]?((\d+)|(\d*\.\d+))(e[+-]?\d+)?/. That covers all the requirements in the CSS spec.

I made a crazy long Regex that can verify CSS colors at https://gist.github.com/clshortfuse/acffca0a5f1412edcf9e132f33febe1c

Implementing one based along that may also fix other issues in clean-css that we aren't aware about (possibly supporting complex values like rgb( 1.2e2, 5e-4, +2).

@clshortfuse
Copy link
Contributor Author

After closer inspection, the regex is unlikely to cause issues, but could be better. I'm making a PR to support scientific notation as well RGB(% % %). But it's not as urgent as I previously thought.

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

Successfully merging this pull request may close these issues.

optimizeColors: rgba|hsla with opacity >=1.0 should change to rgb|hsl
2 participants