Invalid CSS raises error starting in 1.8.0 #125

JasonBarnabe · 2021-03-17T20:06:44Z

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)

The text was updated successfully, but these errors were encountered:

grosser · 2021-03-18T00:12:49Z

kinda like the new behavior more, but it would be nice if it raised a more descriptive error ...
/cc @ojab

dark-panda · 2021-04-15T21:56:53Z

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.

dark-panda · 2021-04-16T19:31:22Z

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.

grosser · 2021-04-17T16:52:52Z

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 ?

dark-panda · 2021-04-18T16:38:53Z

@grosser It never really worked, but it was often silently ignored until the commit in 8868aa6, 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.

ryanscottaudio · 2021-05-26T15:22:03Z

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?

ryanscottaudio · 2021-06-02T15:37:55Z

@grosser pinging on this. is there a way to pass something so that we can return to the pre-1.8.0 functionality?

grosser · 2021-06-02T16:07:59Z

removing the raise would do the trick ?

rikkipitt · 2021-09-11T10:53:40Z

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?

rikkipitt · 2021-09-11T13:08:25Z

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.

grosser · 2021-09-17T22:57:11Z

PR welcome ... would love a unified solution, but as long as the default is "what works for most ppl / is intuitive" 🤷

markedmondson · 2022-09-15T19:44:40Z

PR that allows option passing to mute the exception here: #132

grosser · 2022-09-16T00:26:02Z

1.12.0

grosser · 2022-09-16T00:26:35Z

allows setting rule_set_exceptions: false to ignore these exceptions

dark-panda mentioned this issue Apr 22, 2021

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

Merged

This was referenced Sep 15, 2022

Return the property value in an empty value rule set #131

Merged

Allow rule set argument exceptions to be muted #132

Merged

grosser closed this as completed Sep 16, 2022

JasonBarnabe mentioned this issue Sep 16, 2022

Support CSSParser option rule_set_exceptions fphilipe/premailer-rails#273

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invalid CSS raises error starting in 1.8.0 #125

Invalid CSS raises error starting in 1.8.0 #125

JasonBarnabe commented Mar 17, 2021

grosser commented Mar 18, 2021

dark-panda commented Apr 15, 2021

dark-panda commented Apr 16, 2021

grosser commented Apr 17, 2021

dark-panda commented Apr 18, 2021

ryanscottaudio commented May 26, 2021

ryanscottaudio commented Jun 2, 2021

grosser commented Jun 2, 2021

rikkipitt commented Sep 11, 2021

rikkipitt commented Sep 11, 2021

grosser commented Sep 17, 2021

markedmondson commented Sep 15, 2022 •

edited

grosser commented Sep 16, 2022

grosser commented Sep 16, 2022

Invalid CSS raises error starting in 1.8.0 #125

Invalid CSS raises error starting in 1.8.0 #125

Comments

JasonBarnabe commented Mar 17, 2021

grosser commented Mar 18, 2021

dark-panda commented Apr 15, 2021

dark-panda commented Apr 16, 2021

grosser commented Apr 17, 2021

dark-panda commented Apr 18, 2021

ryanscottaudio commented May 26, 2021

ryanscottaudio commented Jun 2, 2021

grosser commented Jun 2, 2021

rikkipitt commented Sep 11, 2021

rikkipitt commented Sep 11, 2021

grosser commented Sep 17, 2021

markedmondson commented Sep 15, 2022 • edited

grosser commented Sep 16, 2022

grosser commented Sep 16, 2022

markedmondson commented Sep 15, 2022 •

edited