Closed Bug 1333560 Opened 5 years ago Closed 5 years ago

make keyTimes attribute optional, on <animateMotion keyPoints="...">

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

Attachments

(1 file, 3 obsolete files)

Per https://www.w3.org/TR/2001/REC-smil-animation-20010904/#AnimFuncCalcMode

"If calcMode is set to "discrete", "linear" or "spline", but the keyTimes
attribute is not specified, the values in the values attribute are assumed to
be equally spaced through the animation duration, according to the calcMode.

We did implement this but broke it when we (OK I) implemented the restriction that when keyTimes is specified it must match keyPoints.
Attached patch keytimes.txt (obsolete) — Splinter Review
I'll look into adjusting smilAnimateMotionValueLists.js to test this.
Assignee: nobody → longsonr
Attachment #8830025 - Flags: review?(dholbert)
Blocks: 974698
Attached patch something like this should do (obsolete) — Splinter Review
Attachment #8830036 - Flags: review?(dholbert)
Sorry for the review delay! It wasn't immediately obvious from the bug comments here what the problem being fixed is, but let me lay out what I think you're fixing, based on the patch:

It looks like you're wanting us to gracefully ignore <animateMotion> "keyPoints" attributes, when keyTimes is unspecified, and continue to run the animation as if "keyPoints" were unspecified -- is that correct?  (It looks like that's what we were perhaps intending per the SVG 1.2 Tiny quote in bug 974698 comment 0, RE processing "as if the attribute had not been specified.")

If I'm understanding correctly: I'm not sure this change is actually desirable.

(1) From a spec perspective:
 - The SVG2 spec (the newest spec text on this) 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". And IIRC we tend to treat "in error" as "skip the animation".  So I don't immediately see a compelling spec reason that this should change.

(2) From an interop perspective: I think we're currently interoperable with Chrome on this point of behavior. So from an interop perspective, it's not clear that we should change (and there might even be content that accidentally depends on this currently-interoperable behavior).

What do you think?
(Sorry, I meant to include a spec link for my keyPoints spec-quote above: https://svgwg.org/specs/animations/#KeyPointsAttribute )

Intuitively, I agree that "keyPoints" is kinda like "values" and should behave similarly in its interactions with other attributes. However, that doesn't seem to be what the spec says.

The SVG2 spec does have permissive spec text about "values"/"keyTimes":
> For animations specified with a ‘values’ list, the ‘keyTimes’
> attribute ***if specified*** must have exactly as many values
> as there are in the ‘values’ attribute.
https://svgwg.org/specs/animations/#KeyTimesAttribute

(emphasis added around "if specified")
So, keyTimes is clearly optional there.

But for keyPoints, the spec text is not so permissive. The entire keyPoints section seems to assume that "keyTimes" is nonempty, and there's no "if specified" qualifier there.

So: IIUC, our current behavior matches the spec and is interoperable with the dominant browser on this point, and this isn't causing known issues in the wild, so I'm tempted to say we should leave this as-is.

Let me know if there's some subtlety that I'm missing, though!
Flags: needinfo?(longsonr)
Attached image testcase 1 (does the orange rect move?) (obsolete) —
Here's a testcase for this bug.
* In current Firefox & Chrome, the orange rect's animateMotion is ignored entirely (which I think is correct, as described above).  
* With this bug's patch applied, the orange rect animates like the other rects.
(added a viewBox to the testcase, so that its last rect won't be clipped in Bugzilla's lightbox image viewer)
Attachment #8832308 - Attachment is obsolete: true
There's two issues here. 

a) if calcMode="paced" we should ignore keyTimes, but we don't because of a missing return.

https://svgwg.org/specs/animations/#ValueAttributes says

If the interpolation mode is 'paced', the ‘keyTimes’ attribute is ignored.

b) if calcMode != "paced" and we omit keyTimes altogether the SVG 1.1 specification says:

https://www.w3.org/TR/2001/REC-smil-animation-20010904/#AnimFuncCalcMode

If calcMode is set to "discrete", "linear" or "spline", but the keyTimes attribute is not specified, the values in the values attribute are assumed to be equally spaced through the animation duration, according to the calcMode:

That text is removed from SVG 2 and a link to the SMIL specification replaces it:

Except for any SVG-specific rules explicitly mentioned in this specification, the normative definition for this attribute is the SMIL Animation specification. In particular, see SMIL Animation: 'calcMode' attribute ([SMILANIM], section 3.2.3).

The SMIL specification then has that same text that the SVG 2 specification removed. So per that we still have to do it.

As for cross-browser...

There are tests we fail now that we used to pass e.g.: http://hoffmann.bplaced.net/svgtest/animatemotionkeypoints06.svg

Opera 12 and Batik Squiggle 1.7 apparently animate it as did the Adobe SVG plugin. It's true that Chrome/Safari don't currently support this though.
Flags: needinfo?(longsonr)
(In reply to Robert Longson from comment #7)
> a) if calcMode="paced" we should ignore keyTimes, but we don't because of a
> missing return.

Are you saying that keyTimes="bogusValue" currently causes the animation to be dropped, but really (in paced mode) we should be permissive & just ignore the bogus keyTimes attribute?

If so: this part seems reasonable, I think.

> b) if calcMode != "paced" and we omit keyTimes altogether the SVG 1.1
> specification says:
[...]
> The SMIL specification then has that same text that the SVG 2 specification
> removed. So per that we still have to do it.

We have to do what? I'm not clear on that, from your quotes here.

You didn't mention keyPoints at all in comment 7, but IIUC that's really what we're discussing here -- we're talking about whether keyPoints="..." makes an animation invalid, when keyTimes is absent -- right?

If that's indeed what we're discussing, then here are my counterpoints:
 - keyPoints was introduced in SVG -- it's not mentioned in the SMIL spec. So the most (only) normative text on it is in SVG2.
 - And the relevant SVG2 spec text on it (quoted in comment 3) clearly says "there must be exactly as many values in the ‘keyPoints’ list as in the ‘keyTimes’ list", and otherwise "the document is in error".
 - So what does "keyTimes list" mean here?  Every mention I can find in the SVG/SMIL specs seems to be referring to the *actual author-provided "keyTimes" attribute value*.

I think maybe your point in "b" here is to say that the "values...equally spaced through the animation duration" spec-text should be interpreted as generating an *implicit* "keyTimes list", which satisfies the keyPoints requirement -- is that what you're getting at?  If so, that's stretching things a bit, spec-wise.  The specs seem to consistently use "keyTimes list" to refer to the actual keyTimes attribute value.  And the keyPoints spec-text has several other even-more-explicit references to the keyTimes attribute:
 # ‘keyPoints’ ... indicates how far [...] at the moment in time
 # specified by corresponding ‘keyTimes’ value.          <--- pretty clearly refers to the actual keyTimes attribute-val
 # [...]
 # Each progress value in the list corresponds to a
 # value in the ‘keyTimes’ attribute list.               <--- pretty clearly refers to the actual keyTimes attribute-val

> There are tests we fail now that we used to pass e.g.: http://hoffmann.bplaced.net/svgtest/animatemotionkeypoints06.svg

I think it was a bug that we passed that test.  There, the <animateMotion> element doesn't even have a "values" list, so I don't see a direct line from your (b) argument to that testcase's behavior.

> Opera 12 and Batik Squiggle 1.7 apparently animate it as did the Adobe SVG
> plugin. It's true that Chrome/Safari don't currently support this though.

Given market share (particularly on mobile but also on desktop), Chrome/Safari are the most important interop considerations at this point.
OK, so we WONTFIX this and I'll raise a separate bug for part a) alone that calcMode="paced" means that invalid keyTimes attributes are ignored.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
Attachment #8830025 - Attachment is obsolete: true
Attachment #8830025 - Flags: review?(dholbert)
Attachment #8830036 - Attachment is obsolete: true
Attachment #8830036 - Flags: review?(dholbert)
Sounds good - thanks!  [--> Clarifying bug summary slightly, too.]
Summary: keyTimes attribute should be optional → make keyTimes attribute optional, on <animateMotion keyPoints="...">
You need to log in before you can comment on or make changes to this bug.