SMIL: Discrete "to-animation" should only visit the 'to' value

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dholbert, Assigned: birtles)

Tracking

Trunk
x86
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
Created attachment 425782 [details]
testcase

As pointed out in
  http://lists.w3.org/Archives/Public/www-svg/2010Feb/0015.html
a discrete "to-animation" should actually never visit the underlying value.

Quoting the smil-animation spec:
> For the shorthand form to animation, there is only 1 value; a
> discrete to animation will simply set the "to" value for the
> simple duration.
http://www.w3.org/TR/smil-animation/#AnimFuncCalcMode

(The www-svg post above references similar quotes in other SMIL specs).

Currently, we (and Opera 10.10) both treat discrete "to" animation the same as discrete "from-to" animation. (i.e. we visit the underlying value for the first 50% of the simple duration)  Apparently this is incorrect.

See attached testcase. Currently, it oscillates back and forth between x=0 and x=100. But according to the spec, it should start at at x=100 and stay fixed there forever.

Put another way, <animate calcMode="discrete" to="..." > should behave the same as <set to="..." >.
Do you think we should fix this, or should we push to get the specs changed?
(Reporter)

Comment 2

7 years ago
The post to www-svg (from comment 0) indicates that the SVG testsuite contains a test[1] that mandates the "wrong" behavior (our current behavior). So if we changed to match the letter of the spec, it'd make us fail that test, which is bad. (Though, note that we actually already fail that test, for a different reason -- lack of support for <animateColor>, bug 436296.)

Either the spec should be changed, or the test should be changed to match the spec. And and as long as the test remains unchanged, I think we should keep our current behavior.

[1] http://dev.w3.org/SVG/profiles/1.2T/test/svg/animate-elem-227-t.svg

Comment 3

7 years ago
Haven't blindly followed the w3c testcases in the pase. There are at least 2 others that are wrong that we fail, so I don't think that's a reason not to do the right thing.
I think we should fix our behaviour to match the spec and push to get the testcase updated. Also, I think we should do this before we ship so we don't end up breaking content that is tested against our implementation. Anyone agree?
(Reporter)

Comment 5

7 years ago
Sounds reasonable, yeah.
Ok, nominating as a blocker then. I guess this is about 0.5~1 days. Should be only a couple of lines of code plus a few tests.

Jonathan, any chance this will get on the WG agenda since it's already come up a couple of times on www-svg in the past?
blocking2.0: --- → ?
Assignee: nobody → birtles
Status: NEW → ASSIGNED
blocking2.0: ? → betaN+
Created attachment 465531 [details] [diff] [review]
Patch v1a

Proposed fix. It's a one liner so I don't suppose it needs sr as well.
Attachment #465531 - Flags: review?(dholbert)
(Reporter)

Comment 8

7 years ago
Comment on attachment 465531 [details] [diff] [review]
Patch v1a

Thanks for taking care of this!

>diff --git a/content/smil/nsSMILAnimationFunction.cpp b/content/smil/nsSMILAnimationFunction.cpp
>-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Looks like an unintentional removal of this header-line. :)

>     if (IsToAnimation()) {
>-      // Two discrete values: our base value, and the val in our array
>-      aResult = (simpleProgress < 0.5f) ? aBaseValue : aValues[0];
>+      aResult = aValues[0];

Can you add a simple comment explaining the new behavior, like the comment that was there before?  e.g.:
  // Pure to-animation only ever visits one value -- the 'to' value.

r=dholbert with the above.
Attachment #465531 - Flags: review?(dholbert) → review+
Created attachment 466208 [details] [diff] [review]
Patch v1b, r=dholbert

(In reply to comment #8)
> Looks like an unintentional removal of this header-line. :)
Oops--thanks for catching that!

> >     if (IsToAnimation()) {
> >-      // Two discrete values: our base value, and the val in our array
> >-      aResult = (simpleProgress < 0.5f) ? aBaseValue : aValues[0];
> >+      aResult = aValues[0];
> 
> Can you add a simple comment explaining the new behavior, like the comment that
> was there before?  e.g.:
>   // Pure to-animation only ever visits one value -- the 'to' value.
Good idea. Fixed.
Attachment #465531 - Attachment is obsolete: true
Attachment #466208 - Flags: review+
Pushed: http://hg.mozilla.org/mozilla-central/rev/ca457b5758e0
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Created attachment 493392 [details]
testcase

So according to this bug, the two rects in this testcase should animate differently? That seems totally user hostile to me.
In other words I'd really, really expect both to behave the same, and I think the vast, vast majority of authors who aren't SMIL animation model experts would expect the same.
You need to log in before you can comment on or make changes to this bug.