[css3-transitions][css3-animations] disallow interpolation out of property's range

RESOLVED FIXED in mozilla6

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

6 years ago
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

6 years ago
Should invalid values that result from cubic-bezier() be clamped or ignored?
(Assignee)

Comment 2

6 years ago
clamped.  I have patches for this mostly written in my tree.
(Assignee)

Comment 3

6 years ago
Created attachment 530789 [details] [diff] [review]
patch 1: list more range restrictions in nsCSSPropList.h
Attachment #530789 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

6 years ago
Created attachment 530790 [details] [diff] [review]
patch 2: merge more-than-zero into one-or-more
Attachment #530790 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

6 years ago
Created attachment 530791 [details] [diff] [review]
patch 3: honor range restrictions when interpolating
Attachment #530791 - Flags: review?(bzbarsky)
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.
Attachment #530789 - Flags: review?(bzbarsky) → review+
Comment on attachment 530790 [details] [diff] [review]
patch 2: merge more-than-zero into one-or-more

r=me
Attachment #530790 - Flags: review?(bzbarsky) → review+
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.
Attachment #530791 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 9

6 years ago
(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 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?
(Assignee)

Comment 11

6 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Priority: -- → P3
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
You need to log in before you can comment on or make changes to this bug.