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

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

Merged
merged 2 commits into from Jul 27, 2021

Conversation

dark-panda
Copy link
Contributor

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);

Copy link
Contributor

@grosser grosser left a comment

Choose a reason for hiding this comment

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

not a fan of adding 2 replacements for every property to cover an edge-case :(
... also the 2 replacements depending on a magic var is not great either

I was kinda hoping it would go

'margin: ; margin-left: auto; margin-right: auto;'
->  {margin: 'calc(1em / 4 * var(--foo))', 'margin-left': 'auto'}
-> {margin-right: 'calc(1em / 4 * var(--foo))', 'margin-left': 'auto'}

without caring for what exactly is in the expression

@dark-panda
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Comment on lines 362 to 364
value.gsub!(RE_FUNCTIONS) { |c| c.gsub(/\s+/, WHITESPACE_REPLACEMENT) }

matches = value.strip.split(/\s+/)
Copy link
Contributor

Choose a reason for hiding this comment

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

would this be better ?

  • keep the replacement close together
  • replace less if all have the same values
  • do not produce extra hash and string objects
Suggested change
value.gsub!(RE_FUNCTIONS) { |c| c.gsub(/\s+/, WHITESPACE_REPLACEMENT) }
matches = value.strip.split(/\s+/)
value.gsub!(RE_FUNCTIONS) { |c| c.gsub!(/\s+/, WHITESPACE_REPLACEMENT); c }
matches = value.strip.split(/\s+/)
matches.each { |c| c.gsub!(WHITESPACE_REPLACEMENT, ' '); c }

... and ideally wrap this up in a preserve_whitespace_in_functions method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New version pushed, using #split_value_preserving_function_whitespace if that name is okay. How's this look?

Comment on lines 381 to 383
replacement = {top => t, right => r, bottom => b, left => l}.transform_values do |replacement_value|
replacement_value.gsub(WHITESPACE_REPLACEMENT, ' ')
end
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need this then right ?

Comment on lines 651 to 655
matches.each do |c|
c.gsub!(WHITESPACE_REPLACEMENT, ' ')
end

matches
Copy link
Contributor

Choose a reason for hiding this comment

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

each already returns matches

Suggested change
matches.each do |c|
c.gsub!(WHITESPACE_REPLACEMENT, ' ')
end
matches
matches.each do |c|
c.gsub!(WHITESPACE_REPLACEMENT, ' ')
end

@grosser grosser merged commit 23a8f8a into premailer:master Jul 27, 2021
@grosser
Copy link
Contributor

grosser commented Jul 27, 2021

thx! 1.10.0

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.

None yet

2 participants