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

@dark-panda dark-panda commented Apr 16, 2021

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 dark-panda force-pushed the fix-functions branch 2 times, most recently from 133d2e1 to 2be107c Apr 16, 2021
Copy link
Contributor

@grosser grosser left a comment

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

@dark-panda dark-panda commented Apr 22, 2021

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

@dark-panda dark-panda commented Jul 19, 2021

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

value.gsub!(RE_FUNCTIONS) { |c| c.gsub(/\s+/, WHITESPACE_REPLACEMENT) }

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

@grosser grosser Jul 25, 2021

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

@dark-panda dark-panda Jul 26, 2021

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

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

@grosser grosser Jul 26, 2021

we don't need this then right ?

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

matches
Copy link
Contributor

@grosser grosser Jul 26, 2021

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
1 of 7 checks passed
@grosser
Copy link
Contributor

@grosser 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants