501183 - SVG/SMIL: Reliably handle "inherit" and "currentColor" values - compose ancestors first, & compose "color" first

Status: NEW

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

In order for animations to/from the "inherit" special value to behave reliably, we shouldn't compose any element's animations until we've composed its ancestors' animations.  (because *only then* will we know what "inherit" means at a particular point in time)

roc suggests a strategy for handling this in bug 474049 comment #34:
> This shouldn't be too hard to fix. We'd need to make nsSMILCompositorTable a
> two-level hash table, so it maps from nsIContent* to a map from (nsIAtom*,
> isCSS) to the list of nsSMILAnimationFunctions. Then iterating over it becomes
> a loop over the nsIContents, with an inner loop over the attributes of each
> element. Then break the inner loop out into its own function that does all the
> composition for an element; in that function, walk up the ancestor chain
> checking each ancestor to see if it's in nsSMILCompositorTable and not
> processed yet; if it is, recursively invoke the composition function on the
> ancestor.
> 
> In very deeply nested documents traversing all the ancestors of each animated
> element could get slow, but I don't think we have to worry about that; if we
> do, then we could add a flag on elements (as nsINode properties if necessary)
> to indicate that we've already processed an element and all its ancestors.

A related issue: in order to reliably handle animations to/from the "currentColor" special value, we need to solve the "inherit" problem as described above, and we *also* need one more thing:  For a given element, we need to compose animations on the "color" property before we compose animations on any <paint>-valued properties or attributes. (e.g. "fill", "stroke")  This could probably just be handled as a special case in the code by explicitly composing "color" before all other animated attributes.

Comment 1

[Daniel Holbert [:dholbert]]

Bug 552873 is sort of a form of this bug -- if a CSS property & its corresponding mapped-attribute are animated simultaneously, we need to make sure that we compose the mapped-attribute first, since its animated value can influence the base value of the CSS property.

(Of course, we'd still have to compose both the attribute & the CSS property on the ancestors first.)

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

should we delete the testcase: test_smilCSSInherit.xhtml as it is disabled per waiting on a fix for this bug.

Comment 4

[Daniel Holbert [:dholbert]]

It is a valid test, and this is a valid bug. Just, not a high-priority bug.

(Historical note: apparently we disabled it, rather than leaving it as "known-fail", in bug 714496 due to some intermittency.)

If we're needing to prune tests, I wouldn't strongly object to removing it. But I also don't see any harm in keeping it.)