573716 - Reconsider whether additive SMIL sandwich layers should replace lower layers if they fail to add

Status: NEW

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

I think we should reconsider whether a SMIL sandwich layer that fails to sandwich add with a lower layer should then override the lower layers. Currently that's what we do at the end of nsSMILAnimationFunction::ComposeResult:

http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationFunction.cpp#290

Unfortunately that gives bad results in this case:

  <text x="100 120" y="50">ABCD
    <animate attributeName="x" begin="0s" dur="10s"
             by="100 150 200"/>
  </text>

In this example the animation should be completely discarded because the 'by' attribute has a value that is too long, and since we don't know the position of the third glyph in the current coordinate system, we don't know what the implicit 'from' should be for the third list item.

What currently happens is that we animate as if we'd specified from="0 0 0" on the <animate>. This is clearly very wrong. It happens because when we try to sandwich add the sandwich layer from the <animate> with the base layer it fails, and so the last line in nsSMILAnimationFunction::ComposeResult overrides the base layer with the interpolation from "0 0 0" to "100 150 200" from the <animate> layer.

Comment 1

[Jonathan Watt [:jwatt]]

Daniel, Brian, any thoughts on this?

Comment 2

[Daniel Holbert [:dholbert]]

So the current code gives us "reasonable" fallback behavior, when the user accidentally provides additive="sum" to an animation that would otherwise be non-additive.  I'm not sure that's the best idea, though.  Details below.

Consider for example:
>   <text y="30" style="font-family: serif">Hello world
>    <animate attributeName="font-family" from="monospace" to="sans-serif"
>             additive="sum" begin="0.5" dur="1" fill="freeze"/>
>  </text>

The |additive="sum"| there means that the <animate>'s resulting value is supposed to add on to the animation sandwich, rather than replacing what's below it.  However, font-family doesn't support addition (as we discover when we try to add it, when we reach the code at the MXR link jwatt provided in comment 0).  So, instead of adding, we currently end up replacing the value below us in the stack. (effectively ignoring additive="sum")

This happens to match Opera's behavior, but I'm not sure it's specified anywhere, nor am I sure it's a good idea.  SMIL3 says:
> Additive animation is defined for numeric attributes and
> other data types for which an addition function is defined. 
http://www.w3.org/TR/SMIL3/smil-animation.html#animationNS-AdditiveAnim

So since additive animation is clearly *undefined* for e.g. font-family, maybe it would be better to simply fail on additive="sum" font-family animations, rather than treating them like non-additive animations.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> So the current code gives us "reasonable" fallback behavior, when the user
> accidentally provides additive="sum" to an animation that would otherwise be
> non-additive.
I meant to append this to the end of the above sentence: ... "on an attribute that doesn't support addition."

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix: if additive animation fails SandwichAdd, just drop it.

This changes our behavior to ignore the effects of any "broken" <animate> nodes that are additive but fail at adding to the sandwich.  Passes SMIL reftests & mochitests.

Comment 5

[Brian Birtles (:birtles)]

I can see the problem with the first example (comment 0), but like you say Daniel, it does seem like the "reasonable" fallback behaviour in the second example (comment 2) is desirable.

Making it even simpler still:

<text y="30" style="font-family: serif">Hello world
  <set attributeName="font-family" to="monospace" additive="sum" begin="0.5"/>
</text>

I think you'd expect to get monospace rather than getting penalised for adding a spurious 'additive' attribute right? What do you think?

I guess I was just taking "additive animation not defined" as being "non-additive animation" where SMIL Anim / SMIL 3 gives us:
  A non-additive animation simply overrides the result of all lower sandwich layers.

Another possibility is to change the behaviour of SandwichAdd for animated lists (e.g. silently fail for lists of differing lengths, or be more lenient and just add up to the minimum length of the two lists). What do other browsers do for this test case?

Comment 6

[Brian Birtles (:birtles)]

(In reply to comment #5)
> <text y="30" style="font-family: serif">Hello world
>   <set attributeName="font-family" to="monospace" additive="sum" begin="0.5"/>
> </text>

Oh wait. I forgot set can never be additive.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> Another possibility is to change the behaviour of SandwichAdd for animated
> lists (e.g. silently fail for lists of differing lengths, or be more lenient
> and just add up to the minimum length of the two lists).

I think I'd prefer that we do one of these (in the SVG length-lists patch), rather than apply the patch I attached, so that we can keep our current (interop-friendly) fallback behavior with non-additive attributes & additive="sum" animations.

jwatt, thoughts?

Comment 8

[Jonathan Watt [:jwatt]]

I don't mind ignoring additive="sum" for non-additive attributes, but it seems to me like we shouldn't do that for attributes that *are* additive.

In fact if a sandwich layer fails to add, it seems like it could be very confusing to authors to figure out what's going on if we then allow higher sandwich layers to add (so some in the middle just get skipped. Shouldn't we help authors debug by stopping at the last good addition?

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to comment #8)
> I don't mind ignoring additive="sum" for non-additive attributes, but it seems
> to me like we shouldn't do that for attributes that *are* additive.

So I see two options for modifying SandwichAdd() in failure cases for normally-additive attributes:

(1) Make SandwichAdd() fail silently for these cases, as birtles suggested (i.e. have it leave outparam untouched & return NS_OK):
  --> This would make the "bad" additive animation get ignored, effectively.

(2) Make SandwichAdd() return a special error code for these cases -- "I normally support addition, but not *this* addition!" -- and specifically check for that error code at the end of nsSMILAnimationFunction::ComposeResult, and bail out if we see it.  (This would mean we'd need to allow nsSMILAnimationFunction::ComposeResult to fail, too -- currently it returns void and can't communicate failure, I think.)
 --> This would give the behavior jwatt suggests at the end of comment 8 (stopping at the sandwich layer that fails at addition)

Comment 10

[Brian Birtles (:birtles)]

Not sure where we're up to with this but is this the same as bug 565812?

Comment 11

[Daniel Holbert [:dholbert]]

Ah, I believe it is. :) Duping that one to this, since this has more discussion.