Closed Bug 619469 Opened 13 years ago Closed 12 years ago

animation should be disabled for failing conditional-processing conditions

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: longsonr, Assigned: longsonr)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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.
Summary: animation should be disabled for failing switch conditions → animation should be disabled for failing conditional-processing conditions
I'm getting 403 on the test URL (and in fact all documents from that domain: http://hoffmann.bplaced.net/). Any mirror?
Attached image testcase (obsolete) —
Attached patch patchSplinter Review
Assignee: nobody → longsonr
Attachment #502752 - Attachment is obsolete: true
Attachment #594564 - Flags: review?(dholbert)
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+
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/a963e2178e05
Flags: in-testsuite+
Target Milestone: --- → mozilla13
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?)
Couldn't make it compile without it.
https://hg.mozilla.org/mozilla-central/rev/a963e2178e05
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
OS: Windows Vista → All
Hardware: x86 → All
Depends on: 1010681
You need to log in before you can comment on or make changes to this bug.