properties parsed with CSSParserImpl::ParseBoxProperties incorrectly accept values with errors inside functions

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bgrins, Assigned: heycam)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

()

Attachments

(3 attachments, 3 obsolete attachments)

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
Summary: CSS variable used with calc() used in shorthand css property doesn't work → CSS variable used with calc() in shorthand css property doesn't work
Cam, am I missing something or is this an issue with calc and/or css variables?
Flags: needinfo?(cam)
Posted file testcase
(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.
Flags: needinfo?(cam)
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().)
Blocks: 585715
Looks like there was:

  <div style="border: 8px solid green; border-color: red rgb(nonsense)">hello</div>

shows a red border.
Summary: CSS variable used with calc() in shorthand css property doesn't work → properties parsed with CSSParserImpl::ParseBoxProperties incorrectly accept values with errors inside functions
This also applies to ParseGroupedBoxProperty but none of the properties parsed with that accept calc() values at the moment.
Posted patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8523596 - Flags: review?(dbaron)
Posted patch patch v2 (obsolete) — Splinter Review
Update test expectations.
Attachment #8523629 - Flags: review?(dbaron)
Attachment #8523596 - Attachment is obsolete: true
Attachment #8523596 - Flags: review?(dbaron)
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.
Attachment #8523629 - Flags: review?(dbaron) → review-
Posted patch patch v3Splinter Review
(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.
Attachment #8523629 - Attachment is obsolete: true
Attachment #8625597 - Flags: review?(dbaron)
Posted file border-image-variable.html (obsolete) —
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.
Flags: needinfo?(cam)
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.
Flags: needinfo?(cam)
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
Attachment #8627962 - Attachment is obsolete: true
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
Attachment #8625597 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/da921ebcc14f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.