Animations should initially be stopped (until VectorImage::StartAnimation is called) in SVG-as-an-image

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla2.0b8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Right now when we load an SVG-as-an-image, I think we default to starting animations right away (unless StopAnimations() is called) simply because that's what happens when an SVG document is loaded.

We probably should be pre-emptively stopping animations (at time 0) **until** Image::StartAnimation() is called, though.  This would make sure that SVG images that lack animation consumers when they're loaded (e.g. because their display document isn't being shown for some reason) don't needlessly animate.

(spinning this off from bug 610419 comment 5 -- see that comment for a little more context)
(Assignee)

Comment 1

7 years ago
Created attachment 489388 [details] [diff] [review]
wip

Here's an initial fix -- this just adds a call to StopAnimations() (which pauses both SVG animations and animated images inside of the helper-document) to SVGDocumentWrapper::OnStopRequest.

I'm not sure this is exactly what we want yet (we might want to time the pausing slightly differently), but posting it as a WIP for now.

Flagging bz for feedback on this assertion-tweak, for an assertion that this patch (and variants of it per my timing uncertainty above) makes us start failing, at our first animation sample on the "fish.svg" demo on my blog:

=====
@@ -3651,16 +3651,17 @@ nsContentUtils::HasMutationListeners(nsI
   nsIDocument* doc = aNode->GetOwnerDoc();
   if (!doc) {
     return PR_FALSE;
   }
 
   NS_ASSERTION((aNode->IsNodeOfType(nsINode::eCONTENT) &&
                 static_cast<nsIContent*>(aNode)->
                   IsInNativeAnonymousSubtree()) ||
+               !doc->GetScriptGlobalObject() ||
                sScriptBlockerCount == sRemovableScriptBlockerCount,
                "Want to fire mutation events, but it's not safe");
=====

This assertion starts failing because, when Imagelib officially wants us to start animating, there happens to be a script blocker set.  (And our first synchronous animation sample can trigger a HasMutationListeners() check when it modifies an animated value, and this assertion is in HasMutationListeners())

I'm pretty sure the assertion-failure is a false alarm in this situation, because we can't have any scripted mutation listeners in our helper document, since scripts are disabled.  So I've added a line to silence the assertion if scripts are disabled in our document. (Note that we have a similar check in PresShell::FlushPendingNotifications*)  bz, does that assertion-tweak look reasonable to you?

*
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?mark=4793-4793#4770
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #489388 - Flags: feedback?(bzbarsky)
Comment on attachment 489388 [details] [diff] [review]
wip

Yeah, the assert bit looks fine.

Stopping animations up front would seem like a better idea than in OnStopRequest, no?  Or do we not start them before that?
Attachment #489388 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 3

7 years ago
(In reply to comment #2)
> Yeah, the assert bit looks fine.

Great, thanks!

> Stopping animations up front would seem like a better idea than in
> OnStopRequest, no?

Yup, agreed -- that's what I meant by "we might want to time the
pausing slightly differently".

> Or do we not start them before that?

SMIL animations begin at SVGLoad-firing-time -- which can happen as late as late as (but no later than) OnStopRequest (since that method synchronously finishes any pending parsing & layout).
(Assignee)

Comment 4

7 years ago
Created attachment 490212 [details] [diff] [review]
fix

Turns out we already had code to try to accomplish this in VectorImage::OnStopRequest.  (That might be too late, though -- we may or may not have already taken our first SMIL sample at that point.)

A much more sensible place to do this is in SVGDocumentWrapper::OnStartRequest -- at that point:
 (A) we have a nsDocument
 (B) we're setting up other aspects of our nsDocument (telling it that it's an image, giving it an initially-empty viewport)

So this is a good time to also preemptively stop animations in our nsDocument.

NOTE: This doesn't include the assertion-tweak from the second half of Comment 1 -- that assertion isn't failing with this version of the fix.  (not immediately sure what caused the difference on that)
Attachment #489388 - Attachment is obsolete: true
Attachment #490212 - Flags: review?(roc)
(Assignee)

Comment 5

7 years ago
(In reply to comment #4)
> Turns out we already had code to try to accomplish this in
> VectorImage::OnStopRequest.  (That might be too late, though -- we may or may
> not have already taken our first SMIL sample at that point.)

Sorry, that wasn't quite correct -- the existing code (removed in this patch) targeted a more narrow goal: to preemptively stop animations in our helper-doc, when animations are supposed to be disabled (mAnimationMode == kDontAnimMode).

That special-case is no longer needed here, since we'll now globally stop animations even earlier (and any subsequent calls to StartAnimation will catch the kDontAnimMode and return without doing anything).
(Assignee)

Comment 6

7 years ago
NOTE: I filed Bug 611797 on sharing some related code between RasterImage & VectorImage. (which I noticed when fixing this bug)
(Assignee)

Updated

7 years ago
Attachment #490212 - Flags: approval2.0?
(Assignee)

Comment 7

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/953916da7942
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.