Bug 27238 – Animated GIF dynamically removed and readded to page does not re-animate

• Component: Images
• Status: NEW

[Peter Kasting]

Created attachment 32681 [details]
Testcase

Open the attached testcase.  Hitting "Play" repeatedly should repeatedly animate the image.  This works fine in Firefox/IE but fails in Safari/Google Chrome.

[Swapna P]

Hi 
I am Working on this issue.

[Swapna P]

Created attachment 103461 [details]
Added resetAnimation() fuction call in order to restart animation 

For each click, it is creating one pair of instances of HTMLImageElement & RenderImage but they all share the same BitmapImage instance for animation.

For the first click of play button , image is getting animated. If we click for 2nd time, it again calls for BitmapImage::startAnimation() but because the animation is currently continuing, the function just returns without doing anything. But the expected behavior is that for each click, all the existing image animations has to be restarted as IE/FF does.

So, therefore we need to somehow call resetAnimation() for the above behavior in each button click & that's why when the new image being appended, we are calling resetAnimation().

I have thought about all the corner cases to make sure that this change will not have any side effect. Please let me know your opinion.

I have not yet added the test case for this. If the code changes are fine, I will add the test case & upload a final patch.

[WebKit Review Bot]

Comment on attachment 103461 [details]
Added resetAnimation() fuction call in order to restart animation 

Attachment 103461 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9339590

New failing tests:
http/tests/inspector/console-resource-errors.html
http/tests/inspector/network/disabled-cache-crash.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/network/network-cachedresources-with-same-urls.html
http/tests/inspector/console-cd.html
http/tests/inspector/change-iframe-src.html
http/tests/inspector/console-cd-completions.html
http/tests/inspector/resource-har-headers.html
http/tests/inspector/network/download.html
http/tests/inspector-enabled/console-clear-arguments-on-frame-remove.html
http/tests/inspector/inspect-iframe-from-different-domain.html
http/tests/inspector-enabled/console-log-before-frame-navigation.html
animations/animation-add-events-in-handler.html
http/tests/inspector/network-preflight-options.html
http/tests/inspector-enabled/dom-storage-open.html
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/console-websocket-error.html
http/tests/inspector/resource-parameters.html
http/tests/inspector-enabled/database-open.html
http/tests/inspector/network/network-clear-after-disabled.html

[Peter Kasting]

Comment on attachment 103461 [details]
Added resetAnimation() fuction call in order to restart animation 

View in context: https://bugs.webkit.org/attachment.cgi?id=103461&action=review

Does this change the current animation behavior of dynamically inserting a second copy of an image into a page that already contains one copy?  (I don't remember whether that resets animation or not, but whichever way it goes is intentional.)

> Source/WebCore/html/HTMLImageElement.cpp:220
>          m_imageLoader.updateFromElement();

Does this guarantee that the line below will not deref NULL?

[Swapna P]

Hi Peter,

It doesn't change the current animation behavior.
In resetAnimation() function , only the animation values are getting reset.
So it doesn't deref NULL.

[Swapna P]

Hi Peter,

If the above comment satisfies you, let me know. I will upload the patch along with the testcases.

[Peter Kasting]

(In reply to comment #5)
> It doesn't change the current animation behavior.

So, the current behavior is that adding a second copy of the image to the page resets the first and causes both to animate from the beginning?

> In resetAnimation() function , only the animation values are getting reset.
> So it doesn't deref NULL.

? This doesn't answer my question.  Right above the call you added we handle the case of m_imageLoader.image() returning NULL.  Then your new code promptly derefs that pointer blindly, along with the pointer it returns.  I want to know if that's safe.  That has nothing to do with what resetAnimation() does.

[Swapna P]

(In reply to comment #7)
> (In reply to comment #5)
> > It doesn't change the current animation behavior.
> 
> So, the current behavior is that adding a second copy of the image to the page resets the first and causes both to animate from the beginning?

Yes, the current behavior (with my patch) would be the same as you explained above i.e. adding a second copy of the image to the page will reset the first & both will start animation from the beginning.
> 
> > In resetAnimation() function , only the animation values are getting reset.
> > So it doesn't deref NULL.
> 
> ? This doesn't answer my question.  Right above the call you added we handle the case of m_imageLoader.image() returning NULL.  Then your new code promptly derefs that pointer blindly, along with the pointer it returns.  I want to know if that's safe.  That has nothing to do with what resetAnimation() does.

I now understand your question. if m_imageLoader.image() is NULL, m_imageLoader.updateFromElement() populates the m_imageLoader.image(). Therefore the code we have added is safe.

Still to be absolutely sure, we can add NULL check before our code also. It will not affect the functionality

[Peter Kasting]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > In resetAnimation() function , only the animation values are getting reset.
> > > So it doesn't deref NULL.
> > 
> > ? This doesn't answer my question.  Right above the call you added we handle the case of m_imageLoader.image() returning NULL.  Then your new code promptly derefs that pointer blindly, along with the pointer it returns.  I want to know if that's safe.  That has nothing to do with what resetAnimation() does.
> 
> I now understand your question. if m_imageLoader.image() is NULL, m_imageLoader.updateFromElement() populates the m_imageLoader.image(). Therefore the code we have added is safe.

And when m_imageLoader.image() is non-NULL, m_imageLoader.image().image() is guaranteed to be non-NULL?

> Still to be absolutely sure, we can add NULL check before our code also. It will not affect the functionality

If this really cannot be NULL, we should assert (or do nothing), rather than implying it can be NULL by adding a conditional.

[Swapna P]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #5)
> > > > In resetAnimation() function , only the animation values are getting reset.
> > > > So it doesn't deref NULL.
> > > 
> > > ? This doesn't answer my question.  Right above the call you added we handle the case of m_imageLoader.image() returning NULL.  Then your new code promptly derefs that pointer blindly, along with the pointer it returns.  I want to know if that's safe.  That has nothing to do with what resetAnimation() does.
> > 
> > I now understand your question. if m_imageLoader.image() is NULL, m_imageLoader.updateFromElement() populates the m_imageLoader.image(). Therefore the code we have added is safe.
> 
> And when m_imageLoader.image() is non-NULL, m_imageLoader.image().image() is guaranteed to be non-NULL?

Yes, correct. m_imageLoader.image().image() is never NULL.
> 
> > Still to be absolutely sure, we can add NULL check before our code also. It will not affect the functionality
> 
> If this really cannot be NULL, we should assert (or do nothing), rather than implying it can be NULL by adding a conditional.

Therefore, we will do nothing before executing resetAnimation() [no NULL checking required]

Also, I have one more doubt regarding test cases. I saw few test cases failing for Cr-linux though they did not seem to be related to my changes. Do you have any idea what could be the reason? 

And Do i need to add any test cases for my change? Because the expected behavior here is to see all the images animating (visual changes) so I was not sure how to write a test cases for my changes.

[Peter Kasting]

(In reply to comment #10)
> Also, I have one more doubt regarding test cases. I saw few test cases failing for Cr-linux though they did not seem to be related to my changes. Do you have any idea what could be the reason? 

No.

> And Do i need to add any test cases for my change? Because the expected behavior here is to see all the images animating (visual changes) so I was not sure how to write a test cases for my changes.

I'm not totally sure either.  A simple pixel test is going to show the image sitting on its final frame both with and without your patch.  Perhaps your reviewer will have an idea I don't.

[Swapna P]

Hi Peter, thank you for your comments.

Can any one review the patch, and let me know if any further changes needed.

[Ravi Phaneendra Kasibhatla]

Created attachment 108837 [details]
Fix for GIF not animating on dynamic detach & attach

pkasting: Attached is a patch which fixes the issue. 
Patch ensures that any detach of the image element from the DOM also ensures the image associated with the element (CachedResourceClient) is unregistered with the CachedResource which was created while loading the image element. If the CachedResource is not being referenced by any other client it is purged. So, when the image element is reattached, it becomes new client which is registered with the CachedResource and animation state is reset for it.

[WebKit Review Bot]

Comment on attachment 108837 [details]
Fix for GIF not animating on dynamic detach & attach

Attachment 108837 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9878337

New failing tests:
platform/mac/editing/deleting/deletionUI-differing-background.html

[Peter Kasting]

(In reply to comment #13)
> Patch ensures that any detach of the image element from the DOM also ensures the image associated with the element (CachedResourceClient) is unregistered with the CachedResource which was created while loading the image element.

My instinct is that this is a more correct fix.  Unfortunately I don't understand too much about how resource loading and caching actually works so I can't say for sure (and I'm not a reviewer anyway).  You might want to ask whomever worked on CachedImage?

[Ravi Phaneendra Kasibhatla]

Hi Nate,
Can you please review the attached patch (https://bugs.webkit.org/attachment.cgi?id=108837) and let me know your comments if any?

[Nate Chapin]

Comment on attachment 108837 [details]
Fix for GIF not animating on dynamic detach & attach

View in context: https://bugs.webkit.org/attachment.cgi?id=108837&action=review

Antti knows the CachedResource<->CachedResourceClient interface better than I do, so I cc:ed him.  One comment inline, though.

> Source/WebCore/html/HTMLImageElement.cpp:225
>  
> +    // Unregister with CachedImage when removing image element from DOM to ensure CachedImage is not holding dead references.
> +    if (m_imageLoader.image())
> +        m_imageLoader.image()->removeClient(&m_imageLoader);
> +

Currently, ImageLoader only calls CachedImage::removeClient() when it clears its pointer to the CachedImage.  Since ImageLoader holds a CachedResourceHandle<CachedImage> rather than a raw CachedImage*, this patch as written will cause the CachedImage to be keep alive by an object that can't receive its callbacks.

I don't think that's desirable, but maybe I'm missing something.  This should probably just be:
m_imageLoader.setImage(0);

[Swapna]

> Source/WebCore/html/HTMLImageElement.cpp:225
>  
> +    // Unregister with CachedImage when removing image element from DOM to ensure CachedImage is not holding dead references.
> +    if (m_imageLoader.image())
> +        m_imageLoader.image()->removeClient(&m_imageLoader);
> +


Hi , this is Swapna. I submitted patch earlier with gmail id "[e-mail]"

While I tested the  test case with above changes,  I am still able to see the issue.

[Ravi Phaneendra Kasibhatla]

Created attachment 109010 [details]
Fix for GIF not animating on dynamic detach & attach - revision 2

Addressing the comment from Nate.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #18)
> > Source/WebCore/html/HTMLImageElement.cpp:225
> >  
> > +    // Unregister with CachedImage when removing image element from DOM to ensure CachedImage is not holding dead references.
> > +    if (m_imageLoader.image())
> > +        m_imageLoader.image()->removeClient(&m_imageLoader);
> > +
> 
> 
> Hi , this is Swapna. I submitted patch earlier with gmail id "[e-mail]"
> 
> While I tested the  test case with above changes,  I am still able to see the issue.

Hi Swapna,
I have verified the previous patch & current patch on Chromium & GTK ports (on linux) and it is working. Ran layout tests also to ensure no regression with the patch.

[Swapna]

Hi ,

I tested on windows webkit with winlauncer set up with the above patch , for some reason I don't see it working.
Did any one tested on the same environment?
If I click on play continuously it is showing animation for some extent . But after that , it is not animating for click events.

[Antti Koivisto]

Who sets the m_imageLoader back if the same element is re-added to a document? Can this cause us reload the image in that case?

[Antti Koivisto]

Comment on attachment 109010 [details]
Fix for GIF not animating on dynamic detach & attach - revision 2

I suspect this can trigger network reload of the image if the element is re-added to the document (in case it is expired, has no-store, etc). I don't see why bug about restarting animation needs a change with such a broad potential side effects.

[Mustafizur Rahaman( :rahaman)]

Hi Antti Koivisto, 

Could you also please review the patch uploaded by Swapna (vswap65) where we are doing resetting the animation? We had verified that the fix is solving the issue for Windows build.

Thanks.

[Peter Kasting]

Comment on attachment 109010 [details]
Fix for GIF not animating on dynamic detach & attach - revision 2

(In reply to comment #23)
> (From update of attachment 109010 [details])
> I suspect this can trigger network reload of the image if the element is re-added to the document (in case it is expired, has no-store, etc). I don't see why bug about restarting animation needs a change with such a broad potential side effects.

Actually I think this is precisely the correct behavior.  When the image is removed from the document, it should really be removed; any caches should not still hang onto it.  If when re-added later that means we have to redownload it that's perfectly fine.

I think you should rethink your opposition to this patch.

[Peter Kasting]

Comment on attachment 103461 [details]
Added resetAnimation() fuction call in order to restart animation 

I am convinced this patch is wrong.

[Mustafizur Rahaman( :rahaman)]

(In reply to comment #26)
> (From update of attachment 103461 [details])
> I am convinced this patch is wrong.

Hi Peter,

When Swapna uploaded the patch, there were few follow-up discussion regarding your comments and she tried to answer all of your queries. So, may be we missed to realize that the fix was not proper but it would certainly help from our knowledge point of view that in what aspect, you think the patch is wrong?

[Peter Kasting]

(In reply to comment #27)
> in what aspect, you think the patch is wrong?

The fact that something like the other patch is the right way to go.  That patch makes it clear that we were not re-animating because we were not fully dropping everyone's references to the image.  We should be.

[Antti Koivisto]

(In reply to comment #25)
> Actually I think this is precisely the correct behavior.  When the image is removed from the document, it should really be removed; any caches should not still hang onto it.  If when re-added later that means we have to redownload it that's perfectly fine.

Why?

[Peter Kasting]

(In reply to comment #29)
> (In reply to comment #25)
> > Actually I think this is precisely the correct behavior.  When the image is removed from the document, it should really be removed; any caches should not still hang onto it.  If when re-added later that means we have to redownload it that's perfectly fine.
> 
> Why?

Which part are you asking why about?

The reason I think we should drop all references when removing an image from a document is that otherwise they act sort of like we're leaking them -- we aren't displaying the image anymore but we're still keeping data about it alive.

Re-adding an image to a document later does not seem qualitatively different to me than some other way of adding it, e.g. if the document simply waited ten seconds after load and then dynamically added an image.  In that case the image would clearly be subject to loading off the network etc.

The desired behavior here seems like a symptom of the fact that we want browsers to treat these sorts of dynamic re-additions consistently with other additions, and achieving it by ensuring no data structures outlive the original removal seems like the right theoretical behavior that also has the right practical effect here, as opposed to a narrowly-targeted patch to just implement re-animation, which is more of a hack.

[Antti Koivisto]

(In reply to comment #30)

> The reason I think we should drop all references when removing an image from a document is that otherwise they act sort of like we're leaking them -- we aren't displaying the image anymore but we're still keeping data about it alive.

If the HTMLImageElement is staying alive after being removed from he tree then most likely a Javascript wrapper is holding a reference to it. In that case (ignoring gc delay) there is a decent chance that it will get reattached. I see no obvious reason to dump the cached data while it is outside of the tree.

Word "leak" has specific meaning which does not apply here.

> Re-adding an image to a document later does not seem qualitatively different to me than some other way of adding it, e.g. if the document simply waited ten seconds after load and then dynamically added an image.  In that case the image would clearly be subject to loading off the network etc.

Moving an item seems qualitatively different to creating an item to me.

> The desired behavior here seems like a symptom of the fact that we want browsers to treat these sorts of dynamic re-additions consistently with other additions, and achieving it by ensuring no data structures outlive the original removal seems like the right theoretical behavior that also has the right practical effect here, as opposed to a narrowly-targeted patch to just implement re-animation, which is more of a hack.

I'm not convinced by this theory. What do other engines do?

[Mustafizur Rahaman( :rahaman)]

Is it also worth to investigate as why this patch is not solving the issue for windows port?

[Swapna]

@@ -219,6 +219,10 @@ void HTMLImageElement::removedFromDocument()
         document->removeExtraNamedItem(m_id);
     }
 
+    // Unregister with CachedImage when removing image element from DOM to ensure CachedImage is not holding dead references.
+    if (m_imageLoader.image())
+        m_imageLoader.setImage(0);
+

I have tried setting up GTK build and verified with above changes, behaviour on GTK Linux is same as Windows environment. i.e,Image stops animating immediately after few clicks.
Patch proposed by me is actually solving the issue on both Win & GTK ports, i will try to enhance the same considering Resource Leaks from the above conversation.

[Peter Kasting]

Created attachment 109358 [details]
Second testcase

(In reply to comment #31)
> (In reply to comment #30)
> > The desired behavior here seems like a symptom of the fact that we want browsers to treat these sorts of dynamic re-additions consistently with other additions, and achieving it by ensuring no data structures outlive the original removal seems like the right theoretical behavior that also has the right practical effect here, as opposed to a narrowly-targeted patch to just implement re-animation, which is more of a hack.
> 
> I'm not convinced by this theory. What do other engines do?

I've attached a second testcase that is useful for understanding this.  I tested the behavior five ways, and drew conclusions based on those as well as the fact that all engines re-animate the original testcase on hitting play:
(1) With the stock testcase.  This differs from the original in two ways.  First, we have an <img> element pointing to the same src, to test how image objects and elements that point to the same src are linked.  Second, we don't recreate the image object -- we simply dynamically add and remove it from the document.
(2) Removing the <img> element from the testcase, leaving only the image object.  This tests how a single object behaves when dynamically removed and re-added.
(3) Restoring the <img> element but changing the image object to be recreated on each call to run() (like the first testcase did).  This tests whether dropping the old image object ref is meaningful in the presence of a long-lived <img>.
(4) Moving the "var img" declaration entirely into run() and making done() do nothing, so we continually append new image objects.
(5) Removing the appendChild() call in run(), so it merely creates new image objects, but doesn't display them.  This tests whether object creation or addition to the DOM is what resets animation state.

Firefox, IE, and Opera disagree about most things here, except that they all animate all images on load.

Firefox:
(1) Nothing animates on hitting play.
(2) Nothing animates on hitting play.
(3) Both images re-animate every time you hit play.
(4) Each time you hit play, there is a new image, and all the images re-animate.
(5) Each time you hit play, the image re-animates.
Conclusion: The animation states of all the <img> elements and image objects with the same src attributes are linked.  This linked animation state is reset each time a new image object is created.  Addition to the DOM or destruction of old objects are both irrelevant, it's object creation that resets state.

IE:
(1) The image object animates on hitting play, but the <img> element does not.
(2) The image object animates on hitting play.
(3) Nothing animates on hitting play.
(4) Nothing animates on hitting play.
(5) Nothing animates on hitting play.
Conclusion: This behavior makes no sense.  Result (1) suggests that animation states for objects versus elements are tracked separately, yet result (3) (which differs from the original testcase only by the presence of an <img> element) suggests that they are linked together.  I cannot come up with an explanation for how this behavior is possible.

Opera:
(1) Nothing animates on hitting play.
(2) The image object animates on hitting play.
(3) Nothing animates on hitting play.
(4) Nothing animates on hitting play.
(5) Nothing animates on hitting play.
Conclusion: The animation states of all the <img> elements and image objects with the same src attributes are linked.  This linked animation state is reset when we add an object to the DOM, but only if there are no other linked objects in the DOM already.

Firefox and Opera both arguably make some sense.  I would probably prefer to adopt the Firefox behavior.  If we want to do that, we'll need to make some changes to our animation code.  Further testing confirms that if you create a brand new animated image object (to a src that no other object or element has), then wait, and only later add it to the DOM, Firefox begins animating it on creation, so on the later addition, the image appears midway through its animation.  In WebKit I believe we don't trigger animation to start until we first draw() the image.  This would have to change to instead start animation on object creation.  I _think_ the "image invisible, skip frames" logic in BitmapImage.cpp would mean this wouldn't have any CPU load effects, it would merely make the animation start timing be set at load instead of when first drawing, so this change would be feasible.

[Peter Kasting]

Created attachment 109359 [details]
Real second testcase

Oops, attached wrong file.

[Kishore Bolisetty]

Hi All, the above comparison and conclusions on different browser’s is quite useful. Based on the above note and some more testing, I am sharing my observations.
In WebKit also, all the resources with same src attributes are linked. Infact this is what is the CachedResource we get from docloader after cache validation.
I tried to create/append a non-animating image with same src attribute. I could clearly see the images getting appended to the document. Infact I could see the animating Image also getting appended properly to the document. But Since it is a cached Image (cached animating image) and it is already done with the animation, renderer find 0 pending frames of the animation image to paint, and hence nothing is painted except for the first time. 
I tried the following algorithm:
1. Check if the Resource being attached to DOM Tree is a cachedResource.
2. If it is a cached resource, check if it is animation Image. ( Image framecount>1 only for animation Image. So I used this to find if it’s a animating image or not) 
3. If it is a cached animating Image, only then reset the animation.
If this is making sense, I can provide the patch based on the above Algo.
Thanks.

[Peter Kasting]

That proposal wouldn't make us match Gecko.  If I understand correctly it would affect animation at the time of DOM attachment rather than image object creation; and it also wouldn't change the timings as described at the very end of comment 34.

[Kishore Bolisetty]

(In reply to comment #37)
> That proposal wouldn't make us match Gecko.  If I understand correctly it would affect animation at the time of DOM attachment rather than image object creation; and it also wouldn't change the timings as described at the very end of comment 34.

The Proposal Actually is in Sync with Gecko(for restting animation), but little deviation that favours toward webkit rendering approach.
#1> I am resetting the animation during object creation. I mean when src attribute is initialized.
#2> As you know, WebKit engine will actually start the animation while rendering and not at the time of object creation.

Even though, FireFox is resetting and starting animation at the time of Image creation, but I am in favour of WebKit behaviour as mentioned in #2 above(ie., starting animation while rendering), because there is no point in starting animation when the created image is never attached to DOM. 


Here is the svn diff of my proposal:
$ svn diff Source/WebCore/loader/ImageLoader.cpp
Index: Source/WebCore/loader/ImageLoader.cpp
===================================================================
--- Source/WebCore/loader/ImageLoader.cpp       (revision 97439)
+++ Source/WebCore/loader/ImageLoader.cpp       (working copy)
@@ -218,6 +218,12 @@

     if (RenderImageResource* imageResource = renderImageResource())
         imageResource->resetAnimation();
+
+       // TODO:: Kishore To Check if this should be done before Resource Load T
rigger from Memory Cache.
+       // TODO:: CachedResourceLoader::requestResource() -> USE -> notifyLoaded
FromMemoryCache(resource)
+       if(newImage->status() == CachedResource::Cached && newImage->image()->is
AnimationImage()) {
+                       newImage->image()->resetAnimation();
+       }
 }

 void ImageLoader::updateFromElementIgnoringPreviousError()

kbolisetty@IM-LP-093 ~/WebKit
$ svn diff Surce/WebCore/platform/graphics/Image.h
Index: Source/WebCore/platform/graphics/Image.h
===================================================================
--- Source/WebCore/platform/graphics/Image.h    (revision 97439)
+++ Source/WebCore/platform/graphics/Image.h    (working copy)
@@ -124,6 +124,7 @@
     virtual void startAnimation(bool /*catchUpIfNecessary*/ = true) { }
     virtual void stopAnimation() {}
     virtual void resetAnimation() {}
+       virtual bool isAnimationImage() const {return false;}

     // Typically the CachedImage that owns us.
     ImageObserver* imageObserver() const { return m_imageObserver; }

kbolisetty@IM-LP-093 ~/WebKit
$ svn diff Source/WebCore/platform/graphics/BitmapImage.h
Index: Source/WebCore/platform/graphics/BitmapImage.h
===================================================================
--- Source/WebCore/platform/graphics/BitmapImage.h      (revision 97439)
+++ Source/WebCore/platform/graphics/BitmapImage.h      (working copy)
@@ -126,7 +126,7 @@
     // automatically pause once all observers no longer want to render the imag
e anywhere.
     virtual void stopAnimation();
     virtual void resetAnimation();
-
+    virtual bool isAnimationImage() const { return (m_frameCount>1)?true:false;
}
     virtual unsigned decodedSize() const { return m_decodedSize; }

 #if PLATFORM(MAC)

kbolisetty@IM-LP-093 ~/WebKit

[Peter Kasting]

(In reply to comment #38)
> Even though, FireFox is resetting and starting animation at the time of Image creation, but I am in favour of WebKit behaviour as mentioned in #2 above(ie., starting animation while rendering), because there is no point in starting animation when the created image is never attached to DOM. 

Yes there is; to keep timings synced between different images.  For example, if you want to dynamically swap animated images for each other and have them be at the same point in the animation stream, this is useful.

Note that we won't actually animate the image while it's detached, we'd just want to set the start time of the animation correctly.  That will make us spin forward the animation by the right amount when we finally do draw the image.

> Here is the svn diff of my proposal:

Please don't put diffs inline.  If you want to show people a diff attach it to the bug as a patch.

[Peter Kasting]

(In reply to comment #39)
> (In reply to comment #38)
> > Even though, FireFox is resetting and starting animation at the time of Image creation, but I am in favour of WebKit behaviour as mentioned in #2 above(ie., starting animation while rendering), because there is no point in starting animation when the created image is never attached to DOM. 
> 
> Yes there is; to keep timings synced between different images.

And, of course: to match Gecko.

We want browser behaviors to converge to a single common behavior so that authors can write something once and have it work everywhere.  Gratuitously differing from other rendering engines doesn't advance this goal.  The point of documenting other engines' behavior was to try and pick a behavior that matched at least one, not to leave us in a state where we don't match anything.

[Peter Kasting]

I don't know whether fixing https://bugzilla.mozilla.org/show_bug.cgi?id=573583 has affected Firefox' behavior.  Someone might want to retry with a trunk build to see.

[Eric Seidel (no email)]

Comment on attachment 109010 [details]
Fix for GIF not animating on dynamic detach & attach - revision 2

Seems like this only fixes the issue if there is only one copy of the gif on the page.  If there is more than one copy and we only remove/re-add one of them, then that one will still not reset.  Is that expected behavior?

[Pierre Rudloff]

The same problem appears with animated SVG. Should I fill a new bug ?

[Eric Seidel (no email)]

Comment on attachment 109010 [details]
Fix for GIF not animating on dynamic detach & attach - revision 2

Need an answer to my question about multiple copies of the same gif on the page (and if we care about that case).