523355 - implement animation of stroke-dasharray

Status: RESOLVED FIXED

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

Description

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

Attachment: patch

Since I was thinking about animation of stroke-dasharray in bug 523193 last night, I went ahead and implemented it.

Does the SMIL code need to change to explicitly opt in to animating this property?  Or does it pick it up automatically?

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> Does the SMIL code need to change to explicitly opt in to animating this
> property?  Or does it pick it up automatically?

Just nsSMILCSSProperty::IsPropertyAnimatable, to indicate that it's now animatable.

nsSMILCSSValueType::Add() also needs to be tweaked to explicitly fail for stroke-dasharray, since it' an explicitly non-additive[1] property,  and since the "GetZeroValueForUnit" helper function doesn't handle eUnit_Dasharray.

[1] http://www.w3.org/TR/SVG/painting.html#StrokeDasharrayProperty

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Why do you need |list1 && list2| in the while condition if you've ensured the lengths are the same?

Comment 3

[Daniel Holbert [:dholbert]]

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

> PRBool
> nsStyleAnimation::ComputeDistance(const Value& aStartValue,
[SNIP]
>+        double diff;
>+        switch (val1.GetUnit()) {
>+          case eCSSUnit_Percent:
>+            diff = val1.GetPercentValue() - val2.GetPercentValue();
>+            break;
>+          case eCSSUnit_Number:
>+            diff = val1.GetFloatValue() - val2.GetFloatValue();
>+            break;
[SNIP]
>+        distance += diff * diff;

So, this chunk says a change from 10% to 40% is the same distance as a change from 10px to 40px, whereas in fact, this is generally not the case.

(ASIDE: I guess ComputeDistance already has a similar issue for standard lengths... but this issue can't actually cause problems in paced-mode SMIL animations yet, because we'll bail of animations that mix percents with non-percents.)

Anyway -- I think it's worth adding a FIXME note here that these two distances (Percent & Number) are on different scales, and that correctness would require that they be normalized so that their scales match.

>@@ -361,16 +429,74 @@ nsStyleAnimation::AddWeighted(double aCo
[SNIP]
>+        NS_ABORT_IF_FALSE(v1.GetUnit() == eCSSUnit_Number ||
>+                          v1.GetUnit() == eCSSUnit_Percent, "unexpected");
[SNIP]
>+        if (v1.GetUnit() == eCSSUnit_Number) {
[SNIP]
>+        } else {
>+          NS_ABORT_IF_FALSE(v1.GetUnit() == eCSSUnit_Percent, "unexpected");

That last NS_ABORT_IF_FALSE seems redundant, given that we've just recently asserted that it's a number or a percent.  (Not a big deal, though -- maybe it's just there for extra clarity?)

>@@ -743,16 +870,63 @@ nsStyleAnimation::ExtractComputedValue(n
[SNIP]
>+              switch (coord.GetUnit()) {
>+                case eStyleUnit_Coord:
>+                  // Number means the same thing as length; we want to
>+                  // animate them the same way.  Normalize both to number
>+                  // since it has more accuracy (float vs nscoord).
>+                  value.SetFloatValue(nsPresContext::
>+                    AppUnitsToFloatCSSPixels(coord.GetCoordValue()),
>+                    eCSSUnit_Number);
>+                  break;
>+                case eStyleUnit_Factor:
>+                  value.SetFloatValue(coord.GetFactorValue(),
>+                                      eCSSUnit_Number);
>+                  break;

No action items for this chunk, just a note:
Interesting -- I'd been assuming that length-valued properties always ended up with eStyleUnit_Coord-unit values in their computed style, but I see now that they can also end up as Float values, too (at least, for properties recognized by SVG -- not sure about others).

So anyway, we don't currently handle this correctly for simple length-valued properties -- I just filed Bug 523551 on that.

>@@ -743,16 +870,63 @@ nsStyleAnimation::ExtractComputedValue(n
>+              nsCSSValueList *item = new nsCSSValueList;
[SNIP]
>+              switch (coord.GetUnit()) {
[SNIP]
>+                default:
>+                  NS_ABORT_IF_FALSE(PR_FALSE, "unexpected unit");
>+                  return PR_FALSE;
>+              }
>+              *resultTail = item;

The 'default' switch case here will leak the memory allocated for |item|... We're probably not likely to hit that case, judging by the the NS_ABORT_IF_FALSE, but it might be worth using a nsAutoPtr for magical auto-cleanup. (Or alternately, you could just add an explicit "delete item;" to that case, before the return statement.)

>+        const nsCSSValue &val1 = list1->mValue;
>+        const nsCSSValue &val2 = list2->mValue;
[SNIP]
>+        const nsCSSValue& v1 = list1->mValue;
>+        const nsCSSValue& v2 = list2->mValue;

We should probably be consistent about placement of the "&" in declarations of references.

r=dholbert with the above addressed.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> So, this chunk says a change from 10% to 40% is the same distance as a change
> from 10px to 40px, whereas in fact, this is generally not the case.

Sorry -- I should clarify this point a bit.  What I'm saying is that this chunk added to ComputeDistance can technically cause incorrect behavior even before we support interpolation from percent to non-percent, in scenarios like the one below.

<SCENARIO>
  Suppose we're doing a SMIL animation of the stroke-dasharray property, with calcMode="paced", across the following sequence of values: "10px, 10%" // "40px, 10%" // "20px, 20%".  Since we're in paced mode, we'll call ComputeDistance to find the intermediate distances.  The first distance is 30, and the second distance is 10, for a total distance of 40.  So, we'll let the first interval occupy 3/4 of the duration, and we'll let the second interval occupy the final 1/4.  This is supposed to produce an "even pace of change".
  However, if the <svg> element is huge (say, 1000px x 1000px), then the 10% --> 20% change is *actually* a 100px difference -- much larger than the other 10px -> 40px change.  So in fact, the first interval should really take 3/13 of the duration, and the second should take the final 10/13.
</SCENARIO>

In any case -- I don't think it's worth trying to fix this particular issue in this patch -- hence the FIXME suggestion in comment 3. This can probably be fixed as part of bug 520234.

(I'm actually not 100% sure that stroke-dasharray is even supposed to support calcMode="paced".  The SVG spec says that paced mode is "only supported for values that define a linear numeric range".  I assume this includes multi-dimensional numeric values, like rgb() and rect()... and a 'stroke-dasharray' value is really just a multidimensional numeric value, too, so I imagine it's supposed to be supported.  Intuitively, I feel like anything that can be interpolated should be able to support paced-mode animation.)
http://www.w3.org/TR/SVG11/animate.html#CalcModeAttribute

Comment 5

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

Yeah, I was aware the distance measure was not particularly sensible; the invariant I've been trying to maintain (which I should probably document) is that distance between a starting point and the interpolation result is proportional to the portion given to Interpolate, since transitions depends on that.  Other than that, I haven't necessarily worried about keeping distances for different types of units on the same numeric scale because I don't know a reasonable way to do that.

>That last NS_ABORT_IF_FALSE seems redundant, given that we've just recently
>asserted that it's a number or a percent.  (Not a big deal, though -- maybe
>it's just there for extra clarity?)

Yeah, though it's a decent distance away, so I'd sort of like to keep it for clarity.

Comment 6

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

(In reply to comment #3)
> The 'default' switch case here will leak the memory allocated for |item|...
> We're probably not likely to hit that case, judging by the the
> NS_ABORT_IF_FALSE, but it might be worth using a nsAutoPtr for magical
> auto-cleanup. (Or alternately, you could just add an explicit "delete item;" to
> that case, before the return statement.)

And, actually, there's a simpler fix here:  link |item| into the list right after we allocate it, so that deleting |result| in that failure case will also delete |item|.

Comment 7

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

http://hg.mozilla.org/mozilla-central/rev/8fbbf4665e89