premailer / css_parser

Ruby CSS Parser
Other
279 stars 110 forks source link

Allow CSS functions to be used when expanding dimensions shorthand #126

Closed dark-panda closed 3 years ago

dark-panda commented 3 years ago

Using CSS functions causes CssParser::RuleSet#expand_dimensions_shorthand! to raise an ArgumentError. What we attempt to do here is handle functions by using a temporary string replacement to sanely handle arguments like

margin: calc(1em / 4 * 2);
border: calc(var(--foo) / 2);
dark-panda commented 3 years ago

Copying this over from the unrelated issue in #125 so we have it here...

If the value is being split on whitespace using commas as parameter separators, then the case statement can be very misleading, and can raise an error when the size of the split array is not within 1-4 values, as in the case that I was running into, where the value is calc(1em / 4 * 4), or ['calc(1em', '/', '4', '*', '4)'] when split on the whitespace. This used to silently pass by since there was no raise before, and values would not get set, and the whole thing would basically become nil and ignored. You'd get nil values for everything in that case.

However, if you did calc(1em/4*4) where there is no whitespace, the values length would be 1, and you'd end up getting:

{
  "margin-left" => "calc(1em/4*4)",
  "margin-right" => "calc(1em/4*4)",
  "margin-top" => "calc(1em/4*4)",
  "margin-bottom" => "calc(1em/4*4)"
}

Which is correct, but only because of the lack of whitespace in the calc. If you include varying amounts of whitespace, the results change. This is what happens in the current master as well as v1.7.1:

"margin: calc(1em / 4)"
{
  "margin-top" => "calc(1em", 
  "margin-right" => "/", 
  "margin-bottom" => "4)", 
  "margin-left" => "/"
}

"margin: calc(1em /4)"
{
  "margin-top" => "calc(1em", 
  "margin-right" => "/4)", 
  "margin-bottom" => "calc(1em", 
  "margin-left" => "/4)"
}

"margin: calc(1em / 4 * 4)"
# in current master
ArgumentError (Cannot parse calc(1em / 4 * 4))

"margin: calc(1em / 4 * 4)"
# in v1.7.1:
{}

# With commas:
"margin: clamp(1rem, 2.5vw, 2rem)"
{
  "margin-top" => "clamp(1rem,", 
  "margin-right" => "2.5vw,", 
  "margin-bottom" => "2rem)", 
  "margin-left" => "2.5vw,"
}

With my PR, the expected values are expanded:

"margin: calc(1em / 4)"
{
  "margin-top" => "calc(1em / 4)",
  "margin-right" => "calc(1em / 4)",
  "margin-bottom" => "calc(1em / 4)",
  "margin-left" => "calc(1em / 4)"
}

"margin: calc(1em /4)"
{
  "margin-top" => "calc(1em /4)", 
  "margin-right" => "calc(1em /4)", 
  "margin-bottom" => "calc(1em /4)", 
  "margin-left" => "calc(1em /4)"
}

"margin: calc(1em / 4 * 4)"
{
  "margin-top" => "calc(1em / 4 * 4)", 
  "margin-right" => "calc(1em / 4 * 4)", 
  "margin-bottom" => "calc(1em / 4 * 4)", 
  "margin-left" => "calc(1em / 4 * 4)"
}

"margin: clamp(1rem, 2.5vw, 2rem)"
{
  "margin-top" => "clamp(1rem, 2.5vw, 2rem)", 
  "margin-right" => "clamp(1rem, 2.5vw, 2rem)", 
  "margin-bottom" => "clamp(1rem, 2.5vw, 2rem)", 
  "margin-left" => "clamp(1rem, 2.5vw, 2rem)"
}

So basically calc and similar functions were not working even before v1.8.0 at least under certain circumstances where whitespace is involved due to the split(/\s/), basically, they just didn't raise ArgumentErrors in some situations and in others produced odd expansions.

The WHITESPACE_SEPARATOR magic-ish constant that I included is so that we can still split on whitespace as before, but this time around we temporarily change the whitespace within CSS functions to a magic string that doesn't factor in to the split(/\s/). I picked something which would never reasonably be used in actual CSS ("___SPACE___", named similarly to Ruby's constants like __FILE__, __LINE__, __CLASS__, etc.) so we could avoid any name clashes within code. It just needs to be something that's non-whitespace and also wouldn't ever really appear in any CSS.

dark-panda commented 3 years ago

Just dropping in to check in on this PR. Any thoughts on this?

grosser commented 3 years ago

thx! 1.10.0