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

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: dholbert, Assigned: longsonr)

Tracking

Trunk
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Reporter

Description

5 years ago
Posted 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
Reporter

Updated

5 years ago
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
Reporter

Comment 1

5 years ago
[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
Assignee

Comment 2

5 years ago
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.
Assignee

Comment 3

5 years ago
Posted patch keypoints.txtSplinter Review
Assignee: nobody → longsonr
Attachment #8408215 - Flags: review?(dholbert)
Reporter

Comment 4

5 years ago
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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Reporter

Updated

5 years ago
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
Assignee

Comment 10

5 years ago
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.
Assignee

Updated

2 years ago
Depends on: 1333560
You need to log in before you can comment on or make changes to this bug.