555026 - SVG/SMIL: 'keyTimes' attribute should be ignored when calcMode="paced"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase

Quoting SVG 1.1 section 19.2.7:
> If the interpolation mode is "paced", the keyTimes attribute is ignored.

Currently, our behavior doesn't match that statement -- we're influenced by the keyTimes, even with calcMode="paced"

See attached testcase.

EXPECTED BEHAVIOR: Grey block should drop at a constant speed for 2 seconds.
ACTUAL BEHAVIOR: Grey block drops very quickly for 0.2 seconds, and then drops slowly after that.

mozilla-central shows ACTUAL BEHAVIOR:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a4pre) Gecko/20100325 Minefield/3.7a4pre

Opera 10.10 shows EXPECTED BEHAVIOR:
Opera/9.80 (X11; Linux i686; U; en) Presto/2.2.15 Version/10.10

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> Quoting SVG 1.1 section 19.2.7:
> > If the interpolation mode is "paced", the keyTimes attribute is ignored.
[ http://www.w3.org/TR/SVG11/animate.html#ValueAttributes ]

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Here's another testcase.  The animations here vary in their 'keyTimes' attribute (and most have invalid values for 'keyTimes'), but since they're all paced-mode, they should ignore keyTimes altogether and all behave the same.

Right now, they all behave differently.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a4pre) Gecko/20100405 Minefield/3.7a4pre

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (spams "ASSERTION: invalid array index")

This testcase (with non-numeric keyTimes value & calcMode="paced") spams these assertions:
###!!! ASSERTION: invalid array index: 'i < Length()', file ../../dist/include/nsTArray.h, line 324

I get the same assertion-spam if I use an empty keyTimes attribute, and also if I use any out-of-bounds values in my keyTimes list. (e.g. "-2" or "1.01").  I think in all these cases, we're ending up with an empty mKeyTimes array, which we should be ignoring (but we're not).

Comment 4

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

Daniel, the paced and discrete animation were fairly quickly knocked together by someone so I'm not surprised at all if they're not completely conformant. I'd be fairly suspicious that part of the code with regards to spec conformance.

Comment 5

[Daniel Holbert [:dholbert]]

Yeah -- I think that that someone was me, for paced-mode animation. :) (and cdouble for discrete-mode animation)

I've got a patch in hand for this; I want to finish off a comprehensive mochitest for it, and I'll post it first thing tomorrow morning.

Comment 6

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

Oops :) Sorry, I thought it was longer ago than that! I'm sure, except for this minor detail, it's perfect!

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix

Here's a fix, with a crashtest (for the assertion-spam) + mochitest (to check that animated value isn't affected by |keyTimes| -- valid or invalid -- in calcMode="paced").

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 437241 [details] [diff] [review]
fix

Requesting review.

Patch basically just ensures that we're not calcMode="paced" before applying the effects of our keyTimes attribute (in ScaleSimpleProgress) and keySplines attribute (in ScaleIntervalProgress).

Comment 9

[Daniel Holbert [:dholbert]]

Fixed: http://hg.mozilla.org/mozilla-central/rev/5af7d9cc4097