Last Comment Bug 653842 - [css3-transitions][css3-animations] disallow interpolation out of property's range
: [css3-transitions][css3-animations] disallow interpolation out of property's ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla6
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
:
Mentors:
Depends on:
Blocks: 575672
  Show dependency treegraph
 
Reported: 2011-04-29 14:52 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2011-05-09 12:19 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1: list more range restrictions in nsCSSPropList.h (8.20 KB, patch)
2011-05-06 19:07 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
Details | Diff | Review
patch 2: merge more-than-zero into one-or-more (8.45 KB, patch)
2011-05-06 19:07 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
Details | Diff | Review
patch 3: honor range restrictions when interpolating (54.47 KB, patch)
2011-05-06 19:08 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
Details | Diff | Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-29 14:52:49 PDT
Since bug 575672 (though it was allowed in opt builds before that, incorrectly), we've allowed cubic-bezier() timing functions for transitions (and now animations) that have result values outside the 0-1 range.  This means that transitions or animations could cause values outside of the allowed range for the property, which we only check at parse time (and when we compute calc()).

nsStyleAnimation should enforce the range restrictions on properties.
Comment 1 Jesse Ruderman 2011-05-04 10:21:56 PDT
Should invalid values that result from cubic-bezier() be clamped or ignored?
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-04 11:12:19 PDT
clamped.  I have patches for this mostly written in my tree.
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-06 19:07:14 PDT
Created attachment 530789 [details] [diff] [review]
patch 1: list more range restrictions in nsCSSPropList.h
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-06 19:07:44 PDT
Created attachment 530790 [details] [diff] [review]
patch 2: merge more-than-zero into one-or-more
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-06 19:08:13 PDT
Created attachment 530791 [details] [diff] [review]
patch 3: honor range restrictions when interpolating
Comment 6 Boris Zbarsky [:bz] 2011-05-06 19:26:12 PDT
Comment on attachment 530789 [details] [diff] [review]
patch 1: list more range restrictions in nsCSSPropList.h

This seems fine.  We need to escalate the font-size-adjust thing to the spec, right?

I don't really have any useful thoughts on the MathML props.
Comment 7 Boris Zbarsky [:bz] 2011-05-06 19:30:58 PDT
Comment on attachment 530790 [details] [diff] [review]
patch 2: merge more-than-zero into one-or-more

r=me
Comment 8 Boris Zbarsky [:bz] 2011-05-06 20:05:25 PDT
Comment on attachment 530791 [details] [diff] [review]
patch 3: honor range restrictions when interpolating

>+    // TODO: Up to here...

That should go away.

r=me with that.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-08 08:07:18 PDT
(In reply to comment #6)
> This seems fine.  We need to escalate the font-size-adjust thing to the
> spec, right?

I already did: http://lists.w3.org/Archives/Public/www-style/2011Apr/0812.html
Comment 10 :Ms2ger 2011-05-09 12:10:03 PDT
Comment on attachment 530790 [details] [diff] [review]
patch 2: merge more-than-zero into one-or-more

>diff --git a/layout/style/nsCSSProps.h b/layout/style/nsCSSProps.h
>--- a/layout/style/nsCSSProps.h
>+++ b/layout/style/nsCSSProps.h
>@@ -108,21 +108,18 @@
> PR_STATIC_ASSERT((CSS_PROPERTY_PARSE_PROPERTY_MASK &
>                   CSS_PROPERTY_VALUE_PARSER_FUNCTION) == 0);
> 
> #define CSS_PROPERTY_VALUE_RESTRICTION_MASK       (3<<13)
> // The parser (in particular, CSSParserImpl::ParseSingleValueProperty)
> // should enforce that the value of this property must be 0 or larger.
> #define CSS_PROPERTY_VALUE_NONNEGATIVE            (1<<13)
> // The parser (in particular, CSSParserImpl::ParseSingleValueProperty)
>-// should enforce that the value of this property must be greater than 0.
>-#define CSS_PROPERTY_VALUE_POSITIVE_NONZERO       (2<<13)
>-// The parser (in particular, CSSParserImpl::ParseSingleValueProperty)
> // should enforce that the value of this property must be 1 or larger.
>-#define CSS_PROPERTY_VALUE_AT_LEAST_ONE           (3<<13)
>+#define CSS_PROPERTY_VALUE_AT_LEAST_ONE           (2<<13)
> 
> // NOTE: next free bit is (1<<15)

Is this comment still right?
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-09 12:19:11 PDT
https://hg.mozilla.org/mozilla-central/rev/5752142febe7
https://hg.mozilla.org/mozilla-central/rev/c2613933f08b
https://hg.mozilla.org/mozilla-central/rev/e5fc7dbde8be

My current inclination is not to try to get this in to Firefox 5.

Note You need to log in before you can comment on or make changes to this bug.