Last Comment Bug 544855 - SMIL: Discrete "to-animation" should only visit the 'to' value
: SMIL: Discrete "to-animation" should only visit the 'to' value
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Brian Birtles (:birtles)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-08 04:03 PST by Daniel Holbert [:dholbert]
Modified: 2010-11-26 06:15 PST (History)
3 users (show)
bbirtles: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+


Attachments
testcase (278 bytes, image/svg+xml)
2010-02-08 04:03 PST, Daniel Holbert [:dholbert]
no flags Details
Patch v1a (6.73 KB, patch)
2010-08-12 19:56 PDT, Brian Birtles (:birtles)
dholbert: review+
Details | Diff | Splinter Review
Patch v1b, r=dholbert (6.49 KB, patch)
2010-08-15 19:55 PDT, Brian Birtles (:birtles)
bbirtles: review+
Details | Diff | Splinter Review
testcase (558 bytes, image/svg+xml)
2010-11-26 06:13 PST, Jonathan Watt [:jwatt]
no flags Details

Description Daniel Holbert [:dholbert] 2010-02-08 04:03:58 PST
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="..." >.
Comment 1 Jonathan Watt [:jwatt] 2010-02-15 10:12:02 PST
Do you think we should fix this, or should we push to get the specs changed?
Comment 2 Daniel Holbert [:dholbert] 2010-02-15 20:35:19 PST
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 Robert Longson 2010-02-16 01:37:18 PST
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.
Comment 4 Brian Birtles (:birtles) 2010-07-18 19:22:22 PDT
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?
Comment 5 Daniel Holbert [:dholbert] 2010-07-18 19:27:58 PDT
Sounds reasonable, yeah.
Comment 6 Brian Birtles (:birtles) 2010-07-18 19:36:04 PDT
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?
Comment 7 Brian Birtles (:birtles) 2010-08-12 19:56:40 PDT
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.
Comment 8 Daniel Holbert [:dholbert] 2010-08-12 20:15:07 PDT
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.
Comment 9 Brian Birtles (:birtles) 2010-08-15 19:55:14 PDT
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.
Comment 10 Brian Birtles (:birtles) 2010-08-18 03:28:21 PDT
Pushed: http://hg.mozilla.org/mozilla-central/rev/ca457b5758e0
Comment 11 Jonathan Watt [:jwatt] 2010-11-26 06:13:12 PST
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.
Comment 12 Jonathan Watt [:jwatt] 2010-11-26 06:15:03 PST
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.

Note You need to log in before you can comment on or make changes to this bug.