1189185 - Don't call AddPendingRestyle for SVG/SMIL animations that are targeting non-mapped attributes (which are unrelated to style)

Status: NEW

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Load attached testcase, which has 2 SMIL animations:
   - One <animate> targeting "height" on a <rect> (which is completely *unrelated* to CSS). 
   - One <animate> targeting "stroke-width" on a different <rect> (which is backed by CSS).

 2. In gdb, place a breakpoint in nsSMILAnimationController::AddStyleUpdatesTo

 3. Note the calls to aTracker.AddPendingRestyle()

EXPECTED RESULTS:
We should make a call to AddPendingRestyle each refresh-driver tick for the element that's being animated with "stroke-width", but not for the other one that's animating the "height" attribute.

ACTUAL RESULTS:
We make a call to AddPendingRestyle for each of our animations / animation-targets -- regardless of whether the thing being animated is related to CSS.

Setting as depends-on bug 960465, since that's where we added this "AddStyleUpdatesTo" function:
http://hg.mozilla.org/mozilla-central/rev/0288ff191edf#l2.30

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: pseudo-reference-case 1 (without the stroke-width animation)

Here's a reference case, from a "what code gets run for our 'height' animation" perspective (not from a rendering perspective).

This reference case doesn't have the stroke-width animation -- it just has the 'height' animation. So, since no style-related stuff is being updated, we never get a call to PostRestyleEvent(), which means we never visit nsSMILAnimationController::AddStyleUpdatesTo() & add a pending restyle for our animated element. (This is good.) And everything still animates correctly.

In comparison, when we load testcase 1, we get a restyle event for the stroke-width animation, which makes us end up in nsSMILAnimationController::AddStyleUpdatesTo, which makes us unnecessarily call AddPendingRestyle for the rect whose (unrelated-to-CSS) height attribute is being animated.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

In bug 1171966 I tried only to add style updates when we have an outdated mapped attribute but I ran into a bunch of test failures. The problem, however, was that I was only dealing with mapped attributes in the narrow sense of the word (i.e. using attributeType="XML", I think, or whatever nsSMILMappedAttribute represents). You can see the attempt I made here: https://hg.mozilla.org/try/rev/b1df7dcf748e#l1.105

Perhaps we can build on bug 1171966 and just set the "has pending style updates" flag when we do a compose that involves an instance of nsSMILCSSProperty (or subclass such as nsSMILMappedAttribute). I suspect that might not work for some cases such as when an animation finishes or is removed but we might be able to use the mLastCompositorTable to deal with that case.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #3)
> The problem,
> however, was that I was only dealing with mapped attributes in the narrow
> sense of the word (i.e. using attributeType="XML", I think, or whatever
> nsSMILMappedAttribute represents).

Huh -- nsSMILMappedAttribute is indeed the right thing to be looking for, I think.  (That and nsSMILCSSProperty [for attributeType="CSS"] are the two types of SMIL animation targets that can influence style.)

What was the problem you were hitting with this strategy?

(Also: I think we probably shouldn't worry about handling any of this mapped-attribute stuff in bug 1171966's patches; we should keep those patches as safe-and-backportable as possible.)

> Perhaps we can build on bug 1171966 and just set the "has pending style
> updates" flag when we do a compose that involves an instance of
> nsSMILCSSProperty (or subclass such as nsSMILMappedAttribute).

That seems reasonable.   (Alternately, we can just make nsSMILAnimationController::AddStyleUpdatesTo cheap in the "no animations that affect style" case. But, better to prevent it from being called at all if we can.)

> I suspect
> that might not work for some cases such as when an animation finishes or is
> removed but we might be able to use the mLastCompositorTable to deal with
> that case.

I think the finishing/animation-removed cases are already handled via our "ClearAnimationEffects()" calls on the residual entries in mLastCompositorTable.

Setting the "has pending style updates" flag for those animations won't help, because that just gets us a call to nsSMILAnimationController::AddStyleUpdatesTo, which can't see those stale/removed animations.  (So the hypothetical bug you're describing seems like something we'd already be hitting, if it were going to be a problem.)