974698 - Firefox incorrectly honors keyPoints attribute on <animateMotion>, when there is no keyTimes attribute

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:
 1. Load attached testcase.

EXPECTED RESULTS: Lime rect should move to cover up red rect.
ACTUAL RESULTS: Lime rect stops 30% of the way there. (because it's incorrectly honoring keyPoints)


Noticed while investigating bug 966634:  We currently honor the "keyPoints" attribute on animateMotion, even when keyTimes is unspecified. The spec says we should not do so.

SVG 1.1 says:
{
    If a list of ‘keyPoints’ is specified, there must be exactly as many values in the ‘keyPoints’ list as in the ‘keyTimes’ list.

    If there are any errors in the ‘keyPoints’ specification (bad values, too many or too few values), then the document is in error (see Error processing).
}
http://www.w3.org/TR/SVG11/animate.html#KeyPointsAttribute


SVG 1.2 Tiny is a bit clearer (saying explicitly to ignore the attribute):
{
If a list of 'keyPoints' is specified, there must be exactly as many values in the 'keyPoints' list as in the 'keyTimes' list.

If the 'keyPoints' attribute has a value that doesn't conform to the above requirements, the attribute has an unsupported value and the animation element is processed as if the attribute had not been specified.
}
http://www.w3.org/TR/SVGMobile12/animate.html#KeyPointsAttribute

Comment 1

[Daniel Holbert [:dholbert]]

[not sure if this has been around ever since animateMotion support was added, or was introduced more recently than that.  Adding dependency on that bug anyway, since it's related at least]

Comment 2

[Robert Longson [:longsonr]]

We don't seem to follow SVG 1.2 Tiny SMIL error handling (ignore the error) anywhere currently so perhaps that should be another bug. The SVG 1.1 document is in error seems easy to implement and is consistent with out existing error handling i.e. ignore the animation.

Comment 3

[Robert Longson [:longsonr]]

Attachment: keypoints.txt

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8408215 [details] [diff] [review]
keypoints.txt

>diff --git a/content/svg/content/src/SVGMotionSMILAnimationFunction.cpp b/content/svg/content/src/SVGMotionSMILAnimationFunction.cpp
>--- a/content/svg/content/src/SVGMotionSMILAnimationFunction.cpp
>+++ b/content/svg/content/src/SVGMotionSMILAnimationFunction.cpp
>@@ -372,30 +372,26 @@ SVGMotionSMILAnimationFunction::CheckKey
>   // attribute is ignored for calcMode="paced" (even if it's got errors)
>   if (GetCalcMode() == CALC_PACED) {
>     SetKeyPointsErrorFlag(false);
>   }
> 
>-  if (mKeyPoints.IsEmpty()) {
>-    // keyPoints attr is set, but array is empty => it failed preliminary checks
>+  if (mKeyPoints.Length() != mKeyTimes.Length()) {
>+    // there must be exactly as many keyPoints as keyTimes
>     SetKeyPointsErrorFlag(true);
>     return;
>   }

Odd -- the comment about "paced" there seems to not match what we actually do (and I can't find spec support for what that comment says, at the moment)... That's outside the scope of this bug, though, I suppose.

>diff --git a/dom/smil/test/smilAnimateMotionValueLists.js b/dom/smil/test/smilAnimateMotionValueLists.js
>--- a/dom/smil/test/smilAnimateMotionValueLists.js
>+++ b/dom/smil/test/smilAnimateMotionValueLists.js
>@@ -105,16 +105,17 @@ const gValidKeyPoints = [
>   "0; 0; 1;", // Trailing semicolons are allowed
>   "0; 0; 1; ",
>   "0; 0.000; 1",
>   "0; 0.000001; 1",
> ];
> 
> const gInvalidKeyPoints = [
>   "0; 1",
>+  "0; 0.5; 0.75; 1",

While you're here, could you add a comment to explain why these values are invalid?

Perhaps something above gValidKeyPoints, saying that any keyPoints list here must have 3 values to be valid, because test code will match them up against a keyTimes list with 3 values.

r=me with that comment. Thanks!

Comment 5

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=019c074ade51

Comment 6

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13cad5da4f9a

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/13cad5da4f9a

Comment 8

[Brian Birtles (:birtles)]

Oh, I really wanted to preserve this behavior. It's really useful. We use it in Parapara animation so that will be broken by this. I even added it to Web Animations since it's so useful and I was using support in Firefox as part of the justification for changing this. :)

Comment 9

[Brian Birtles (:birtles)]

For my own reference, here is the test I was using to compare cross-browser behavior: http://people.mozilla.org/~bbirtles/tests/keypoints.html

Comment 10

[Robert Longson [:longsonr]]

You could always back it out and wontfix this bug. It's not a big change.

Comment 11

[Brian Birtles (:birtles)]

(In reply to Robert Longson from comment #10)
> You could always back it out and wontfix this bug. It's not a big change.

I'll have a proper look at it on Monday and see how hard it is to work around. If it's not hard to work around then we can leave it in until I make the change to SVG.

Comment 12

[Brian Birtles (:birtles)]

(In reply to Robert Longson from comment #10)
> You could always back it out and wontfix this bug. It's not a big change.

I fixed Parapara so it no longer needs this. The feature I was more concerned about was the ability to specify keyPoints in any order.

When SVG animation is rebased on top of Web Animations we'll likely remove this restriction but for now it's fine.