481562 - SVG Animation: <svg> elements shouldn't create a time container until appended to a document

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Right now, our nsSVGSVGElement::BindToTree method gives slightly odd behavior -- it creates a time container the first time it's called, even if we aren't bound to a document yet.

Instead, it should wait to create the time container until *after* it's got a document (and has access to the document's animation controller).  Justification below.

Consider the following experiment:

 (0) Start with a XHTML document
 (1) Create div and svg elements dynamically
 (2) Call setCurrentTime(1.0) on svg (to seek the animation forward)
 (3) Append svg as child of div (Note that div itself has no parent)
 (4) Append div to our XHTML document

In this experiment, the setCurrentTime call in step (2) will silently fail (as it should), but if we swap steps (2) and (3), then the setCurrentTime call succeeds.  That's not particularly sensible.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: reftest testcase, version a

Here's a testcase that carries out the experiment from comment 0, with the initial ordering of steps.
(In this testcase, the setCurrentTime call currently fails, as it should.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: reftest testcase, version b

(In reply to comment #0)
> but if we swap steps (2) and (3), then the setCurrentTime call
> succeeds.  That's not particularly sensible.

Here's a testcase that has these steps switched w.r.t. the previous testcase.  mozilla-central currently has broken behavior on this testcase -- we honor the setCurrentTime call, when we really shouldn't.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: reftest reference case

... and here's a simple reference case for these testcases.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch v1

Here's a patch to fix this, with reftests.

The first set of added reftests (deferred-tree-2*) are just the testcases that I've already attached here. (though I've fixed the removal of "reftest-wait" class -- that was broken in the versions I attached previously on this bug)

The second set of added reftests (deferred-tree-3*) cover an issue that was broken in an earlier version of my patch.  My earlier (broken) patch cleared the mTimedDocumentRoot pointer inside of nsSVGSVGElement::UnbindFromTree.  But that causes bad behavior when we're merely moving a <svg> element around within the same document -- it'll make us lose any existing time-container state. (e.g. we'll forget about any setCurrentTime / pauseAnimations calls that have occurred).  So, this second set of reftests verify that we handle this sort of situation correctly.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: patch v2

Here's a slight refinement of the previous patch.

The previous patch had a special case to explicitly clear the time-container when we move an outermost <svg> element from a SMIL-enabled document to a SMIL-disabled document, but I've removed that special case in this version.  I think we make more sense without it, because we already preserve time-container state when detaching a <svg> element from a document and then reattaching it later (as illustrated in the 2 new tests added in this patch), and I think it makes sense to also preserve this state when moving a <svg> element to a SMIL-disabled document and back.  The time container will have no effect while we're in the SMIL-disabled document (since no sampling will happen), so we'll still be honoring the disabled-ness. (Note that this only would come into play in an extremely odd use-case of dynamically generating documents before & after tweaking the about:config pref, and then moving SVG content between the documents.)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: patch v3

One final refinement -- we don't need the temporary 'outermost' variable anymore. We can just put the WillBeOutermostSVG call directly in the "if" check.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 365788 [details] [diff] [review]
patch v3

I think this is ready for review.  Summary of patch:
 - Change top half of nsSVGSVGElement::BindToTree so that we *only* create a time container if we have a document with SMIL enabled.[1]
 - Change bottom half of nsSVGSVGElement::BindToTree to simplify conditions, since we've already gotten a smilController pointer. 

Note: I've slightly narrowed the conditions under which we call "mTimedDocumentRoot->Begin()" in the second half, as an optimization / minor cleanup. Previously, that call was within a "if (mTimedDocumentRoot)" call, and now it's within "if (mTimedDocumentRoot && smilController)".

I claim that this shouldn't have any functional effect (aside from saving us some redundant calls to Begin()), because
 - we only ever need to call Begin() *once* on a time-container
 - If, with my patch, we happen to skip that clause for a given time-container because smilController is null, we can be sure that we must've hit that clause before with the same time-container and with a *non-null* smilController -- in particular, we must've hit it with those conditions during the BindToTree call that created the time-container in the first place. (because we only create the time-container if smilController is non-null)

[1] when I say "with smil enabled", I just mean "with nsIDocument::GetAnimationController returning non-null". See my patch v1 on bug 473904 which adds an about:config pref to toggle this.

Comment 8

[Daniel Holbert [:dholbert]]

(FWIW, I've verified that this patch passes both the existing & included SMIL reftests.)

Comment 9

[Daniel Holbert [:dholbert]]

Pushed: http://hg.mozilla.org/mozilla-central/rev/b0df372b9192