css_parser
css_parser copied to clipboard
premailer
Invalid CSS raises error starting in 1.8.0
Not sure what the general strategy is in this gem for handling invalid CSS, but the behaviour changed between 1.7.1 and 1.8.0.
html = <<~HTML
<!DOCTYPE html>
<html>
<head>
<style type="text/css">
a {
line-height: !important;
}
</style>
</head>
<body>
</body>
</html>
HTML
parser = CssParser::Parser.new
parser.load_string! html
puts parser.to_h
In 1.7.1:
{"all"=>{"<!DOCTYPE html> <html> <head> <style type=\"text/css\"> a"=>{"line-height"=>"!important"}}}
In 1.8.0:
15: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/parser.rb:494:in `load_string!'
14: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/parser.rb:161:in `add_block!'
13: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/parser.rb:329:in `parse_block_into_rule_sets!'
12: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/parser.rb:329:in `scan'
11: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/parser.rb:361:in `block in parse_block_into_rule_sets!'
10: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/parser.rb:168:in `add_rule!'
9: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/parser.rb:168:in `new'
8: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:243:in `initialize'
7: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:606:in `parse_declarations!'
6: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:606:in `each'
5: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:614:in `block in parse_declarations!'
4: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/2.6.0/forwardable.rb:230:in `add_declaration!'
3: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:99:in `[]='
2: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:99:in `new'
1: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:33:in `initialize'
/home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:41:in `value=': value is empty (ArgumentError)
kinda like the new behavior more, but it would be nice if it raised a more descriptive error ... /cc @ojab
We're having a similar issue where the upgrade from 1.7.1 to 1.9.0 has started raising exceptions like the following:
ArgumentError - Cannot parse calc(1em * 8 / 4) auto 0
I haven't been able to reduce this to a minimal example but I'll report back when I get one.
I've pushed a PR that fixes the issue I had in #126. I believe these issues aren't related so perhaps I ought to open up a new issue, but when I commented yesterday I thought it might be an overall-sort-of-parsing issue before I actually decided to try and fix the issue. At any rate, the PR fixes the calc issue I ran into.
if calc worked before then something went wrong during refactoring and we should be able to undo that ? ... have you tried a git-bisect with your testcase to pinpoint the bad commit ?
@grosser It never really worked, but it was often silently ignored until the commit in 8868aa69eb4369161a89765df11ab2537915c968, specifically with this code:
https://github.com/premailer/css_parser/blob/master/lib/css_parser/rule_set.rb#L373
The issue being 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.
Can we return to the question at hand? Or is there a way to go back to pre-1.8.0 functionality (i.e. letting invalid CSS pass) using an argument or something?
@grosser pinging on this. is there a way to pass something so that we can return to the pre-1.8.0 functionality?
Hey folks, I'm experiencing something similar where emails contain invalid margin or padding (e.g. 5 values instead of 4, 3, 2 or 1).
ArgumentError: Cannot parse 10px 10px 10px 5px 10px
In my case, it would be good to ignore these invalid styles too. Is the current workaround to use a pre-1.8.0 version of this gem?
I can confirm that in my case, 1.7.1 works without the raise. @grosser would it be possible to feature flag this if people want to turn it off? Appreciate your work on this, cheers.
PR welcome ... would love a unified solution, but as long as the default is "what works for most ppl / is intuitive" 🤷
PR that allows option passing to mute the exception here: https://github.com/premailer/css_parser/pull/132
