Last Comment Bug 628407 - SMIL repeat events are fired too many times when an animation element is detached / reattached from DOM tree
: SMIL repeat events are fired too many times when an animation element is deta...
Status: UNCONFIRMED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-24 14:25 PST by Lukas Laag
Modified: 2011-01-25 04:37 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
sample to reproduce the problem (1.25 KB, text/html)
2011-01-24 14:26 PST, Lukas Laag
no flags Details

Description Lukas Laag 2011-01-24 14:25:03 PST
User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b9) Gecko/20100101 Firefox/4.0b9
Build Identifier: 4.0b9

Hi,

I have noticed a behavior which I cannot understand in the new svg smil animation feature (see attached sampled). I ran into the problem while developing an UI which has tabs displaying SMIL animations. The animations have a repeat handler. When the user switches tabs, the toolkit I use (gwt) removes the SVG from the DOM tree (without removing the repeat handler), and later re-appends it if the tab needs display again.

However when the SVG gets re-attached, I get a huge event firing activity (much more than what would have happened if the animation had remained attached) and it worsens with time (tens then hundreds then thousands of events fired when one reattaches the element).


Reproducible: Always

Steps to Reproduce:
1. open the sample file
2. click the "hide" button, then the "show" button
3. repeat step 2 a few times and watch the repeat event counter go up
Actual Results:  
This happens on FF4b9 on both win7 and linux. I tested with opera too but no problem there

Expected Results:  
The events should keep firing at the same rate as when the element is attached, or maybe not at all.

This happens on FF4b9 on both win7 and linux. I tested with opera too but no problem there.
Comment 1 Lukas Laag 2011-01-24 14:26:06 PST
Created attachment 506530 [details]
sample to reproduce the problem
Comment 2 Brian Birtles (:birtles) 2011-01-24 17:06:08 PST
There are basically two things happening here:
A) Inserting an <animate> element into an already running SVG document fragment
B) Inserting an <svg> element (which has already run) into a compound document

The behaviour described here is a result of (A), i.e. rebinding the <animate> element. SMIL doesn't say what should happen in this case.

We have decided not to preserve animation state but rebuild it upon rebinding to the tree. This decision seems to be supported by most others in the discussion below.

http://lists.w3.org/Archives/Public/www-svg/2010Oct/0048.html

The reason for this is mostly for consistency. For example, when inserting animation X into an SVG document fragment that is in play, even if we decide not to fire its past events, we still need to run through its past intervals. If one of those past intervals causes animation Y to begin, presumably Y should fire its beginEvent. So we'd either end up selectively suppressing events for one element, i.e. X but not Y, (which seems inconsistent and is also hard to implement), or temporarily turning off all events (which I think would produce even more unexpected results).

Thus in the test case when we re-attach the <svg> element we will also rebind the <animate> element which will discard its old animation state and seek forwards to the current time of the parent <svg> element, firing events along the way. The number of events will be proportional to the time distance from t=0 to t=currentTime hence the increasing number of events upon each rebind.


However, while I think we're doing the right thing by (A), it's worth considering if we could do something more helpful regarding (B), i.e. inserting an <svg> element (which has already run) into a compound document.

It seems like if a whole <svg> document fragment is detached and re-attached we should be able to preserve timing information and avoid re-running these past intervals but unfortunately that's not the case. An <svg> document fragment can have timing dependencies on other <svg> document fragments within the one compound document. So detaching the fragment will alter the timing of those other fragments, as will re-attaching. So we really do need to rebuild the timing information on a re-attach.

What we might be able to do is to say that the initial seek when re-attaching a whole document fragment doesn't fire events. That is, when we bind an <svg> element, if its time container has already played, put it in the seeking state. This will suppress *most* events (specifically, repeatEvents, endEvents and all but the first beginEvent).

That probably wouldn't be too inconsistent and would certainly improve performance in that case.


For the use case mentioned for this bug, however, we really should not be detaching and re-attaching SVG document fragments. As mentioned above, because we can have dependencies between SVG document fragments detaching and re-attaching is expensive. It would be much better in this case to pause the fragment and set display:none. That would eliminate the extra events.


Note that Opera and WebKit do not support TimeEvents properly. Opera seems to dispatch events somewhat unpredictably and last I tested WebKit, it didn't dispatch them at all.
Comment 3 Brian Birtles (:birtles) 2011-01-24 17:10:40 PST
One thing I failed to mention is that <svg> elements remember the time they were detached from the tree, and resume from that time upon being re-attached.
Comment 4 Lukas Laag 2011-01-25 00:18:48 PST
Hi Brian,
Many thanks for this detailed explanation, it makes much more sense now. To sum it up from my web developer point of view:
1/ It is a bad programming pattern to reattach an SVG fragment containing SMIL animation as this will force the SVG to rebuild its internal state.
2/ In a future version you might be able to take into account the time at which the fragment is reattached and not re-fire past events as you rebuild the internal state of the SVG fragment to synchronize it with the present time.

There is just one bit which was not clear to me: you mention cases where where an <svg> document fragment can have timing dependencies on other <svg> document fragments. When will that happen ?
Comment 5 Brian Birtles (:birtles) 2011-01-25 03:50:04 PST
(In reply to comment #4)
> Hi Brian,
> Many thanks for this detailed explanation, it makes much more sense now. To sum
> it up from my web developer point of view:
> 1/ It is a bad programming pattern to reattach an SVG fragment containing SMIL
> animation as this will force the SVG to rebuild its internal state.
> 2/ In a future version you might be able to take into account the time at which
> the fragment is reattached and not re-fire past events as you rebuild the
> internal state of the SVG fragment to synchronize it with the present time.

Right, you said it much better that I! Thanks Lukas!

> There is just one bit which was not clear to me: you mention cases where where
> an <svg> document fragment can have timing dependencies on other <svg> document
> fragments. When will that happen ?

Yes, for example:

<html>
  <body>
    <svg>
      <circle>
        <animate id="a" begin="2s; b.begin+1s" ... />
      </circle>
    </svg>
    <svg>
      <rect>
        <animate id="b" begin="5s; a.end" ... />
      </rect>
    </svg>
  </body>
</html>

With this kind of arrangement we have time dependencies between different SVG document fragments but within the one compound document.

If you remove one SVG fragment the dependencies in the other should be updated accordingly. Likewise if you re-attach it.

(This kind of use case pushing the boundaries a bit. It gets a bit complicated because the <svg> fragments are technically separate time containers that can be paused independently. When one is paused, the time in the other needs to be updated. This is something we've tested and should work in Firefox but I'm not sure about other browsers.)
Comment 6 Lukas Laag 2011-01-25 04:37:35 PST
Very interesting, I did not know one could do that. Many thanks again.

Note You need to log in before you can comment on or make changes to this bug.