Closed Bug 610419 Opened 12 years ago Closed 12 years ago
animation not running in SVG-as-an-image
Something from this push... http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=17b72e252d83 seems to have broken animations in SVG-as-an-image, in nightly builds. e.g. load http://blog.dholbert.org/2010/10/svg-as-image.html -- the fish doesn't move in today's nightly. Investigating...
Regression is from http://hg.mozilla.org/mozilla-central/rev/04311a70fa33 Probably just need to add an "&& !IsResourceDoc()" check to the added clause there.
Yup, that fixes it. I guess resource documents are "hidden" as far as mIsHidden is concerned -- makes sense. I'd initially thought that any test for animated SVG-as-an-image would have to rely on setTimeout, but I think I can test it without timeouts using mozRequestAnimationFrame/MozBeforePaint/MozAfterPaint. Working up a test right now...
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
What about animated images in resource documents where the display document is not showing? Are they still stopped with this patch?
Comment on attachment 488955 [details] [diff] [review] fix First, I'll change your question slightly, replacing "display document is not showing" with "display document gets a call to OnPageHide". (I'll address the "display document was never showing in the first place" case aftwerward.) So: Do animated images get stopped in resource documents where the display document gets a call to OnPageHide? For SVG-as-an-image resource documents, the answer is: "Yes, if the display document was the only/last animation consumer for our SVG image." When the display document becomes not showing (via OnPageHide), it calls SetImagesNeedAnimating(PR_FALSE), which calls DecrementAnimationConsumers(), which (if no animation consumers are left for our image) calls SVGDocumentWrapper::StopAnimation, and halts animations in the helper-doc. That all should work correctly since bug 606942. For external resource documents: actually, it looks like we don't want the patch to affect those -- nsDocument::OnPageHide / OnPageShow already forwards PageHide/PageShow calls to its external resource documents -- so they already get unpaused whenever their display document is shown, and re-paused whenever their display document is hidden. So, I think this patch should actually just check "IsBeingUsedAsImage" instead of "IsResourceDoc".[change 1] ======= Now, if the display document was never showing in the first place (unexpected, but was the case in Bug 608295 which regressed this), then: For SVG-as-an-image resource documents: we probably should pause animations in our helper-document with PAUSE_IMAGE. [change 2] For external resource documents: they will behave the same as the display document (never have their animations started in the first place), because their mIsShowing status is kept in sync with the display document. (The original patch here might have broken that, but [change 1] noted above will keep that from breaking). I'll repost the patch with [change 1] & [change 2] noted above, and a reftest.
(In reply to comment #4) > Now, if the display document was never showing in the first place (unexpected, > but was the case in Bug 608295 which regressed this), then: > > For SVG-as-an-image resource documents: we probably should pause animations in > our helper-document with PAUSE_IMAGE. [change 2] ...though (d'oh) of course we have no access to our display document (nor do we necessarily have any single unique display document) for SVG-as-an-image. So -- instead "[change 2]" -- to keep SVG-as-an-image from animating in cases where there's only one display document and that display document never gets shown, I think we might want to start off our helper-document as being paused with PAUSE_IMAGE, and then only unpause it when we get the call to SVGDocumentWrapper::StartAnimation() (e.g. from an animation consumer being added). I'd prefer to make that change in a separate bug, though, since I think it's slightly different in scope and intent from this bug's fix.
Updated patch with s/IsResourceDoc/mIsBeingUsedAsImage/ (change 1 from comment 4), plus a reftest.
Comment on attachment 489003 [details] [diff] [review] fix v2 canceling review request while I double-check that the comment about OnPageShow / OnPageHide is correct...
Comment on attachment 489003 [details] [diff] [review] fix v2 (ok, confirmed that the comment is correct)
(This should be blockingb8+ IMHO, since it's a behavior regression w.r.t. beta 7)
Attachment #489003 - Flags: review?(roc) → review+
blocking2.0: ? → beta8+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.