Extend nsStyleAnimation to support CSS rect() values

RESOLVED FIXED in mozilla1.9.3a1



10 years ago
9 years ago


(Reporter: dholbert, Assigned: dbaron)


(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



10 years ago
nsStyleAnimation (and probably nsStyleCoord?) need to be extended to support CSS rect() values. (e.g. for the clip property, which takes values like "rect(1px, 2px, 3px, 4px)")
I just realized SMIL will also need Auto support in UncomputeValue, but that can come in a later patch...

Comment 3

9 years ago
Comment on attachment 409467 [details] [diff] [review]

>@@ -543,16 +586,55 @@ nsStyleAnimation::AddWeighted(double aCo
>+      if (rect1->mTop.GetUnit() != rect2->mTop.GetUnit() ||
>+          rect1->mRight.GetUnit() != rect2->mRight.GetUnit() ||
>+          rect1->mBottom.GetUnit() != rect2->mBottom.GetUnit() ||
>+          rect1->mLeft.GetUnit() != rect2->mLeft.GetUnit()) {
>+        // At least until we have calc()
>+        return PR_FALSE;
>+      }

I'm not sure that comment is correct.

In other cases, unit mismatches mean we have <length> vs <percent>, which indeed can be fixed using e.g. 
>  calc(coeff1 * lengthVal + coeff2 * pctVal)

However, in the case of rect() units (at least for 'clip'), I think the mismatch would have to be <length> vs "auto", and calc() won't get us anywhere with that, right?

Comment 5

9 years ago
Right - if that's a possibility, then disregard comment 3. :)  I just wasn't sure whether "calc(0.6 * auto)" was meaningful.  I guess it could be, in principle.

Comment 6

9 years ago
Comment on attachment 409467 [details] [diff] [review]

>@@ -543,16 +586,55 @@ nsStyleAnimation::AddWeighted(double aCo
>+    case eUnit_CSSRect: {
>+        switch ((rect1->*member).GetUnit()) {
>+          case eCSSUnit_Auto:
>+            (result->*member).SetAutoValue();
>+            break;
>+      aResultValue.SetAndAdoptCSSRectValue(result.forget(), eUnit_CSSRect);
>+      break;

So, the above code lets us successfully add "auto" subcomponent-values to each other (and produce "auto").

This means that the following two SMIL animations will both succeed (and will affect the computed style of "clip" for their duration):
  <animate attributeName="clip" begin="0" dur="1"
           from="rect(10px, 10px, 10px, auto)"
           by="rect(5px, 5px, 5px, auto)" 
  <animate attributeName="clip"  begin="0" dur="1"
           from="rect(auto, auto, auto, auto)"
           by="rect(auto, auto, auto, auto)"

This outcome goes against what I was expecting -- I was assuming that "by-animations" that involve "auto" should fail. For comparison, consider this similar example with the "fill" property:
  <animate attributeName="fill"  begin="0" dur="1"
This "none + none" animation will fail (and hence won't affect the computed style of "fill").  This makes intuitive sense, because it's not sensible to add "none" -- not even to itself.[1]

Despite that, are we saying that it's sensible to add "auto" to itself?

[1] To be fair, in the case of fill:none, the spec also explicitly says "Only RGB color values are additive."


9 years ago
Blocks: 530983
(In reply to comment #6)
> So, the above code lets us successfully add "auto" subcomponent-values to each
> other (and produce "auto").

Hmmm.  So I guess I want to check that the weights add up to 1 before doing that, and fail if they don't.

Comment 8

9 years ago
That makes sense to me.
Created attachment 414898 [details] [diff] [review]

Revised, per the previous comments, to only support AddWeighted on auto components of a rect when the coefficients sum to 1.
Attachment #409467 - Attachment is obsolete: true
Attachment #414898 - Flags: review?(bzbarsky)
Attachment #409467 - Flags: review?(bzbarsky)
Comment on attachment 414898 [details] [diff] [review]

r=bzbarsky.  Sorry for the lag...
Attachment #414898 - Flags: review?(bzbarsky) → review+
Last Resolved: 9 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.