Bug 474257. Implement SVG 1.1 erratum to make beginElementAt/endElementAt return void. r+sr=roc

content/smil/nsSMILTimedElement.cpp

@@ -93,63 +93,39 @@ nsSMILTimedElement::nsSMILTimedElement()
 // Animation and SVG 1.1. In SMIL Animation all methods have a void return
 // type and the new instance time is simply added to the list and restart
 // semantics are applied as with any other instance time. In the SVG definition
-// the methods return a bool depending on the restart mode. There are some
-// cases where this is problematic.
+// the methods return a bool depending on the restart mode.
 //
-// For example, if a call is made to beginElementAt and the resolved time
-// after including the offset falls outside the current interval then using
-// the SMIL Animation definition an element with restart == whenNotActive
-// would restart with this new instance time. The SVG definition however seems
-// to imply that in this case the implementation should ignore the new
-// instance time if the restart mode == whenNotActive and the element is
-// currently active and return false.
+// This inconsistency has now been addressed by an erratum in SVG 1.1:
 //
-// It is tempting to try and determine when a new instance time will actually
-// cause a restart but this is not possible as in the meantime a new event may
-// trump the new instance time. We take a compromise of returning true and
-// false according to the SVG definition but adding the instance time to the
-// list regardless. This may produce different results to an implementation that
-// follows strictly the behaviour implied by the SVG spec.
+// http://www.w3.org/2003/01/REC-SVG11-20030114-errata#elementtimecontrol-interface
 //
+// which favours the definition in SMIL, i.e. instance times are just added
+// without first checking the restart mode.
 
-/* boolean beginElementAt (in float offset); */
-PRBool
+nsresult
 nsSMILTimedElement::BeginElementAt(double aOffsetSeconds,
                                    const nsSMILTimeContainer* aContainer)
 {
-  // If restart == never or restart == whenNotActive, check whether we're
-  // in a state that allows us to restart.
-  if ((mRestartMode == RESTART_NEVER &&
-       (mElementState == STATE_ACTIVE || mElementState == STATE_POSTACTIVE)) ||
-      (mRestartMode == RESTART_WHENNOTACTIVE &&
-       mElementState == STATE_ACTIVE)) {
-    return PR_FALSE;
-  }
-
   if (!AddInstanceTimeFromCurrentTime(aOffsetSeconds, PR_TRUE, aContainer)) {
     // Probably we don't have a time container
     NS_ERROR("Failed to begin element");
-    return PR_FALSE;
+    return NS_ERROR_FAILURE;
   }
 
-  return PR_TRUE;
+  return NS_OK;
 }
 
-/* boolean endElementAt (in float offset); */
-PRBool
+nsresult
 nsSMILTimedElement::EndElementAt(double aOffsetSeconds,
                                  const nsSMILTimeContainer* aContainer)
 {
-  if (mElementState != STATE_ACTIVE)
-    return PR_FALSE;
-
   if (!AddInstanceTimeFromCurrentTime(aOffsetSeconds, PR_FALSE, aContainer)) {
     // Probably we don't have a time container
     NS_ERROR("Failed to end element");
-    return PR_FALSE;
+    return NS_ERROR_FAILURE;
   }
 
-  return PR_TRUE;
+  return NS_OK;
 }
 
 //----------------------------------------------------------------------
@@ -190,10 +166,11 @@ void
 nsSMILTimedElement::AddInstanceTime(const nsSMILInstanceTime& aInstanceTime,
                                     PRBool aIsBegin)
 {
-  if (aIsBegin)
+  if (aIsBegin) {
     mBeginInstances.AppendElement(aInstanceTime);
-  else
+  } else {
     mEndInstances.AppendElement(aInstanceTime);
+  }
 
   UpdateCurrentInterval();
 }

content/smil/nsSMILTimedElement.h

@@ -75,7 +75,7 @@ public:
    *                   current time.
    * @return NS_OK if the operation succeeeded, or an error code otherwise.
    */
-  PRBool BeginElementAt(double aOffsetSeconds,
+  nsresult BeginElementAt(double aOffsetSeconds,
                           const nsSMILTimeContainer* aContainer);
 
   /*
@@ -89,7 +89,7 @@ public:
    *                   current time.
    * @return NS_OK if the operation succeeeded, or an error code otherwise.
    */
-  PRBool EndElementAt(double aOffsetSeconds,
+  nsresult EndElementAt(double aOffsetSeconds,
                         const nsSMILTimeContainer* aContainer);
 
   /** 

content/smil/test/test_smilRestart.xhtml

@@ -11,15 +11,15 @@
 <svg id="svg" xmlns="http://www.w3.org/2000/svg" width="120px" height="120px">
   <!-- These 3 circles only differ in their animation's "restart" value -->
   <circle cx="20" cy="20" r="15" fill="blue">
-    <animate attributeName="cx" from="20" to="100" begin="0s" dur="4s"
+    <animate attributeName="cx" from="20" to="100" begin="1s" dur="4s"
              restart="always" id="always" attributeType="XML"/>
   </circle>
   <circle cx="20" cy="60" r="15" fill="blue">
-    <animate attributeName="cx" from="20" to="100" begin="0s" dur="4s"
+    <animate attributeName="cx" from="20" to="100" begin="1s" dur="4s"
              restart="whenNotActive" id="whenNotActive" attributeType="XML"/>
   </circle>
   <circle cx="20" cy="100" r="15" fill="blue">
-    <animate attributeName="cx" from="20" to="100" begin="0s" dur="4s"
+    <animate attributeName="cx" from="20" to="100" begin="1s" dur="4s"
              restart="never" id="never" attributeType="XML"/>
   </circle>
 </svg>
@@ -30,8 +30,10 @@
 /** Test for SMIL Restart Behavior  **/
 
 /* Global Variables */
-var dur = 4.0; // this must match the "dur" attribute on animations above.
 var svg = document.getElementById("svg");
+var always = document.getElementById("always");
+var whenNotActive = document.getElementById("whenNotActive");
+var never = document.getElementById("never");
 
 SimpleTest.waitForExplicitFinish();
 
@@ -40,60 +42,62 @@ function main() {
   checkInitialState();
 }
 
-// Attempt a "beginElement" call on the given element, and
-// complain if we don't get the expected return value.
-// 'time' is only provided for better diagnostic output.
-function tryRestartElem(id, time, expectedRetVal) {
-  var elem = document.getElementById(id); 
-  var retVal = elem.beginElement();
-  is(retVal, expectedRetVal,
-     "Error restarting animation '" + id + 
-     "' at time = " + time + ": " + 
-     "expected return value of " + expectedRetVal + 
-     ", but got " + retVal + ".");
+function restartAll() {
+  always.beginElement();
+  whenNotActive.beginElement();
+  never.beginElement();
 }
 
 function checkInitialState() {
-  svg.setCurrentTime(0.0);
-  setTimeout('doCheckInitialState(0.0)', 0);
+  // At first everything should be starting at 1s
+  is(always.getStartTime(), 1);
+  is(whenNotActive.getStartTime(), 1);
+  is(never.getStartTime(), 1);
+
+  // Now try to restart everything early, should be allowed by all
+  var restartTime = svg.getCurrentTime();
+  restartAll();
+  setTimeout('doCheckInitialState(' + restartTime + ')',0);
 }
-function doCheckInitialState(time) {
-  tryRestartElem('always', time, true);
-  tryRestartElem('whenNotActive', time, false);
-  tryRestartElem('never', time, false);
-  checkHalfwayState();
+function doCheckInitialState(restartTime) {
+  is(always.getStartTime(), restartTime);
+  is(whenNotActive.getStartTime(), restartTime);
+  is(never.getStartTime(), restartTime);
+
+  // Now skip to half-way
+  var newTime = restartTime + 0.5 * always.getSimpleDuration();
+  svg.setCurrentTime(newTime);
+  setTimeout('checkHalfwayState()', 0);
 }
 
 function checkHalfwayState() {
-  svg.setCurrentTime(0.5 * dur);
-  setTimeout('doCheckHalfwayState(0.5 * dur)', 0);
+  // Try to restart half-way through--only 'always' should succeed
+  var restartTime = svg.getCurrentTime();
+  restartAll();
+  setTimeout('doCheckHalfwayState(' + restartTime + ')',0);
 }
-function doCheckHalfwayState(time) {
-  tryRestartElem('always', time, true);
-  tryRestartElem('whenNotActive', time, false);
-  tryRestartElem('never', time, false);
-  checkEndingState();
+function doCheckHalfwayState(restartTime) {
+  is(always.getStartTime(), restartTime);
+  isnot(whenNotActive.getStartTime(), restartTime);
+  isnot(never.getStartTime(), restartTime);
+
+  // Now skip to the end 
+  var newTime = always.getStartTime() + always.getSimpleDuration() + 1;
+  svg.setCurrentTime(newTime);
+  setTimeout('checkEndingState()', 0);
 }
 
 function checkEndingState() {
-  svg.setCurrentTime(dur);
-  setTimeout('doCheckEndingState(dur)', 0);
+  // Try to restart after all animations have finished--'always' and
+  // 'whenNotActive' should succeed
+  var restartTime = svg.getCurrentTime();
+  restartAll();
+  setTimeout('doCheckEndingState(' + restartTime + ')',0);
 }
-function doCheckEndingState(time) {
-  tryRestartElem('always', time, true);
-  tryRestartElem('whenNotActive', time, true);
-  tryRestartElem('never', time, false);
-  checkAfterEndingState();
-}
-
-function checkAfterEndingState() {
-  svg.setCurrentTime(dur * 3);
-  setTimeout('doCheckAfterEndingState(dur * 3)', 0);
-}
-function doCheckAfterEndingState(time) {
-  tryRestartElem('always', time, true);
-  tryRestartElem('whenNotActive', time, true);
-  tryRestartElem('never', time, false);
+function doCheckEndingState(restartTime) {
+  is(always.getStartTime(), restartTime);
+  is(whenNotActive.getStartTime(), restartTime);
+  isnot(never.getStartTime(), restartTime);
   SimpleTest.finish();
 }
 

content/svg/content/src/nsSVGAnimationElement.cpp

@@ -386,12 +386,7 @@ nsSVGAnimationElement::BeginElementAt(float offset)
 
   ownerSVG->RequestSample();
 
-  // We ignore the return code. The SMIL version of this interface has a void
-  // return type and no exception specification so there's no way to indicate
-  // that begin failed (e.g. because the element has restart="none").
-  mTimedElement.BeginElementAt(offset, mTimedDocumentRoot);
-
-  return NS_OK;
+  return mTimedElement.BeginElementAt(offset, mTimedDocumentRoot);
 }
 
 /* void endElement (); */
@@ -411,8 +406,5 @@ nsSVGAnimationElement::EndElementAt(float offset)
 
   ownerSVG->RequestSample();
 
-  // As with BeginElementAt, ignore the return code.
-  mTimedElement.EndElementAt(offset, mTimedDocumentRoot);
-
-  return NS_OK;
+  return mTimedElement.EndElementAt(offset, mTimedDocumentRoot);
 }