Closed
Bug 974710
Opened 10 years ago
Closed 10 years ago
Firefox incorrectly reports errors with "values" attribute on <animateMotion> element
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: dholbert, Assigned: longsonr)
References
Details
Attachments
(2 files)
141 bytes,
image/svg+xml
|
Details | |
916 bytes,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
STR: 1. Open Browser console (Ctrl+Shift+J) 2. Load attached testcase ACTUAL RESULTS: 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.
Reporter | ||
Comment 1•10 years ago
|
||
[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
Assignee | ||
Comment 2•10 years ago
|
||
It's always done this since the initial code landed in bug 436418 patch E. this was the original code... PRBool 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) { MarkStaleIfAttributeAffectsPath(aAttribute); } 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.
Assignee | ||
Comment 3•10 years ago
|
||
Assignee: nobody → longsonr
Attachment #8406910 -
Flags: review?(dholbert)
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8406910 [details] [diff] [review] console.txt 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+
Assignee | ||
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb0b8d757f4
Flags: in-testsuite-
Comment 6•10 years ago
|
||
sorry had to backout for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=37916126&tree=Mozilla-Inbound
Assignee | ||
Comment 7•10 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/32df543ab3c6
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/32df543ab3c6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Updated•10 years ago
|
QA Whiteboard: [good first verify]
You need to log in
before you can comment on or make changes to this bug.
Description
•