619469 - animation should be disabled for failing conditional-processing conditions

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

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.

Comment 1

[Brian Birtles (:birtles)]

I'm getting 403 on the test URL (and in fact all documents from that domain: http://hoffmann.bplaced.net/). Any mirror?

Comment 2

[Robert Longson [:longsonr]]

Attachment: testcase

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

Comment 4

[Daniel Holbert [: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

Comment 5

[Robert Longson [:longsonr]]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/a963e2178e05

Comment 6

[Daniel Holbert [:dholbert]]

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?)

Comment 7

[Robert Longson [:longsonr]]

Couldn't make it compile without it.

Comment 8

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/a963e2178e05