Closed Bug 524539 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
Summary: SVG/SMIL: Enable support for "stroke-dasharray" → SVG/SMIL: Enable support for animating "stroke-dasharray" CSS property
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.)
Attachment #408475 - Flags: review?(dbaron)
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
Attachment #408484 - Attachment description: patch 2: enable SMIL animation (but not addition) of stroke-dasharray → patch 2: enable SMIL animation (but disallow addition) of stroke-dasharray
Attachment #408484 - Flags: review?(roc)
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.
Attachment #408475 - Flags: review?(dbaron) → review-
(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.
Attachment #408475 - Attachment is obsolete: true
Attachment #408476 - Attachment is obsolete: true
Attachment #413216 - Flags: review?(dbaron)
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
Attachment #413216 - Flags: review?(dbaron) → review+
(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. :)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
You need to log in before you can comment on or make changes to this bug.