Closed Bug 763784 Opened 7 years ago Closed 4 years ago

VectorImage::GetAnimated should check for CSS animations

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: dholbert, Assigned: jwatt)

References

Details

Attachments

(1 file)

Right now, VectorImage::GetAnimated (implementing the readonly boolean attribute "imgIContainer::animated") just checks whether we have SMIL animations registered.

It should check for CSS animations as well.

RELEVANT CODE:
VectorImage::GetAnimated()
 http://mxr.mozilla.org/mozilla-central/source/image/src/VectorImage.cpp#346

...which calls SVGDocumentWrapper::IsAnimated()
 http://mxr.mozilla.org/mozilla-central/source/image/src/SVGDocumentWrapper.cpp#144

The latter is what we should fix.

NOTE: To the extent that anyone cares about this attribute's value, I think we should err on the side of returning "true" ("yes, it's animated"), if we can't tell.  (e.g. if there are animations in a stylesheet but they're not used).

False positives shouldn't trip up consumers -- they'll just look like animated images whose frames all happen to be identical.  But false negatives could potentially be bad, if an extension or some of our chrome code wants to do something special for animated images and incorrectly flags us as static.

However, we should watch out for cases of e.g. ua.css getting an animation registered on in all documents, for one obscure element, that only plays under special circumstances.  That sort of thing could suddenly make this function start returning true unconditionally, which would be bad.
I don't know exactly what invocations we'd need to check for CSS animations.

dbaron says it shouldn't be too hard, but we do need to be sure to wait until style has been resolved. I think that should be covered by our FlushLayout() call in SVGDocumentWrapper::OnStopRequest().
Attached patch patchSplinter Review
I wrote this as part of bug 1190881, but that bug still needs work to fix other issues. Brian made the suggestion about adding the new function to DocumentTimeline so I guess I should get a different reviewer.
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Attachment #8722239 - Flags: review?(dholbert)
Looks good to me (although you might want to adjust the comment to say "CSS animation/transitions/Web animations" or something since we now support script-generated animations using the same mechanism).
(In reply to Brian Birtles (:birtles) from comment #3)
> Looks good to me (although you might want to adjust the comment to say "CSS
> animation/transitions/Web animations" or something since we now support
> script-generated animations using the same mechanism).

We don't support scripts inside of SVG images, so it might also be correct to avoid mentioning the Web Animations api here.
Comment on attachment 8722239 [details] [diff] [review]
patch

Review of attachment 8722239 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this!

::: image/SVGDocumentWrapper.cpp
@@ +120,5 @@
> +          // CSS animations/transitions:
> +         ((doc->Timeline()->HasAnimations()) ||
> +          // SMIL animations:
> +          (doc->HasAnimationController() &&
> +           doc->GetAnimationController()->HasRegisteredAnimations()));

This return statement is now sufficiently-complex (6 lines, multiple comments, multiple nested conditions) that we might as well split it up into separate returns, IMO -- for readability, as well as for debuggability in cases where SVGDocumentWrapper::IsAnimated() unexpectedly returns the wrong value.

Please split into something like:
  if (!doc) {
    return false;
  }

  if (doc->Timeline()->HasAnimations()) {
    return true; // CSS animations/transitions
  }
  if (doc->HasAnimationController() &&
      doc->GetAnimationController()->HasRegisteredAnimations()) {
    return true; // SMIL animations
  }
  return false;


Also: I suspect it's impossible for there to be CSS transitions inside of an SVG image, since without scripting or interactivity, there's no way to change a CSS property in a way that triggers a transition. So, we might even want to drop "transitions" from the comment here. Not a big deal though; I'm fine keeping it, as well, for completeness about what sorts of thing the Timeline() API is querying.
Attachment #8722239 - Flags: review?(dholbert) → review+
(I should say: I haven't vetted the Timeline()->HasAnimations() API, but I'm assuming you & Brian have each sanity-checked that it's the right thing to be using here.)
(In reply to Daniel Holbert [:dholbert] from comment #6)
> (I should say: I haven't vetted the Timeline()->HasAnimations() API, but I'm
> assuming you & Brian have each sanity-checked that it's the right thing to
> be using here.)

On IRC Brian noted that the timeline is our one point of attachment to the refresh driver (it tells us whether we need to run the refresh driver or not) so it seem like exactly the right place to check in this case too.
https://hg.mozilla.org/mozilla-central/rev/4b13eb25130f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.