487450 - Remove SMIL animation effects from SVG elements, when animation target changes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase: red square should move out & then snap back behind green square

In our current SVG Animation implementation, when you retarget an SVG animation node, its effects on its old target will remain -- effectively "pausing" the animation at the moment of removal.

For correctness & sanity, we should *remove* animation effects from the old target once an animation node has been retargeted.

This fix should use the member variable nsSMILAnimationController::mLastCompositorTable, which stores all animations (for each animated attribute) in the previous sample.  We should be able to check that against the current sample's compositor table, detect what (if anything) has changed, and then clean up stale animation effects.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> This fix should use the member variable
> nsSMILAnimationController::mLastCompositorTable [snip]

Actually, I think there's a cleaner way to do this without mLastCompositorTable (which means we can remove mLastCompositorTable, which is great, because it seems messy :)).

The basic goal here is to jump in whenever the animation's target elem/attr changes, so we can clear our animation effects from the old target elem/attr.

If the animation is targeted with xlink:href, we need to consider these cases:
 (a) xlink:href attribute value is changed
 (b) xlink:href attribute value is cleared / unset
 (c) The DOM is munged (or node ids are changed) so that a different node is the new target (i.e. a new node is the first node in the document with the target ID)
 (d) The <animate> node is removed from the tree

We can cover (a) and (b) by adding hooks to SetAttr / UnsetAttr (in a xlink:href-specific clause).  We can cover (c) by adding code to be run during the overridden version of nsReferencedElement::ContentChanged in bug 491080's patch (assuming that nsReferencedElement behaves as magically as I'm hoping it does).  And we can cover (d) by adding code to UnbindFromTree.

If we *aren't* using xlink:href targeting -- i.e. if we're just targeting our parent -- we'd only need to catch these cases:
 (e) the <animate> node is (re)moved (so that it's no longer targeting its old parent)
 (f) we set the xlink:href attribute (so that we aren't targeting our parent anymore)

We can cover (e) by adding code to UnbindFromTree, and we can cover (f) by adding code to SetAttr.

And then there's one final group of tweaks to consider, regardless of whether we're targeting via xlink:href or not:
 (g) we change (or clear) the value of the attribute "attributeName"
 (h) we change (or clear) the value of the attribute "attributeType"
 (i) we tweak some other attribute on the animation node in such a way as to make it invalid. (i.e. set one of the main attributes to have a gibberish value)

(g) through (i) could probably all be handled by adding code to SetAttr as well -- perhaps we should just preemptively clear animation effects on the target elem/attribute, at the beginning of any SetAttr call on an animation node.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch v1

Disregard comment 1, I think -- when I wrote it, I think I forgot that we were using ref-counted pointers in compositor-table, and I was worried about using pointers from the previous sample that might point to dead content.

Anyway, here's a fix to this using mLastCompositorTable.  This patch makes the testcase here pass, and it passes reftests.  I want to test this a bit more (& bundle tests with it) before requesting review, though.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: patch v2

Changes in this patch:
 - The patch now gets rid of static wrapper-method "nsSMILCompositor::ComposeAttributes" -- it's unnecessary, because nsSMILAnimationController can just as easily do the hash-iteration itself.  (This mirrors the behavior of the clear-animation-effects code introduced in this patch.)
 - Added reftests covering all of the cases described in comment 1.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: [ignore -- attached wrong patch]

This patch adds a necessary ID attribute in two reftests on the previous patch.  

Now, all the reftests fail pre-patching and pass post-patching, with one exception -- anim-remove-7.svg passes both before & after patching, because it doesn't actually involve changing an animation *target* -- it changes the animation "by" amount to be something invalid (leaving the target the same) and we already correctly handle that.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: patch v2b

(sorry, I attached wrong patch file before. this is the correct one.)

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (mVal->mIsAnimated) {
+    mVal->SetAnimValue(mVal->mBaseVal, mSVGElement);
+  }  

Shouldn't we be doing something here so that mIsAnimated gets set to false?

Otherwise looks great!

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: patch v2c

(In reply to comment #6)
> Shouldn't we be doing something here so that mIsAnimated gets set to false?

Yup, we should.  (AFAICT, that won't actually change our functional behavior -- it just avoids a bunch of needless calls to AnimationNeedsResample(), thereby making us faster.)

That's fixed in this patch, plus a tweak to the reftest.list manifest.  (It now assumes that the xlink:href animation-targeting patch -- bug 491080 -- stacks on top of this patch, rather than the other way around.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: diffs between patch v2b & v2c

Here's a raw diff between patches v2b vs v2c.  (I couldn't do an interdiff, because patch v2b won't apply cleanly against stock mozilla-central -- it wants to stack on top of the xlink:href-targeting patch.)

Comment 9

[Daniel Holbert [:dholbert]]

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/53e1bb6cf3ad