Bug 705236 part 1 - Allow trailing separator in SMIL values list; r=dholbert

content/smil/nsSMILParserUtils.cpp

@@ -615,11 +615,6 @@ nsSMILParserUtils::ParseValuesGeneric(const nsAString& aSpec,
     }
   }
 
-  // Disallow ;-terminated values lists.
-  if (tokenizer.lastTokenEndedWithSeparator()) {
-    return NS_ERROR_FAILURE;
-  }
-
   return NS_OK;
 }
 

content/smil/nsSMILTimedElement.cpp

@@ -55,6 +55,7 @@
 #include "nsString.h"
 #include "mozilla/AutoRestore.h"
 #include "mozilla/Util.h"
+#include "nsCharSeparatedTokenizer.h"
 
 using namespace mozilla;
 
@@ -1271,10 +1272,6 @@ nsSMILTimedElement::SetBeginOrEndSpec(const nsAString& aSpec,
                                       bool aIsBegin,
                                       RemovalTestFunction aRemove)
 {
-  PRInt32 start;
-  PRInt32 end = -1;
-  PRInt32 length;
-  nsresult rv = NS_OK;
   TimeValueSpecList& timeSpecsList = aIsBegin ? mBeginSpecs : mEndSpecs;
   InstanceTimeList& instances = aIsBegin ? mBeginInstances : mEndInstances;
 
@@ -1282,17 +1279,20 @@ nsSMILTimedElement::SetBeginOrEndSpec(const nsAString& aSpec,
 
   AutoIntervalUpdateBatcher updateBatcher(*this);
 
-  do {
-    start = end + 1;
-    end = aSpec.FindChar(';', start);
-    length = (end == -1) ? -1 : end - start;
+  nsCharSeparatedTokenizer tokenizer(aSpec, ';');
+  if (!tokenizer.hasMoreTokens()) { // Empty list
+    return NS_ERROR_FAILURE;
+  }
+
+  nsresult rv = NS_OK;
+  while (tokenizer.hasMoreTokens() && NS_SUCCEEDED(rv)) {
     nsAutoPtr<nsSMILTimeValueSpec>
       spec(new nsSMILTimeValueSpec(*this, aIsBegin));
-    rv = spec->SetSpec(Substring(aSpec, start, length), aContextNode);
+    rv = spec->SetSpec(tokenizer.nextToken(), aContextNode);
     if (NS_SUCCEEDED(rv)) {
       timeSpecsList.AppendElement(spec.forget());
     }
-  } while (end != -1 && NS_SUCCEEDED(rv));
+  }
 
   if (NS_FAILED(rv)) {
     ClearSpecs(timeSpecsList, instances, aRemove);

content/smil/test/Makefile.in

@@ -91,6 +91,7 @@ _TEST_FILES = \
 	  test_smilTiming.xhtml \
 	  test_smilTimingZeroIntervals.xhtml \
 	  test_smilUpdatedInterval.xhtml \
+	  test_smilValues.xhtml \
 	  test_smilXHR.xhtml \
 	  $(NULL)
 

content/smil/test/smilAnimateMotionValueLists.js

@@ -40,6 +40,8 @@
 /* Lists of valid & invalid values for the various <animateMotion> attributes */
 const gValidValues = [
   "10 10",
+  "10 10;",  // Trailing semicolons are allowed
+  "10 10;  ",
   "   10   10em  ",
   "1 2  ; 3,4",
   "1,2;3,4",
@@ -49,7 +51,6 @@ const gValidValues = [
 
 const gInvalidValues = [
   ";10 10",
-  "10 10;",  // We treat semicolon-terminated value-lists as failure cases
   "10 10;;",
   "1 2 3",
   "1 2 3 4",
@@ -128,3 +129,30 @@ const gValidPathWithErrors = [
  "m0 0 L30,,30",
  "M10 10 L50 50 abc",
 ];
+
+const gValidKeyPoints = [
+  "0; 0.5; 1",
+  "0;.5;1",
+  "0; 0; 1",
+  "0; 1; 1",
+  "0; 0; 1;", // Trailing semicolons are allowed
+  "0; 0; 1; ",
+  "0; 0.000; 1",
+  "0; 0.000001; 1",
+];
+
+const gInvalidKeyPoints = [
+  "0; 1",
+  "0; 1;",
+  "0",
+  "1",
+  "a",
+  "",
+  "  ",
+  "0; -0.1; 1",
+  "0; 1.1; 1",
+  "0; 0.1; 1.1",
+  "-0.1; 0.1; 1",
+  "0; a; 1",
+  "0;;1",
+];

content/smil/test/test_smilAnimateMotionInvalidValues.xhtml

@@ -64,6 +64,13 @@ function testAttr(aAttrName, aAttrValueArray, aIsValid, aIsTodo)
       // our value is rejected.
       anim.setAttribute("rotate", Math.PI/4);
       componentsToCheck = CTMUtil.CTM_COMPONENTS_ALL;
+      if (aAttrName == "keyPoints") {
+        // Add three times so we can test a greater range of values for
+        // keyPoints
+        anim.setAttribute("values", "0 0; 25 25; 50 50");
+        anim.setAttribute("keyTimes", "0; 0.5; 1");
+        anim.setAttribute("calcMode", "discrete");
+      }
     }
 
     var curCTM = gRect.getCTM();
@@ -158,6 +165,9 @@ function main()
   testAttr("path", gInvalidPath, false, false);
   testAttr("path", gValidPathWithErrors, true, false);
 
+  testAttr("keyPoints", gValidKeyPoints, true, false);
+  testAttr("keyPoints", gInvalidKeyPoints, false, false);
+
   testMpathElem(gValidPath, true, false);
   testMpathElem(gInvalidPath, false, false);
 

content/smil/test/test_smilTiming.xhtml

@@ -43,7 +43,8 @@ function main() {
   is(svg.getCurrentTime(), 0, "should be paused at 0 in <svg> load handler");
 
   var tests =
-    [ testOffsetStartup,
+    [ testListSyntax,
+      testOffsetStartup,
       testMultipleBegins,
       testNegativeBegins,
       testSorting
@@ -62,6 +63,40 @@ function checkSample(time, expectedValue) {
   is(circle.cx.animVal.value, expectedValue);
 }
 
+function getStartTime(anim) {
+  var startTime;
+  try {
+    startTime = anim.getStartTime();
+  } catch(e) {
+    if (e.code == DOMException.INVALID_STATE_ERR) {
+      startTime = 'none';
+    } else {
+      ok(false, "Unexpected exception: " + e);
+    }
+  }
+  return startTime;
+}
+
+function testListSyntax() {
+  var specs = [ [ '0', 0 ],
+                [ '3;', 3 ],
+                [ '3; ', 3 ],
+                [ '3 ; ', 3 ],
+                [ '3;;', 'none' ],
+                [ '3;; ', 'none' ],
+                [ ';3', 'none' ],
+                [ ' ;3', 'none' ],
+                [ '3;4', 3 ],
+                [ ' 3 ; 4 ', 3 ] ];
+  for (var i = 0; i < specs.length; i++) {
+    var anim = createAnim()
+    anim.setAttribute('begin', specs[i][0]);
+    is(getStartTime(anim), specs[i][1],
+       "Got unexpected start time for " + specs[i][0]);
+    removeAnim(anim);
+  }
+}
+
 function testOffsetStartup(anim) {
   anim.setAttribute('begin', '3s');
   checkSample(0,-100);

content/smil/test/test_smilValues.xhtml

@@ -0,0 +1,170 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<head>
+  <title>Test for SMIL values</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<a target="_blank"
+  href="https://bugzilla.mozilla.org/show_bug.cgi?id=557885">Mozilla Bug
+  474742</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+<svg id="svg" xmlns="http://www.w3.org/2000/svg" width="120px" height="120px">
+  <circle cx="-100" cy="20" r="15" fill="blue" id="circle"/>
+</svg>
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+<![CDATA[
+/** Test for SMIL values **/
+
+var gSvg = document.getElementById("svg");
+SimpleTest.waitForExplicitFinish();
+
+function main()
+{
+  gSvg.pauseAnimations();
+
+  var testCases = Array();
+
+  // Single value
+  testCases.push({
+    'attr' : { 'values': 'a' },
+    'times': [ [ 0, 'a' ] ]
+  });
+
+  // The parsing below is based on the following discussion:
+  //
+  //   http://lists.w3.org/Archives/Public/www-svg/2011Nov/0136.html
+  //
+  // In summary:
+  // * Values lists are semi-colon delimited and semi-colon terminated.
+  // * However, if there are extra non-whitespace characters after the final
+  //   semi-colon then there's an implied semi-colon at the end.
+  //
+  // This differs to what is specified in SVG 1.1 but is consistent with the
+  // majority of browsers and with existing content (particularly that generated
+  // by Ikivo Animator).
+
+  // Trailing semi-colon
+  testCases.push({
+    'attr' : { 'values': 'a;' },
+    'times': [ [ 0, 'a' ], [ 10, 'a' ] ]
+  });
+
+  // Trailing semi-colon + whitespace
+  testCases.push({
+    'attr' : { 'values': 'a; ' },
+    'times': [ [ 0, 'a' ], [ 10, 'a' ] ]
+  });
+
+  // Whitespace + trailing semi-colon
+  testCases.push({
+    'attr' : { 'values': 'a ;' },
+    'times': [ [ 0, 'a' ], [ 10, 'a' ] ]
+  });
+
+  // Empty at end
+  testCases.push({
+    'attr' : { 'values': 'a;;' },
+    'times': [ [ 0, 'a' ], [ 5, '' ], [ 10, '' ] ]
+  });
+
+  // Empty at end + whitespace
+  testCases.push({
+    'attr' : { 'values': 'a;; ' },
+    'times': [ [ 0, 'a' ], [ 4, 'a' ], [ 5, '' ], [ 10, '' ] ]
+  });
+
+  // Empty in middle
+  testCases.push({
+    'attr' : { 'values': 'a;;b' },
+    'times': [ [ 0, 'a' ], [ 5, '' ], [ 10, 'b' ] ]
+  });
+
+  // Empty in middle + trailing semi-colon
+  testCases.push({
+    'attr' : { 'values': 'a;;b;' },
+    'times': [ [ 0, 'a' ], [ 5, '' ], [ 10, 'b' ] ]
+  });
+
+  // Whitespace in middle
+  testCases.push({
+    'attr' : { 'values': 'a; ;b' },
+    'times': [ [ 0, 'a' ], [ 5, '' ], [ 10, 'b' ] ]
+  });
+
+  // Empty at start
+  testCases.push({
+    'attr' : { 'values': ';a' },
+    'times': [ [ 0, '' ], [ 5, 'a' ], [ 10, 'a' ] ]
+  });
+
+  // Whitespace at start
+  testCases.push({
+    'attr' : { 'values': ' ;a' },
+    'times': [ [ 0, '' ], [ 5, 'a' ], [ 10, 'a' ] ]
+  });
+
+  // Embedded whitespace
+  testCases.push({
+    'attr' : { 'values': ' a b ; c d ' },
+    'times': [ [ 0, 'a b' ], [ 5, 'c d' ], [ 10, 'c d' ] ]
+  });
+
+  // Whitespace only
+  testCases.push({
+    'attr' : { 'values': '  ' },
+    'times': [ [ 0, '' ], [ 10, '' ] ]
+  });
+
+  for (var i = 0; i < testCases.length; i++) {
+    gSvg.setCurrentTime(0);
+    var test = testCases[i];
+
+    // Create animation elements
+    var anim = createAnim(test.attr);
+
+    // Run samples
+    for (var j = 0; j < test.times.length; j++) {
+      var curSample = test.times[j];
+      gSvg.setCurrentTime(curSample[0]);
+      checkSample(anim, curSample[1], curSample[0], i);
+    }
+
+    anim.parentNode.removeChild(anim);
+  }
+
+  SimpleTest.finish();
+}
+
+function createAnim(attr)
+{
+  const svgns = "http://www.w3.org/2000/svg";
+  var anim = document.createElementNS(svgns, 'animate');
+  anim.setAttribute('attributeName','class');
+  anim.setAttribute('dur','10s');
+  anim.setAttribute('begin','0s');
+  anim.setAttribute('fill','freeze');
+  for (name in attr) {
+    anim.setAttribute(name, attr[name]);
+  }
+  return document.getElementById('circle').appendChild(anim);
+}
+
+function checkSample(anim, expectedValue, sampleTime, caseNum)
+{
+  var msg = "Test case " + caseNum +
+    " (values: '" + anim.getAttribute('values') + "')," +
+    "t=" + sampleTime +
+    ": Unexpected sample value:";
+  is(anim.targetElement.className.animVal, expectedValue, msg);
+}
+
+window.addEventListener("load", main, false);
+]]>
+</script>
+</pre>
+</body>
+</html>

content/svg/content/src/SVGMotionSMILAnimationFunction.cpp

@@ -447,7 +447,7 @@ SVGMotionSMILAnimationFunction::SetKeyPoints(const nsAString& aKeyPoints,
 void
 SVGMotionSMILAnimationFunction::UnsetKeyPoints()
 {
-  mKeyTimes.Clear();
+  mKeyPoints.Clear();
   SetKeyPointsErrorFlag(false);
   mHasChanged = true;
 }