Closed
Bug 619469
Opened 13 years ago
Closed 12 years ago
animation should be disabled for failing conditional-processing conditions
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: longsonr, Assigned: longsonr)
References
()
Details
Attachments
(1 file, 1 obsolete file)
6.05 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
http://www.w3.org/TR/SVG11/struct.html#ConditionalProcessingOverview ‘requiredFeatures’, ‘requiredExtensions’ and ‘systemLanguage’ attributes affect ‘animate’, ‘animateColor’, ‘animateMotion’, ‘animateTransform’, and ‘set’ elements. If the conditional statement on these animation elements fails, the animation will never be triggered.
Assignee | ||
Updated•13 years ago
|
Assignee | ||
Updated•13 years ago
|
Summary: animation should be disabled for failing switch conditions → animation should be disabled for failing conditional-processing conditions
Comment 1•13 years ago
|
||
I'm getting 403 on the test URL (and in fact all documents from that domain: http://hoffmann.bplaced.net/). Any mirror?
Assignee | ||
Comment 2•13 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Assignee: nobody → longsonr
Attachment #502752 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Updated•12 years ago
|
Attachment #594564 -
Flags: review?(dholbert)
Comment 4•12 years ago
|
||
Comment on attachment 594564 [details] [diff] [review] patch > /*static*/ void > nsSMILAnimationController::AddAnimationToCompositorTable( > nsISMILAnimationElement* aElement, nsSMILCompositorTable* aCompositorTable) > { >+ if (!aElement->PassesConditionalProcessingTests()) >+ // If the animation fails conditional processing >+ // it is ignored. >+ return; My initial reaction was "we should catch this earlier, before even registering the animation element with the animation controller". But I suppose conditions could change between samples & we could dynamically go from not-passing-conditional to passing-conditional. So, your patch is probably the most straightforward way to solve this. However, I think we do want to do this up one stack-level -- in nsSMILAnimationController::SampleAnimation. I think we want to wrap these three lines in a PassesConditionalProcessingTests() check: > 690 SampleAnimationParams* params = static_cast<SampleAnimationParams*>(aData); > 691 > 692 SampleTimedElement(animElem, params->mActiveContainers); > 693 AddAnimationToCompositorTable(animElem, params->mCompositorTable); Two notes on the test: >+++ b/layout/reftests/svg/smil/anim-conditions-01.svg >@@ -0,0 +1,11 @@ >+ <rect width="100%" height="100%" fill="lime"> >+ <set systemLanguage="foo" attributeName="width" to="0%" dur="5s" begin="0s"/> >+ </rect> Nit: jst-review says that <set> line is more than 80 characters -- perhaps add a newline in the middle somewhere? Also, could you add an additional <set> element (or an additional test) to be sure we actually render animations that have *succeeding* conditionals? e.g. you could apply s/lime/orange/ above, and then insert a <set [somePassingConditional] attributeName="fill" to="lime" ... > r=dholbert with those changes
Attachment #594564 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 5•12 years ago
|
||
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/a963e2178e05
Flags: in-testsuite+
Target Milestone: --- → mozilla13
Comment 6•12 years ago
|
||
Comment on attachment 594564 [details] [diff] [review] patch Thanks for the fix! Sorry, one other thing I just noticed & was wondering about: >+bool >+nsSVGAnimationElement::PassesConditionalProcessingTests() >+{ >+ nsCOMPtr<DOMSVGTests> tests(do_QueryInterface( >+ static_cast<nsSVGElement*>(this))); Do we really need the static_cast there? (& if we don't, could you push a followup to remove it?)
Assignee | ||
Comment 7•12 years ago
|
||
Couldn't make it compile without it.
Comment 8•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a963e2178e05
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
OS: Windows Vista → All
Hardware: x86 → All
You need to log in
before you can comment on or make changes to this bug.
Description
•