animation should be disabled for failing conditional-processing conditions

RESOLVED FIXED in mozilla13

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Robert Longson, Assigned: Robert Longson)

Tracking

Trunk
mozilla13
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

6.05 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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

6 years ago
(Assignee)

Updated

6 years ago
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?
(Assignee)

Comment 2

6 years ago
Created attachment 502752 [details]
testcase
(Assignee)

Comment 3

5 years ago
Created attachment 594564 [details] [diff] [review]
patch
Assignee: nobody → longsonr
Attachment #502752 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 5

5 years ago
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?)
(Assignee)

Comment 7

5 years ago
Couldn't make it compile without it.
https://hg.mozilla.org/mozilla-central/rev/a963e2178e05
Status: NEW → RESOLVED
Last Resolved: 5 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.