Closed Bug 610419 Opened 10 years ago Closed 10 years ago

animation not running in SVG-as-an-image

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: dholbert, Assigned: dholbert)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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...
blocking2.0: --- → ?
Regression is from http://hg.mozilla.org/mozilla-central/rev/04311a70fa33

Probably just need to add an "&& !IsResourceDoc()" check to the added clause there.
Attached patch fix (obsolete) — Splinter Review
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
Attachment #488955 - Flags: review?(roc)
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.
Attachment #488955 - Flags: review?(roc)
(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.
Spun off Bug 610491 for Comment 5.
Attached patch fix v2Splinter Review
Updated patch with s/IsResourceDoc/mIsBeingUsedAsImage/ (change 1 from comment 4), plus a reftest.
Attachment #488955 - Attachment is obsolete: true
Attachment #489003 - Flags: review?(roc)
Comment on attachment 489003 [details] [diff] [review]
fix v2

canceling review request while I double-check that the comment about OnPageShow / OnPageHide is correct...
Attachment #489003 - Flags: review?(roc)
Comment on attachment 489003 [details] [diff] [review]
fix v2

(ok, confirmed that the comment is correct)
Attachment #489003 - Flags: review?(roc)
(This should be blockingb8+ IMHO, since it's a behavior regression w.r.t. beta 7)
Blocks: 608295
Landed: http://hg.mozilla.org/mozilla-central/rev/66de91ff0bbd
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.