524539 - SVG/SMIL: Enable support for animating "stroke-dasharray" CSS property

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Bug 523355 added "stroke-dasharray" support to nsStyleAnimation.  I'm filing this bug on using that for SVG/SMIL.

Bug 523355 comment 1 essentially covers what's needed.  We also need some minor style-system tweaks to support "none" stroke-dasharray values in nsStyleAnimation::UncomputeStyle.

Patch coming up soon.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: patch 1: fix nsStyleAnimation::UncomputeValue to work with 'none' stroke-dasharray values

Currently, if we call the void* version of UncomputeValue on a "none" value of stroke-dasharray, we abort -- specifically, we fall into the eUnit_None case, which assumes our type is either PaintServer (which it's not, so we skip that clause) or Single Value (which it's not, so we abort when we fall into that clause).

This patch extends the eUnit_None case there to allow "eStyleAnimType_Custom" and in particular stroke-dasharray, which it uncomputes to a null pointer.

This patch also extends nsCSSDeclaration::AppendStorageToString to accept null ValueLists, and treat them simply as "none" (which they represent).  (Currently, a null value-list makes that function crash when we try to evaluate "val->mValue" -- where 'val' is a null pointer.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch 1, "diff -w" version

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: patch 2: enable SMIL animation (but disallow addition) of stroke-dasharray

This just adds the minimal required code on the SMIL side, plus tests.  (It also does some minimal cleanup in IsPropertyAnimatable to remove the type-based grouping of properties, for the property-types that we now support.)

This is just the stuff I mentioned in bug 523355 comment 1 -- quoting that comment here for convenience:
> (In reply to bug 523355 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 4

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

Comment on attachment 408475 [details] [diff] [review]
patch 1: fix nsStyleAnimation::UncomputeValue to work with 'none' stroke-dasharray values

I don't want to use different representation if the property comes from UncomputeValue vs if it comes from the parser; we should have consistent representations.

I think the easiest way to achieve that is to drop your nsCSSDeclaration changes and make your nsStyleAnimation changes make a one-item value list with the value being none, just like the parser does.  That's probably best, although making none always be represented by a null value list would also be acceptable (although the change in nsCSSDeclaration that assumes that 'none' is the right keyword for any null value list bothers me a little).

Sorry for the delay in getting to this.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: patch 1v2: use single-value ValueList for "none"

(In reply to comment #4)
> make your nsStyleAnimation changes make a one-item value list with
> the value being none, just like the parser does.  That's probably best,

Done in this patch.  This changes ExtractComputedValue to create a one-item list with value "none".  It also changes AddWeighted to explicitly fail if it encounters a "none" value.

Comment 6

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

Comment on attachment 413216 [details] [diff] [review]
patch 1v2: use single-value ValueList for "none"

>diff --git a/layout/style/nsStyleAnimation.cpp b/layout/style/nsStyleAnimation.cpp
>--- a/layout/style/nsStyleAnimation.cpp
>+++ b/layout/style/nsStyleAnimation.cpp
>@@ -591,16 +591,26 @@ nsStyleAnimation::AddWeighted(nsCSSPrope
>       PRUint32 len1 = 0, len2 = 0;
>       for (const nsCSSValueList *v = list1; v; v = v->mNext) {
>         ++len1;
>       }
>       for (const nsCSSValueList *v = list2; v; v = v->mNext) {
>         ++len2;
>       }
>       NS_ABORT_IF_FALSE(len1 > 0 && len2 > 0, "unexpected length");
>+      NS_ABORT_IF_FALSE(list1 && list2, "unexpected null value");

This isn't needed; the previous assertion already covers it.

>+      if ((list1 && list1->mValue.GetUnit() == eCSSUnit_None) ||
>+          (list2 && list2->mValue.GetUnit() == eCSSUnit_None)) {

No need to null-check list1 or list2 here.

>+        // One of our values is "none".  Can't do addition with that.
>+        NS_ABORT_IF_FALSE(
>+          (!list1->mValue.GetUnit() != eCSSUnit_None || len1 == 1) &&
>+          (!list2->mValue.GetUnit() != eCSSUnit_None || len2 == 1),
>+          "multi-value valuelist with 'none' as first element");

You need to drop the first ! on both of these lines.

r=dbaron with that

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #6)
> >+        // One of our values is "none".  Can't do addition with that.
> >+        NS_ABORT_IF_FALSE(
> >+          (!list1->mValue.GetUnit() != eCSSUnit_None || len1 == 1) &&
> >+          (!list2->mValue.GetUnit() != eCSSUnit_None || len2 == 1),
> >+          "multi-value valuelist with 'none' as first element");
> 
> You need to drop the first ! on both of these lines.

Thanks, good catch!  That was a typo from converting !(foo && bar) into (!foo || !bar)...  I guess that just ended up testing "0 != eCSSUnit_None", which is trivially true. :)

Comment 8

[Daniel Holbert [:dholbert]]

Addressed changes in comment 6:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/551f9fd81008

Comment 9

[Daniel Holbert [:dholbert]]

Landed:
http://hg.mozilla.org/mozilla-central/rev/1c1825d6960d
http://hg.mozilla.org/mozilla-central/rev/065bc628734b