705236 - Allow trailing separator in SMIL values list

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

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

In our parsing of the SMIL "values" attribute we make sure to reject values that end with the semicolon-separator.

Therefore, the following specification (from the bug URL) will be rejected:

  values="1 1;1 1;1 1;1.0640166083 1.1499234035;1 1;1 1;1 1;1.0640166083 1.1499234035;1 1;1 1;1 1;1.0640166083 1.1499234035;1 1;"

and the frog's legs won't swell.

The relevant code is here:
http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILParserUtils.cpp#617

I'm wondering if we should review this decision.

I haven't done a thorough investigation of the implications of this. Some questions I have yet to answer are:
1) Are there value types where an empty string is a valid value? Animated string for example?
2) Can we just fix values or are there attributes where we need to allow trailing separators in order to be consistent?

Interoperability:
Opera 11.60beta and Chrome 15 appear to ignore the trailing separator--or at least the frog's legs swell as expected.

We have a few tests that check that we reject such specifications and these would have to be updated. Specifically here:
http://mxr.mozilla.org/mozilla-central/source/content/smil/test/smilAnimateMotionValueLists.js#52

Originally when we did the SMIL parsing we made it as strict as possible (figuring that it's always easier to make it less strict in future than the other way around). For the sake of interop though it seems good to relax this.

Daniel, Cameron, Jonathan any thoughts?

Comment 1

[Cameron McCormack (:heycam)]

I believe "list of strings" is the only type where an empty string is a valid element, yes.  The list of animatable string attributes is:

  * xlink:href=""
  * class=""
  * result="", in="" and in2="" on filter primitives
  * target="" on <a>

All of those attributes have a useful meaning assigned to an empty string value.  (Well, maybe an empty xlink:href="" isn't so useful, but it does have a defined meaning.)

We could make it so that if you explicitly want an empty string value as the last item in the list, you write:

  <animate attributeName="class" values="one;two;;"/>

which would be three values; "one", "two" and "".

Can you test some implementation to see what they do with trailing single and double semicolons for string-typed animated attributes?

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) (away 21 - around 30 November) from comment #1)
> We could make it so that if you explicitly want an empty string value as the
> last item in the list, you write:
> 
>   <animate attributeName="class" values="one;two;;"/>
> 
> which would be three values; "one", "two" and "".

Just to be clear (I think you know this) -- if we were to extend our existing behavior such that we accepted empty-string values, then that would technically represent four values:
 "one, "two", "", and ""
(the three ";" characters divide the values attribute up into 4 sections)

It sounds like you're suggesting a special-case syntax, to allow empty-string values while simultaneously providing graceful behavior for semicolon-terminated lists.  I think that's reasonable, but it'd be good to have that behavior specified somewhere (if even on a www-svg message that gets tacit approval).

So to be clear, the behavior you're suggesting is (I think):
 (a) "values" is semicolon-delimited and semicolon-terminated.
 (b) if there are extra characters after the final semicolon,
     then it's assumed that there's an implied semicolon at the end.
...or something like that.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #0)
> 2) Can we just fix values or are there attributes where we need to allow
> trailing separators in order to be consistent?

We can't just do this for "values".

Firstly, we'd also need to fix keyTimes, keySplines, and keyPoints.  Like the "values" attr, these are semicolon-separated lists, whose entries correspond directly to entries in the 'values' list. (so authors who mistakenly add a trailing delimiter on "values" would likely do so on these attrs, too).

Secondly, the "begin" and "end" attributes also take semicolon-separated lists -- so it'd be a bit odd if we supported a trailing semicolon on 'values' & friends but not on begin/end.

That covers it for SVG Animation attributes, I think (based on a quick find-in-page for 'semicolon' on the animation page of the spec[1])

[1] http://www.w3.org/TR/SVG11/animate.html

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Just to be clear (I think you know this) -- if we were to extend our
> existing behavior such that we accepted empty-string values, then that would
> technically represent four values:
>  "one, "two", "", and ""
> (the three ";" characters divide the values attribute up into 4 sections)

Do we not accept empty-string values for string-typed attributes already?

But yes, with the current behaviour values="one;two;;" would be four values, but my suggestion would make that three values.

> It sounds like you're suggesting a special-case syntax, to allow
> empty-string values while simultaneously providing graceful behavior for
> semicolon-terminated lists.  I think that's reasonable, but it'd be good to
> have that behavior specified somewhere (if even on a www-svg message that
> gets tacit approval).

Yes, we should discuss it on the list.

> So to be clear, the behavior you're suggesting is (I think):
>  (a) "values" is semicolon-delimited and semicolon-terminated.
>  (b) if there are extra characters after the final semicolon,
>      then it's assumed that there's an implied semicolon at the end.
> ...or something like that.

Yes, although I would probably still want to make values="one;two; " be two values.  (So whitespace wouldn't be counted as extra characters for (b).)

(In reply to Daniel Holbert [:dholbert] from comment #3)
> That covers it for SVG Animation attributes, I think (based on a quick
> find-in-page for 'semicolon' on the animation page of the spec[1])

Yeah, I don't remember any attributes that take semicolon-separated lists other than the animation attributes.  A similar situation comes up with, say, <polygon points=""> where we might want to ignore a trailing comma.  Not sure if it is worth extending the consistency here, though.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) (away 21 - around 30 November) from comment #4)
> Do we not accept empty-string values for string-typed attributes already?

I'd initially assumed our values-list-parsing wouldn't accept them, but after glancing back at the code, I'm pretty sure it does accept empty strings, assuming the empty-string value falls in the middle of the list somewhere. (rather than at the end, where it'd look like a semicolon-terminated list)  Sorry for my confusion on that.

> Yes, although I would probably still want to make values="one;two; " be two
> values.  (So whitespace wouldn't be counted as extra characters for (b).)

Agreed -- I should have been clearer on that, sorry.  This part already falls out of the existing spec, actually:
   Leading and trailing white space, and white space before and after semicolon
   separators, will be ignored.
   http://www.w3.org/TR/SVG11/animate.html#ValueAttributes
Any tweaks we make per this bug would definitely need to be consistent with that.

> Yeah, I don't remember any attributes that take semicolon-separated lists
> other than the animation attributes.  A similar situation comes up with,
> say, <polygon points=""> where we might want to ignore a trailing comma. 
> Not sure if it is worth extending the consistency here, though.

For the record, we actually used to allow *multiple* commas at the end (*and* at the beginning) of comma-wsp-separated lists in SVG code -- we basically treated commas & whitespace as the same -- see bug 562310 comment 0 for details.  This was all just due to having ad-hoc simple/buggy tokenizing implementations.  I cleaned that up in bug 562310 -- however, if we want to relax the comma-disallowed-at-end-of-list restriction, it's a relatively simple change. (We just need to remove checks for "lastTokenEndedWithSeparator()".)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> see bug 562310 comment 0 for details.

(Note: at the end of that comment, I said that Opera 10.10 did allow a single comma at the end of comma-wsp-separated lists, at that point in time.  I assume they still do.)

Comment 7

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

(In reply to Cameron McCormack (:heycam) (away 21 - around 30 November) from comment #4)
> (In reply to Daniel Holbert [:dholbert] from comment #2)
> > I think that's reasonable, but it'd be good to
> > have that behavior specified somewhere (if even on a www-svg message that
> > gets tacit approval).
> 
> Yes, we should discuss it on the list.

Thanks for all your help. I hope you don't mind that I put your proposals on the list!

http://lists.w3.org/Archives/Public/www-svg/2011Nov/0136.html

Comment 8

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

Attachment: Test case describing proposed behaviour

Comment 9

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

Attachment: Part 1, v1a

Make us ignore trailing semi-colons for animation list attributes (or test that we already do): values, keyTimes, keySplines, keyPoints, begin, end

Comment 10

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

Attachment: Part 2, v1a

Time up test_smilTiming.xhtml and add tests for end attribute.

Comment 11

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

(In reply to Brian Birtles (:birtles) from comment #10)
> Time up test_smilTiming.xhtml and add tests for end attribute.

Err, tidy up.

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 599840 [details] [diff] [review]
Part 1, v1a

Looks great! Just a few nits on the tests:

>+++ b/content/smil/test/smilAnimateMotionValueLists.js
[...]
> const gInvalidValues = [
[...]
>-  "a", " a; ", ";a;",
>+  "a", " ;a ",

I think we can skip this change, right?  The value you removed, " a; ", should still be invalid because it's only a single value & we're expecting a pair of values in this context.  (It's equivalent to "a", which is also invalid.)

>+++ b/content/smil/test/test_smilValues.xhtml
[...]
>+  // The parsing below is based on the following discussion:
>+  //
>+  //   http://lists.w3.org/Archives/Public/www-svg/2011Nov/0136.html
>+  //
>+  // In summary:
>+  // * Values list are semi-colon delimited and semi-colon terminated.

"Values list are" wants either s/list/lists/ or s/are/is/

>+    // Run samples
>+    for (var j = 0; j < test.times.length; j++) {
>+      var times = test.times[j];

"var times" needs another name here, because
 (a) it corresponds to a single time value (not multiple)
 (b) it's confusing to have test.times[] and times[] in the same chunk of code, referring to different things. :)

Maybe "var time" or "var curTime"?

>+++ b/content/svg/content/src/SVGMotionSMILAnimationFunction.cpp
> void
> SVGMotionSMILAnimationFunction::UnsetKeyPoints()
> {
>-  mKeyTimes.Clear();
>+  mKeyPoints.Clear();
>   SetKeyPointsErrorFlag(false);
>   mHasChanged = true;
> }

Whoah, good catch!  Was this caught by one of the tests here?

r=me with the test nits

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 599842 [details] [diff] [review]
Part 2, v1a

Looks good, just a few nits:

>+++ b/content/smil/test/test_smilTiming.xhtml
>+  testCases.push(testStartTime('0', 0));
>+  testCases.push(testStartTime('0', 0));

Looks like these two testcases ^ are identical -- probably one was supposed to be different (or else removed)?

>+    if ('times' in test) {
>+      for (var j = 0; j < test.times.length; j++) {
>+        var times = test.times[j];
>+        checkSample(times[0], times[1], params);
>+      }
>+    }

As in the other patch, "var times" probably wants to be named "time" here.

>+function testStartTime(beginSpec, expectedStartTime) {
>+  return { 'attr'     : { 'begin': beginSpec },
>+           'startTime': expectedStartTime };
> }

This function would probably be better-named "constructStartTimeTest" or something like that.  ("testStartTime" is misleading since it doesn't actually test anything.)

r=me with the above

(I didn't sanity-check the values themselves supermuch - I'll just trust you (& the test's greenness) on those being correct. :) )

Comment 14

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

Attachment: Part 1, v1b r=dholbert

Thanks so much for the quick reviews Daniel!

(In reply to Daniel Holbert [:dholbert] from comment #12)
> >+++ b/content/svg/content/src/SVGMotionSMILAnimationFunction.cpp
> > void
> > SVGMotionSMILAnimationFunction::UnsetKeyPoints()
> > {
> >-  mKeyTimes.Clear();
> >+  mKeyPoints.Clear();
> >   SetKeyPointsErrorFlag(false);
> >   mHasChanged = true;
> > }
> 
> Whoah, good catch!  Was this caught by one of the tests here?

Sadly, no. I looked for somewhere to add a test but couldn't find any obvious place (plus I noticed we're not really testing keyPoints much--there's TODO in db_smilAnimateMotion.js for this).

Comment 15

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

Attachment: Part 2, v1b r=dholbert

(In reply to Daniel Holbert [:dholbert] from comment #13)
> This function would probably be better-named "constructStartTimeTest" or
> something like that.  ("testStartTime" is misleading since it doesn't
> actually test anything.)

I've gone with StartTimeTest--i.e. an noun/object since I think that describes it better (and because it's shorter).

Comment 16

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

(In reply to Daniel Holbert [:dholbert] from comment #13)
> (I didn't sanity-check the values themselves supermuch - I'll just trust you
> (& the test's greenness) on those being correct. :) )

Yeah, that's fine. Current try run:
https://tbpl.mozilla.org/?tree=Try&rev=bfd096df364d

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #15)
> I've gone with StartTimeTest--i.e. an noun/object since I think that
> describes it better (and because it's shorter).

Sounds good to me.

Comment 18

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

Pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d5d8b8121d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1e552fbde97

Comment 19

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7d5d8b8121d1
https://hg.mozilla.org/mozilla-central/rev/c1e552fbde97

as a side note, unless you need it for you own tracking purposes, there is no more the need to set [inbound] in the whiteboard.