Closed Bug 544855 Opened 10 years ago Closed 9 years ago

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

Categories

(Core :: SVG, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: dholbert, Assigned: birtles)

Details

Attachments

(3 files, 1 obsolete file)

Attached image 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?
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
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?
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
Attached patch Patch v1a (obsolete) — Splinter Review
Proposed fix. It's a one liner so I don't suppose it needs sr as well.
Attachment #465531 - Flags: review?(dholbert)
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+
(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+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Attached image 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.