508206 - SVG SMIL: relative units used in animation don't update immediately

Status: NEW

(Core :: SVG, defect)

Description

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

Attachment: Standalone test case

If we use relative units in an animation such as em, ex, or percentages then if the value that these units are relative to (e.g. the font-size on the parent, or the size of the viewport) changes, the animation values will not update until the next animation sample.

So, for example, if we have

<g font-size="10px">
  <circle...>
    <animate attributeName="cx" from="0" to="10em"..../>
  ...

Then, mid-way through the animation, 'cx' will have an animVal in user units of 50. However, if, using script, we change the font-size on the <g> to 5px and in the same script block query the animVal of the circle, we will get back '50' again. However, if we let the animation do a sample (e.g. using setTimeout) and then query, the value will be 25.

Basically, what happens is that we have a series of animation function values which are the endpoints of the intervals we interpolate between. These function values can be in any units at all. So you can have a single animation that interpolates from '5em' to '70%' to '20mm'. Obviously we need to convert this to some canonical form for interpolation. This we do in user units.

When we store the result of the animation in the animVal we store it in the same units as the baseVal because it's convenient. (There's probably a bug lying here as well in that if the base value is in em-units but the animation values are not--we could get into an inconsistent state when the context font-size has been changed but the animation has not yet been resampled.) In any case, even if we stored the value for animVal in user units we'd run into the same problem because the final value is derived from other values which may use relative units. Therefore we need to resample the animation whenever the relative base of any of the units used in the animation changes.

Ideally we'd just like to do a minimal resample of just the affected animations but I don't think we have the facility to do this at the moment.

I'm not quite sure how to fix this. We probably don't want to resample the whole animation every time an animated length is queried by the DOM. That sounds bad. Could we:
(i) Detect if any font-size or viewport changes in the entire SVG document fragment
(ii) Detect if there is any animation being used in the document fragment
And then if those two check prove true, mark the document as needing resample (we already have and use the facility for marking the document as needing resample).

Comment 1

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

Attachment: Mochitest adapted to automated running

This is just a variation of the standalone test case suitable for inclusion in test suite. As it uses 'display: none' it is broken by bug 507067.

Comment 2

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

My outline of a proposed solution is not quite right for part (ii). We already have a means of marking the animation as needing a resample (nsSVGElement::AnimationNeedsResample) which is pretty cheap if there's no animation.

It's (i) that's the problem. I'm not sure how to detect all the possible ways font-size could be changed in order to set the flag. I guess there are a few ways:
a) By setting font-size on an SVG element.
b) By setting style="... font-size: ..." on an SVG element.
c) By updating the content of an SVG <style> element.
d) By updating the content of an XHTML <style> element in a containing document?
e) By animation?

So we'd need to call AnimationNeedsResample in any of those cases.

It's probably easier to detect when the viewBox changes.

Comment 3

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

Attachment: Test case patch (failing test case only)

Failing test case. Not sure that it needs review, but thought I'd run this by you.

Comment 4

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

Discussed this with dholbert yesterday. Decided it's very low priority for now. Other browsers don't currently support any kind of DOM liveness for animation so we're already one up there.

Also, my description of things that might cause the font-size to change above is pretty inaccurate. For a start '(e) By animation' is not needed. And there are many other ways the effective font-size could change such as by tree surgery or even just by changing the id on a parent element that matched some CSS selector.

Furthermore, this issue applies to more than just lengths, but also to, e.g., animation that uses the currentColor keyword.

For now, we suggest just checking in this failing test case (i.e. uses 'todo').

Comment 5

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

Please check in the patch (testcase only) but don't mark the bug as resolved.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't understand how this is detecting SMIL support.

Comment 7

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

Attachment: Test case patch v1b

Updated testing for SMIL support. Please check in this version.

Comment 8

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/c6b896d1d3bd