I was trying to set a shorthand margin property using a css variable and bumped into something that looks like a bug with variables used inside of calc(). Test case here: http://jsbin.com/livafoxama/2/edit?html,css,output <div class="foo"> foo </div> <div class="bar"> bar </div> <style> :root { --pad: 40px; } .foo { padding: 0 calc(0px + var(--pad)); } .bar { padding-left: calc(0px + var(--pad)); padding-right: calc(0px + var(--pad)); } </style> Notice that the bar element is padded on the sides, but the foo element isn't

Cam, am I missing something or is this an issue with calc and/or css variables?

(without all the jsbin gunk) Interestingly, it works without the "0 " at the beginning of the padding value.

An issue with CSS variables. Given it works without the "0 ", I wonder if we're accidentally parsing the stored token stream as one of the component longhand properties, rather than parsing it as the shorthand and then getting the corresponding longhand property that was set.

Actually, turns out there is an underlying problem with parsing shorthand properties like 'padding' that use ParseBoxProperties. Here's a test that doesn't use variables. Notice that padding is applied even though the property should have failed to parse. ParseBoxProperties calls ParseSingleValueProperty which will end up calling ParseVariant for each of the four non-negative values it expects. ParseVariant, when encountering a "calc(" with a syntax error inside it, will SkipUtil(')') and return false. But ParseBoxProperties just uses the false return value of ParseSingleValueProperty to break the NS_FOR_CSS_SIDES loop, and seeing that we have one valid value, will return true. Then, back up in ParseProperty (which called ParsePropertyByFunction for 'padding') it calls ExpectEndProperty, which returns true because we're now at the ';'.

I guess ParseBoxProperties needs to distinguish between ParseSingleValueProperty consuming something and failing versus consuming nothing and failing. (I guess prior to calc() there wasn't a way for it to consume something and fail for the properties that ParseBoxProperties uses, so this is a regression from implementing calc().)

Looks like there was: <div style="border: 8px solid green; border-color: red rgb(nonsense)">hello</div> shows a red border.

This also applies to ParseGroupedBoxProperty but none of the properties parsed with that accept calc() values at the moment.

I fixed ParseGroupedBoxProperty too and tested locally by allowing calc() values in border-image-width and border-image-outset. I put invalid values using calc() in property_database.js for when we end up supporting calc() in these properties.

Update test expectations.

Comment on attachment 8523629 [details] [diff] [review] patch v2 Instead of writing all this new code to see if we consumed tokens, why not just use GetNextTokenLocation both before and after the call to ParseSingleValueProperty or ParseNonNegativeVariant and see if they're different? Also, maybe add some rgb(nonsense) to the tests for properties that take colors, including both box properties and others? Sorry again for the delay getting to this.

(In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from comment #11) > Comment on attachment 8523629 [details] [diff] [review] > patch v2 > > Instead of writing all this new code to see if we consumed tokens, why > not just use GetNextTokenLocation both before and after the call to > ParseSingleValueProperty or ParseNonNegativeVariant and see if they're > different? Yeah, that works.

Just bumped into a similar problem in Bug 1049012, and I'm curious if this fix will cover it or if I should file a new bug? See the attached testcase, bug basically vars don't seem to work in border-image properties.

Seems more like a problem in linear-gradients rather than border-image.

(In reply to Brian Grinstead [:bgrins] from comment #14) > Just bumped into a similar problem in Bug 1049012, and I'm curious if this > fix will cover it or if I should file a new bug? See the attached testcase, > bug basically vars don't seem to work in border-image properties. No it doesn't seem to help with this. Please do file a new bug.

Comment on attachment 8627962 [details] border-image-variable.html (In reply to Cameron McCormack (:heycam) from comment #16) > (In reply to Brian Grinstead [:bgrins] from comment #14) > > Just bumped into a similar problem in Bug 1049012, and I'm curious if this > > fix will cover it or if I should file a new bug? See the attached testcase, > > bug basically vars don't seem to work in border-image properties. > > No it doesn't seem to help with this. Please do file a new bug. Alright, filed Bug 1179078

Comment on attachment 8625597 [details] [diff] [review] patch v3 I'd prefer if CSSParserImpl::ParseBoxPropertyVariant set aConsumedTokens to true when returning true, rather than setting aConsumedTokens to false in that case. (It shouldn't need to check the token location.) CSSParserImpl::ParseBoxProperty should also assert that !nsCSSProps::PropHasFlags(aPropID, CSS_PROPERTY_VALUE_PARSER_FUNCTION) r=dbaron with that