520488 - Extend nsStyleAnimation to support CSS rect() values

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Daniel Holbert [:dholbert]]

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)")

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just realized SMIL will also need Auto support in UncomputeValue, but that can come in a later patch...

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 409467 [details] [diff] [review]
patch

>@@ -543,16 +586,55 @@ nsStyleAnimation::AddWeighted(double aCo
[SNIP]
>+      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 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That depends whether we support calc(0.6 * auto + 0.4 * 40px) :-)

Comment 5

[Daniel Holbert [:dholbert]]

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

[Daniel Holbert [:dholbert]]

Comment on attachment 409467 [details] [diff] [review]
patch

>@@ -543,16 +586,55 @@ nsStyleAnimation::AddWeighted(double aCo
>+    case eUnit_CSSRect: {
[snip]
>+        switch ((rect1->*member).GetUnit()) {
[snip]
>+          case eCSSUnit_Auto:
>+            (result->*member).SetAutoValue();
>+            break;
[snip]
>+      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"
           from="none"
           by="none">
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."
http://www.w3.org/TR/SVG11/animate.html#AnimationAttributesAndProperties

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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

[Daniel Holbert [:dholbert]]

That makes sense to me.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Revised, per the previous comments, to only support AddWeighted on auto components of a rect when the coefficients sum to 1.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 414898 [details] [diff] [review]
patch

r=bzbarsky.  Sorry for the lag...

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/bfe4de8f0428