Closed
Bug 481562
Opened 16 years ago
Closed 16 years ago
SVG Animation: <svg> elements shouldn't create a time container until appended to a document
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
People
(Reporter: dholbert, Assigned: dholbert)
Details
Attachments
(4 files, 2 obsolete files)
2.90 KB,
application/xhtml+xml
|
Details | |
2.90 KB,
application/xhtml+xml
|
Details | |
528 bytes,
application/xhtml+xml
|
Details | |
16.42 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•16 years ago
|
||
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.)
Assignee | ||
Comment 2•16 years ago
|
||
(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.
Assignee | ||
Comment 3•16 years ago
|
||
... and here's a simple reference case for these testcases.
Assignee | ||
Comment 4•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Summary: SVG Animation: Shouldn't create a time container until appended to a document → SVG Animation: <svg> elements Shouldn't create a time container until appended to a document
Assignee | ||
Updated•16 years ago
|
Summary: SVG Animation: <svg> elements Shouldn't create a time container until appended to a document → SVG Animation: <svg> elements shouldn't create a time container until appended to a document
Assignee | ||
Comment 5•16 years ago
|
||
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.)
Assignee | ||
Updated•16 years ago
|
Attachment #365596 -
Attachment is obsolete: true
Assignee | ||
Comment 6•16 years ago
|
||
One final refinement -- we don't need the temporary 'outermost' variable anymore. We can just put the WillBeOutermostSVG call directly in the "if" check.
Attachment #365770 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Attachment #365788 -
Flags: superreview?(roc)
Attachment #365788 -
Flags: review?(roc)
Assignee | ||
Comment 7•16 years ago
|
||
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.
Assignee | ||
Comment 8•16 years ago
|
||
(FWIW, I've verified that this patch passes both the existing & included SMIL reftests.)
Attachment #365788 -
Flags: superreview?(roc)
Attachment #365788 -
Flags: superreview+
Attachment #365788 -
Flags: review?(roc)
Attachment #365788 -
Flags: review+
Assignee | ||
Comment 9•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•