589436 - Add support for SMIL animation of string attributes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

We should add support for SMIL animation of string attributes. These are:

  'class'
  'href' on <a>, <image>, <pattern>, <filter> etc. etc.
  'in1', 'in2' and 'result' on filter primitive elements
  'target' on <a>

Comment 1

[Jonathan Watt [:jwatt]]

Note that adding support for 'xlink:href' on <a> involves supporting QNames in the SMIL 'attributeName' attribute.

Comment 3

[Robert Longson [:longsonr]]

Attachment: part 1 - const

I need to call SplitQName from a const method.

Comment 4

[Robert Longson [:longsonr]]

Attachment: part 2 - string

Comment 5

[Robert Longson [:longsonr]]

Attachment: part 3 - tests

Comment 6

[Robert Longson [:longsonr]]

will ask for roc sr for part 2 eventually.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 471254 [details] [diff] [review]
part 1 - const

>diff --git a/content/base/public/nsIContent.h b/content/base/public/nsIContent.h
>-  virtual nsISMILAttr* GetAnimatedAttr(nsIAtom* aName) = 0;
>+  virtual nsISMILAttr* GetAnimatedAttr(PRInt32 aNamespaceID, nsIAtom* aName) = 0;

As long as you're touching this line in a patch that's largely about cleaning up const-ness, would you mind making |nsIAtom* aName| const, too?   (From glancing at nsSVGElement::GetAnimatedAttr, I don't think that should break anything)

I think that should just be a matter of adding 'const' on these 5 lines:
http://mxr.mozilla.org/mozilla-central/search?string=GetAnimatedAttr%28ns

Comment 8

[Daniel Holbert [:dholbert]]

Oh, nevermind about comment 7 - it looks like nsSVGElement::GetAnimatedAttr creates an nsAtomString, which needs a non-const nsIAtom.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 471254 [details] [diff] [review]
part 1 - const

r=jst, but the change to GetAnimatedAttr() doesn't seem to belong in this patch.

Comment 10

[Robert Longson [:longsonr]]

(In reply to comment #9)
> r=jst, but the change to GetAnimatedAttr() doesn't seem to belong in this
> patch.

Oops, that should be in the part 2 patch instead.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 471256 [details] [diff] [review]
part 2 - string

>diff --git a/content/smil/SMILStringType.cpp b/content/smil/SMILStringType.cpp
>+SMILStringType::Init(nsSMILValue& aValue) const
>+{
>+  NS_PRECONDITION(aValue.IsNull(), "Unexpected value type");
>+  aValue.mU.mPtr = new nsString();
[...]
>+SMILStringType::Destroy(nsSMILValue& aValue) const
>+{
>+  NS_PRECONDITION(aValue.mType == this, "Unexpected SMIL value");
>+  delete static_cast<nsAString*>(aValue.mU.mPtr);

Perhaps it'd be cleaner for the static_cast for |delete| to match the type used for |new|? (nsString, in this case)

I guess virtual inheritance of destructors should keep this from actually causing problems, but as long as the |new| and |delete| calls are only a few lines apart in the code, it seems like it's more future-proof for them to use the same type.  I don't have strong feelings on this, though - up to you.

>diff --git a/content/smil/nsSMILTargetIdentifier.h b/content/smil/nsSMILTargetIdentifier.h
>   inline PRBool Equals(const nsSMILTargetIdentifier& aOther) const
>   {
>     return (aOther.mElement       == mElement &&
>             aOther.mAttributeName == mAttributeName &&
>+            aOther.mAttributeNamespaceID == mAttributeNamespaceID &&
>             aOther.mIsCSS         == mIsCSS);

Add whitespace to the contextual lines so that the "==" characters continue to line up.

>diff --git a/content/svg/content/src/nsSVGAnimateMotionElement.cpp b/content/svg/content/src/nsSVGAnimateMotionElement.cpp
>-nsIAtom*
>-nsSVGAnimateMotionElement::GetTargetAttributeName() const
>+PRBool
>+nsSVGAnimateMotionElement::GetTargetAttributeName(PRInt32 *aNamespaceID,
>+                                                  nsIAtom **aLocalName) const

Make the *'s there be left-hugging, not right-hugging, to match contextual code.

This applies to all modified function-signatures for GetTargetAttributeName(), in this patch.

>diff --git a/content/svg/content/src/nsSVGAnimationElement.cpp b/content/svg/content/src/nsSVGAnimationElement.cpp
>+PRBool
>+nsSVGAnimationElement::GetTargetAttributeName(PRInt32 *aNamespaceID,
>+                                              nsIAtom **aLocalName) const
[...]
>   NS_ASSERTION(nameAttr->Type() == nsAttrValue::eAtom,
>     "attributeName should have been parsed as an atom");
>-  return nameAttr->GetAtomValue();
>+
>+  nsAutoString stringValue;
>+  nameAttr->ToString(stringValue);
>+  return NS_SUCCEEDED(nsContentUtils::SplitQName(this, stringValue,

Since we know we're dealing with an atom-typed nsAttrValue, it'd be faster to pass
  nsDependentAtomString(nameAttr->GetAtomValue())
directly to SplitQName, instead of generating |stringValue|, if we can.

(I'm actually not sure this suggestion would work, though, since SplitQName takes a |const nsAFlatString&|, and I don't know if nsDependentAtomString inherits from nsAFlatString.  If it works, we should do it; if not, this is fine as-is.)

>diff --git a/content/svg/content/src/nsSVGScriptElement.cpp b/content/svg/content/src/nsSVGScriptElement.cpp
>+void
>+nsSVGScriptElement::DidAnimateString(PRUint8 aAttrEnum)
>+{
>+  if (aAttrEnum == HREF) {
>+    MaybeProcessScript();
>+    return;

Can this trigger scripts to run synchronously (during a SMIL sample)?  If so, we should avoid that...

>diff --git a/content/svg/content/src/nsSVGString.cpp b/content/svg/content/src/nsSVGString.cpp
>+void
>+nsSVGString::SetAnimValue(const nsAString& aValue, nsSVGElement *aSVGElement)
>+{
>+  mAnimVal = new nsString();
>+  *mAnimVal = aValue;
>+  aSVGElement->DidAnimateString(mAttrEnum);
>+}

I'd expect that we need to control access to this function with a "mIsAnimated" variable, like we do in nsSVGLength2. (to prevent DOM calls from modifying the animated value, when it's already been set by SMIL animation.) Maybe I'm missing something, though?

>+nsSMILValue
>+nsSVGString::SMILString::GetBaseValue() const
>+{
>+  nsSMILValue val(&SMILStringType::sSingleton);
>+  nsAutoString string;
>+  mSVGElement->GetStringBaseValue(mVal->mAttrEnum, string);
>+  *static_cast<nsAString*>(val.mU.mPtr) = string;
>+  return val;

I don't think we need the extra |string| variable (and the extra string-copying we get from using it). We should be able to directly pass
  *static_cast<nsAString*>(val.mU.mPtr)
(or an alias for it) into GetStringBaseValue.

Comment 12

[Daniel Holbert [:dholbert]]

Based on clicking through "xlink:href" links at
   http://www.w3.org/TR/SVG11/attindex.html
here's a (non-exhaustive) list of some other elements whose xlink:href attributes need to be animatable:
<a>
  http://www.w3.org/TR/SVG/linking.html#AElementXLinkHrefAttribute
<textpath>
  http://www.w3.org/TR/SVG/text.html#TextPathElementHrefAttribute
<pattern>
  http://www.w3.org/TR/SVG/pservers.html#PatternElementHrefAttribute
<radialGradient>, <linearGradient>

AFAICT, we don't invalidate for animations on those elements' xlink:href elements in this patch -- can you make sure that works? (I think we just need a DidAnimateString() implementation in the appropriate place, right?)

Also, xlink:href on some elements is explicitly *not* animatable in the spec -- it'd be nice to have some tests to assert that animations on those don't work.  (and perhaps a custom DidAnimateString impl with NS_ABORT_IF_FALSE(PR_FALSE), to make sure we don't let ourselves to animate them?)

Some of those non-animatable-xlink:href elements are:
<mpath>
  http://www.w3.org/TR/SVG11/animate.html#MPathElementHrefAttribute
<script> (which it looks like your current patch assumes is animatable?)
  http://www.w3.org/TR/SVG11/script.html#ScriptElementHrefAttribute
Also <animate> & friends, I think, though it looks like there's no "Animatable" declaration where I'd expect it at http://www.w3.org/TR/SVG11/animate.html#HrefAttribute

Comment 13

[Robert Longson [:longsonr]]

Attachment: part 3 - tests

include a gradient test

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 478028 [details] [diff] [review]
part 3 - tests

>diff --git a/layout/reftests/svg/smil/anim-gradient-href-01.svg b/layout/reftests/svg/smil/anim-gradient-href-01.svg
>+         calcMode="discrete"

Nit: this (calcMode="discrete") doesn't actually change anything, right?  I'd expect we should fall into discrete animation even without this, since we can't interpolate strings.  (Regardless, we're seeking to a time after the animation has completed, so we'll be at the final value regardless of calcMode.)

Maybe this is still useful just for readability, though... so no need to remove it, if you like it. I'm just thinking out loud. :)

>diff --git a/layout/reftests/svg/smil/reftest.list b/layout/reftests/svg/smil/reftest.list
>+# animate some string attributes:
>+== anim-gradient-href-01.svg lime.svg
>+== anim-image-href-01.svg lime.svg

The latter test there (anim-image-href-01.svg) isn't included in this patch.

This test looks good -- r=dholbert on it -- but I'd ideally like to have tests for more string attributes (in comment 0 & comment 12) before the final patch here lands.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 471256 [details] [diff] [review]
part 2 - string

Canceling review on part 2 -- feedback already provided on it in comment 11 / comment 12.

Comment 16

[Robert Longson [:longsonr]]

Attachment: part 2 - string

(In reply to comment #11)
> 
> Perhaps it'd be cleaner for the static_cast for |delete| to match the type used
> for |new|? (nsString, in this case)
> 
> I guess virtual inheritance of destructors should keep this from actually
> causing problems, but as long as the |new| and |delete| calls are only a few
> lines apart in the code, it seems like it's more future-proof for them to use
> the same type.  I don't have strong feelings on this, though - up to you.

I like it my way so I left this as it is :-)

> 
> >diff --git a/content/smil/nsSMILTargetIdentifier.h b/content/smil/nsSMILTargetIdentifier.h
> >   inline PRBool Equals(const nsSMILTargetIdentifier& aOther) const
> >   {
> >     return (aOther.mElement       == mElement &&
> >             aOther.mAttributeName == mAttributeName &&
> >+            aOther.mAttributeNamespaceID == mAttributeNamespaceID &&
> >             aOther.mIsCSS         == mIsCSS);
> 
> Add whitespace to the contextual lines so that the "==" characters continue to
> line up.

Done.

> 
> >diff --git a/content/svg/content/src/nsSVGAnimateMotionElement.cpp b/content/svg/content/src/nsSVGAnimateMotionElement.cpp
> >-nsIAtom*
> >-nsSVGAnimateMotionElement::GetTargetAttributeName() const
> >+PRBool
> >+nsSVGAnimateMotionElement::GetTargetAttributeName(PRInt32 *aNamespaceID,
> >+                                                  nsIAtom **aLocalName) const
> 
> Make the *'s there be left-hugging, not right-hugging, to match contextual
> code.
> 
> This applies to all modified function-signatures for GetTargetAttributeName(),
> in this patch.

I had a go but this is all over the place. I've tried to make individual files consistent but that's all. Some I've even made left hugging where the rest of the file is like that :-(

> >+nsSVGAnimationElement::GetTargetAttributeName(PRInt32 *aNamespaceID,
> >+                                              nsIAtom **aLocalName) const
> [...]
> >   NS_ASSERTION(nameAttr->Type() == nsAttrValue::eAtom,
> >     "attributeName should have been parsed as an atom");
> >-  return nameAttr->GetAtomValue();
> >+
> >+  nsAutoString stringValue;
> >+  nameAttr->ToString(stringValue);
> >+  return NS_SUCCEEDED(nsContentUtils::SplitQName(this, stringValue,
> 
> Since we know we're dealing with an atom-typed nsAttrValue, it'd be faster to
> pass
>   nsDependentAtomString(nameAttr->GetAtomValue())
> directly to SplitQName, instead of generating |stringValue|, if we can.
> 
> (I'm actually not sure this suggestion would work, though, since SplitQName
> takes a |const nsAFlatString&|, and I don't know if nsDependentAtomString
> inherits from nsAFlatString.  If it works, we should do it; if not, this is
> fine as-is.)

It worked so done.

> 
> >diff --git a/content/svg/content/src/nsSVGScriptElement.cpp b/content/svg/content/src/nsSVGScriptElement.cpp
> >+void
> >+nsSVGScriptElement::DidAnimateString(PRUint8 aAttrEnum)
> >+{
> >+  if (aAttrEnum == HREF) {
> >+    MaybeProcessScript();
> >+    return;
> 
> Can this trigger scripts to run synchronously (during a SMIL sample)?  If so,
> we should avoid that...

Added an mIsAnimateable to all strings to prevent that.

> 
> >diff --git a/content/svg/content/src/nsSVGString.cpp b/content/svg/content/src/nsSVGString.cpp
> >+void
> >+nsSVGString::SetAnimValue(const nsAString& aValue, nsSVGElement *aSVGElement)
> >+{
> >+  mAnimVal = new nsString();
> >+  *mAnimVal = aValue;
> >+  aSVGElement->DidAnimateString(mAttrEnum);
> >+}
> 
> I'd expect that we need to control access to this function with a "mIsAnimated"
> variable, like we do in nsSVGLength2. (to prevent DOM calls from modifying the
> animated value, when it's already been set by SMIL animation.) Maybe I'm
> missing something, though?

Strings don't need an isAnimated boolean because they use mAnimVal == null to indicate that.

> 
> >+nsSMILValue
> >+nsSVGString::SMILString::GetBaseValue() const
> >+{
> >+  nsSMILValue val(&SMILStringType::sSingleton);
> >+  nsAutoString string;
> >+  mSVGElement->GetStringBaseValue(mVal->mAttrEnum, string);
> >+  *static_cast<nsAString*>(val.mU.mPtr) = string;
> >+  return val;
> 
> I don't think we need the extra |string| variable (and the extra string-copying
> we get from using it). We should be able to directly pass
>   *static_cast<nsAString*>(val.mU.mPtr)
> (or an alias for it) into GetStringBaseValue.

Done.

(In reply to comment #12)
> Based on clicking through "xlink:href" links at
>    http://www.w3.org/TR/SVG11/attindex.html
> here's a (non-exhaustive) list of some other elements whose xlink:href
> attributes need to be animatable:

I've introduced an mIsAnimatable attribute on stringinfo and set each value by reading the spec.

> 
> AFAICT, we don't invalidate for animations on those elements' xlink:href
> elements in this patch -- can you make sure that works? (I think we just need a
> DidAnimateString() implementation in the appropriate place, right?)

Actually no. We don't need that, nsSVGElement::DidAnimateString does a refresh we don't need to override that.

> 
> Also, xlink:href on some elements is explicitly *not* animatable in the spec --
> it'd be nice to have some tests to assert that animations on those don't work. 
> (and perhaps a custom DidAnimateString impl with NS_ABORT_IF_FALSE(PR_FALSE),
> to make sure we don't let ourselves to animate them?)

I haven't added tests for this, although I suppose could. I've added a mIsAnimateable flag that prevents the animated value from being set.

> 
> Some of those non-animatable-xlink:href elements are:
> <mpath>
>   http://www.w3.org/TR/SVG11/animate.html#MPathElementHrefAttribute
> <script> (which it looks like your current patch assumes is animatable?)

Oops, fixed.

>   http://www.w3.org/TR/SVG11/script.html#ScriptElementHrefAttribute
> Also <animate> & friends, I think, though it looks like there's no "Animatable"

<animate> href isn't modelled as an nsSVGString so it doesn't count! The rest are explicitly disabled by the mIsAnimatable flag.

Comment 17

[Robert Longson [:longsonr]]

(In reply to comment #14)
> Comment on attachment 478028 [details] [diff] [review]
> part 3 - tests
> 
> >diff --git a/layout/reftests/svg/smil/anim-gradient-href-01.svg b/layout/reftests/svg/smil/anim-gradient-href-01.svg
> >+         calcMode="discrete"
> 
> Nit: this (calcMode="discrete") doesn't actually change anything, right?  I'd
> expect we should fall into discrete animation even without this, since we can't
> interpolate strings.  (Regardless, we're seeking to a time after the animation
> has completed, so we'll be at the final value regardless of calcMode.)
> 
> Maybe this is still useful just for readability, though... so no need to remove
> it, if you like it. I'm just thinking out loud. :)

Right it's just an aide-mémoire.

I can write more tests. I bet the gradient test fails on Mac because of bug 568881 so I'll need to tryserver this whole lot too.

Comment 18

[Robert Longson [:longsonr]]

Attachment: part 3 - tests

From comment 0...

Does not test class. That's not modelled by an nsSVGString currently - perhaps it should be but it's out of scope of this bug. 

tests for in and result being animatable on filter primitives.

Does not test href or target on a as there's no visible effect although I could write a mochitest I suppose. The SVG testsuite test shows it works with this patch though.

From comment 11...
See above for <a>

Does not test textPath as there's nothing special about it.

Tests pattern, linearGradient and additionally image and use. Note that use is special so does deserve a test.

Mochitests for non-animated stuff tests mpath and script.

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 478034 [details] [diff] [review]
part 2 - string

(In reply to comment #16)
> I had a go but this is all over the place. I've tried to make individual files
> consistent but that's all. Some I've even made left hugging where the rest of
> the file is like that :-(

Yup, consistency-within-a-file is what we've aimed for in the past, I think (with a slight bias towards left-hugging, since the "*" is part of the type, not part of the variable-name).  Thanks for fixing that up!

diff --git a/content/svg/content/src/nsSVGAnimateMotionElement.cpp b/content/svg/content/src/nsSVGAnimateMotionElement.cpp
[...]
> nsSMILTargetAttrType
> nsSVGAnimateMotionElement::GetTargetAttributeType() const
> {
>   // <animateMotion> doesn't take an attributeType, since it doesn't target an
>   // 'attribute' per se.  We'll just return 'XML' for simplicity.  (This just
>   // needs to match what we expect in nsSVGElement::GetAnimAttr.)
>   return eSMILTargetAttrType_XML;
>-}
>+}
>\ No newline at end of file

Looks like your new patch accidentally deletes the newline at the end of nsSVGAnimateMotionElement.cpp, per above-quoted patch-chunk.  Could you revert that?  r=dholbert with that tweak.

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 478532 [details] [diff] [review]
part 3 - tests

One general comment: It looks like all the reftests use fill="freeze" and take a snapshot 0.5s after the end time.

As long as we've got multiple reftests, I'd prefer if you tweaked one or two of them to take a snapshot at 50% through the animation (at time 0.25 seconds), which should get the final animation value. In particular, if you do that & also remove calcMode="discrete" in the same test, that would give us some code-coverage of SMILStringType::Interpolate() for free, which would be nice.  (Right now, Interpolate() will never get hit by your tests.)

>diff --git a/layout/reftests/svg/smil/anim-gradient-href-01.svg b/layout/reftests/svg/smil/anim-gradient-href-01.svg
>+    <linearGradient id="grad1a" gradientUnits="objectBoundingBox" x1="0" y1="0" x2="1" y2="0">
>+      <stop id="red"     stop-color="red"       offset="0"/>
[...]
>+      <stop id="lime"     stop-color="lime"     offset="0"/>

Nit: The spacing on these two lines looks a little odd -- it looks like you're trying to make the attributes line up between those two lines, but "stop-color" doesn't quite line up.

>diff --git a/layout/reftests/svg/smil/anim-use-href-01.svg b/layout/reftests/svg/smil/anim-use-href-01.svg
>+  <use xlink:href="#sym1">
>+    <animate attributeName="xlink:href"
>+     calcMode="discrete"
>+     begin="0s" dur="0.5s"
>+     from="#sym1" to="#sym2"
>+     fill="freeze"/>
>+  </use>

Doesn't this test hit an assertion-failure in debug builds, bug 563481?

I'd expect that it should, just by virtue of the fact that it has a <use> element that targets a <symbol>.  Assuming that it does, it needs an annotation (e.g. "asserts(1)") in the reftest.list file, or it'll trigger a reftest failure (and hence orange) due to unexpected assertion-failures.

r=dholbert with the above addressed.

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to comment #20)
> Doesn't this test hit an assertion-failure in debug builds, bug 563481?
> Assuming that it does, it needs an annotation

(include a comment mentioning "bug 563481" with the annotation, too)

Comment 22

[Robert Longson [:longsonr]]

Attachment: part 2 - string

Comment 23

[Robert Longson [:longsonr]]

(In reply to comment #20)
> Doesn't this test hit an assertion-failure in debug builds, bug 563481?
> 
> I'd expect that it should, just by virtue of the fact that it has a <use>
> element that targets a <symbol>.  Assuming that it does, it needs an annotation
> (e.g. "asserts(1)") in the reftest.list file, or it'll trigger a reftest
> failure (and hence orange) due to unexpected assertion-failures.
> 
> r=dholbert with the above addressed.

6 asserts seems to cover it on any platform according to tryserver. I'll include this and the other test changes when I do the combined patch for approval.

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to comment #23)
> I'll
> include this and the other test changes when I do the combined patch for
> approval.

IMHO this should be blocking2.0=betaN+ (& hence shouldn't need approval) -- requesting blocking status.

Comment 25

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

How does this relate to there being a feature freeze?  Is this the last piece of something that's already otherwise done?

Also, roc's offline Sept. 27 - Oct. 8, so you're not going to get a prompt review from him.

Comment 26

[Daniel Holbert [:dholbert]]

(In reply to comment #25)
> Is this the last piece
> of something that's already otherwise done?

It's among the last few SVG types that we can't animate yet.  (the others being Bug 522306, bug 522308, and bug 589439 are the other missing types.  I think jwatt is currently working on the first of those, and I'm not sure if the others will make it.)

> How does this relate to there being a feature freeze?

My impression is that this is a (relatively small) missing piece of an already-implemented feature, and would be ok to land post-feature-freeze, as long as it has tests & gets some beta coverage.

Comment 27

[Jonathan Watt [:jwatt]]

Yeah, roc was clear (to me) that he wants us to keep working on these while he's away and get them in. That includes the three bugs dholbert mentioned in the previous comment that are on my plate.

Comment 28

[Daniel Holbert [:dholbert]]

(In reply to comment #23)
> 6 asserts seems to cover it on any platform according to tryserver. I'll
> include this and the other test changes when I do the combined patch for
> approval.

I fixed that & my other nits from comment 20 in my patch queue, in preparation for landing this:
 http://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/rev/5d98c7dc2997

I've confirmed that the assertion-count is correct & the tests pass.
longsonr: if you're around, let me know if there were any other test changes you were intending to make, and I'll add them before pushing.

Comment 29

[Robert Longson [:longsonr]]

Attachment: part 3 - tests

Oops sorry about that, I thought I'd uploaded this update. Feel free to use this or your update as you see fit.

Comment 30

[Daniel Holbert [:dholbert]]

oops, looks like the Comment 29 version is actually missing the chunk to add the mochitest to its Makefile (to get run)  :).  That chunk was there in the old version, though, so I've got it in my patch-queue.

I changed my patch-queue version to match a few of your tweaks -- will land in the next hour or two, depending on how the landing queue progresses.

Comment 31

[Robert Longson [:longsonr]]

Thanks Daniel.

Comment 32

[Daniel Holbert [:dholbert]]

You're welcome! Landed:
 http://hg.mozilla.org/mozilla-central/rev/4aaba15aaecf
 http://hg.mozilla.org/mozilla-central/rev/8efdbfd80d46
 http://hg.mozilla.org/mozilla-central/rev/dc5fc7c30ee3