Closed Bug 974698 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: dholbert, Assigned: longsonr)

References

Details

Attachments

(2 files)

Attached file 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
Summary: <animateMotion> incorrectly honors keyPoints attribute when there is no keyTimes attribute → Firefox incorrectly honors keyPoints attribute on <animateMotion>, when there is no keyTimes attribute
[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]
Depends on: animateMotion
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.
Attached patch keypoints.txtSplinter Review
Assignee: nobody → longsonr
Attachment #8408215 - Flags: review?(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!
Attachment #8408215 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/13cad5da4f9a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Blocks: 1017871
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. :)
For my own reference, here is the test I was using to compare cross-browser behavior: http://people.mozilla.org/~bbirtles/tests/keypoints.html
You could always back it out and wontfix this bug. It's not a big change.
(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.
(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.
Depends on: 1333560
You need to log in before you can comment on or make changes to this bug.