Closed Bug 974710 Opened 6 years ago Closed 6 years ago

Firefox incorrectly reports errors with "values" attribute on <animateMotion> element


(Core :: SVG, defect)

Not set





(Reporter: dholbert, Assigned: longsonr)




(2 files)

Attached image testcase 1
 1. Open Browser console (Ctrl+Shift+J)
 2. Load attached testcase

This error appears in Browser Console:
 "Unexpected value 0,0; 200,0 parsing values attribute."
(though in actuality, the attribute seems to be parsed/handled correctly.)

EXPECTED RESULTS: No error reported in console.
[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
It's always done this since the initial code landed in bug 436418 patch E.

this was the original code...

SVGMotionSMILAnimationFunction::SetAttr(nsIAtom* aAttribute,
                                        const nsAString& aValue,
                                        nsAttrValue& aResult,
                                        nsresult* aParseResult)
  if (aAttribute == nsGkAtoms::rotate) {
     nsresult rv = SetRotate(aValue, aResult);
     if (aParseResult) {
       *aParseResult = rv;
   } else if (aAttribute == nsGkAtoms::by ||
              aAttribute == nsGkAtoms::from ||
              aAttribute == nsGkAtoms::to ||
              aAttribute == nsGkAtoms::values) {
   } else {
     // Defer to superclass method
     return nsSMILAnimationFunction::SetAttr(aAttribute, aValue,
                                             aResult, aParseResult);
  return true;

In the central case it calls MarkStaleIfAttributeAffectsPath but does not set *aParseResult to anything and then drops through to return true. Since *aParseResult is initialised to NS_ERROR_FAILURE the caller thinks that this function has found something to parse but failed to parse it.

We either need to add

     if (aParseResult) {
       *aParseResult = NS_OK;

after MarkStaleIfAttributeAffectsPath(aAttribute);

or add
     return false;

depending on whether MarkStaleIfAttributeAffectsPath completes the parsing of these values.
Attached patch console.txtSplinter Review
Assignee: nobody → longsonr
Attachment #8406910 - Flags: review?(dholbert)
Comment on attachment 8406910 [details] [diff] [review]

So, this means we'll *never* warn for these attributes (even on invalid input), right?  Ideally it'd be nice to still warn if the value is invalid -- but since we're not even parsing the value yet at this point (not until sample-time), I suppose we don't know whether it's invalid. So, I suppose not-warning-ever seems reasonable. (more reasonable than the current warning-on-everything behavior, at least)

One nit:

>diff --git a/content/svg/content/src/SVGMotionSMILAnimationFunction.cpp b/content/svg/content/src/SVGMotionSMILAnimationFunction.cpp
>@@ -76,16 +76,19 @@ SVGMotionSMILAnimationFunction::SetAttr(
>       *aParseResult = NS_OK;
>     }
>     MarkStaleIfAttributeAffectsPath(aAttribute);
>   } else if (aAttribute == nsGkAtoms::by ||
>              aAttribute == nsGkAtoms::from ||
>              aAttribute == nsGkAtoms::to ||
>              aAttribute == nsGkAtoms::values) {
>     MarkStaleIfAttributeAffectsPath(aAttribute);
>+    if (aParseResult) {
>+      *aParseResult = NS_OK;
>+    }

You're setting aParseResult after calling MarkStaleIfAttributeAffectsPath, but the clause above (shown in patch-context) does these steps in the opposite order.  Could you tweak one clause or the other so that we're consistent?

Thanks for taking this!
Attachment #8406910 - Flags: review?(dholbert) → review+
The ReportFailure code path also calls aResult.SetTo(aValue); and this side effect is necessary. We can just combine the values (and other attributes) into the existing nsGkAtoms::path code path to do this though.
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.