594198 - SVG SMIL animation on stroke-dashoffset should not require units

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

Attachment: Test case showing non-interpolated dashoffset

In bug 570555 we removed the requirement for SMIL animation values to have units for font-size. We should do this for other length types too.

In particular, this doesn't appear to work for stroke-dashoffset. In the attached test case the animation jumps between the from and to values without interpolating.

Looking into this the problem appears to be that when we parse the from and to values we get
  from: {0,  unit:coord},
  to:   {45, unit:float}.

Then when we go to do the interpolation in nsStyleAnimation, we call GetCommonUnit which returns eUnit_Null since the units don't match and then the interpolation fails and we fall back to discrete animation.

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleAnimation.cpp#76
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleAnimation.cpp#1163

So I guess either we fix the parsing or, better I think, upgrade GetCommonUnits to ConvertToCommonUnit as per the comment here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleAnimation.cpp#1169

Comment 1

[Brian Birtles (:birtles)]

Nominating as blocking 2.0.

The current behaviour is inconsistent (if the from value is '0px' or '1' rather than '0' it works as expected) and incompatible with Opera and WebKit. I think this could be a real headache for web developers (as it was for me).

From a preliminary investigation it seems like it should be straightforward to fix. 1~2 days at most?

The risk is that the code in question is shared with CSS transitions so we need to be sure not to cause any regressions there.

Comment 2

[Daniel Holbert [:dholbert]]

Putting this on my list, after chatting with birtles in IRC. (I might not get to it until next-week-ish, though, since this can be fixed post-feature-freeze.  If anyone else wants to take this before I get to it, feel free to steal it from me. :))

Comment 3

[Brian Birtles (:birtles)]

(In reply to comment #1)
> The current behaviour is inconsistent (if the from value is '0px' or '1' rather
> than '0' it works as expected)

Just to clarify this.
Setting from="1" to="45" works as expected.
Setting from="0px" to="45px" also works as expected.
Setting from="0px" to="45" does NOT work (i.e. no interpolation)

The problem is in matching units. I believe from="0" to="45" should be treated as matching units and therefore interpolate-able.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v1

I'm a bit hesitant to make *all* eUnit_Coord values interoperate with *all* eUnit_Float values in nsStyleAnimation, because that would have wider-reaching implications, and it could potentially have negative side-effects. (It'd also mean we'd need to do int --> float conversions in the common-unit-finding code, which would lose precision earlier than we might like.)

However, we do clearly need to make "0" eUnit_Coord values interoperate with all eUnit_Float values, so that e.g. <animate from="0" to="20" ...> will work.

So this patch takes a conservative approach, tweaking nsSMILCSSValueType to check for cases where we mix a eUnit_Coord "0" value with a eUnit_Float value.  In those cases, this patch just switches out the "0" value for one with eUnit_Float, so that nsStyleAnimation won't fail.

This also makes some cleanup fixes in contextual code:
 (a) s/aStyleCoord/aValue/ in "InvertSign" helper-method (that's cruft from back before nsStyleAnimation::Value was created)
 (b) removes unnecessary checks for failure in "new"

I've confirmed that this fixes this bug's testcase & passes existing SMIL reftests & mochitests.  Still need to write reftests -- I'll attach those as a separate patch here.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 487737 [details] [diff] [review]
fix v1

fixing a bug in from-by animation uncovered by test-writing -- reposting in a few min.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v1a with tests

Here's the fix, with tests included now.

The only code change vs. comment 4 is that I added a clause to nsSMILCSSValueType::Add() to update the outparam (destWrapper) for the situation where our destValue has been redirected (by FinalizeStyleAnimationValues, to point to the zero for eUnit_Float).

For tests, I added some 0-mixed-with-other-unitless-value cases to the from-to/from-by/paced-mode CSS mochitests. (I have different bundles for SVG-specific properties vs. non-SVG-specific properties, since our output is slightly different -- we automatically append units for the non-SVG-specific properties, whereas we don't for the SVG-specific properties.)

I also added a few reftests with unitless values (using "hg cp" which makes for minimal but confusing-to-bugzilla diffs), but those aren't really specific to this bug here -- those are more for comprehensiveness.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: patch v1b with tests

(Sorry, one last tweak -- this version just removes an incorrect/confused XXX comment added to a mochitest in the previous patch-version.)

Comment 8

[Brian Birtles (:birtles)]

Comment on attachment 487776 [details] [diff] [review]
patch v1b with tests

Looks really good. Thanks Daniel!

One question, I don't think we test anywhere for, e.g. from="0" to="50px". Is that right? And is that something we ought to support and that might possibly regress?

Comment 9

[Daniel Holbert [:dholbert]]

Thanks!

Good call on testing from="0" to="50px" -- I don't expect this would regress that (since 0 effectively gets parsed as if it were "0px", and this bug only shifts it back to being unitless-0 if we're combining it with another unitless value).

I'll add a test for that & make sure it passes before landing.

Comment 10

[Daniel Holbert [:dholbert]]

er s/this bug only/this patch only/

Comment 11

[Daniel Holbert [:dholbert]]

Landed with additional mochitests:
 http://hg.mozilla.org/mozilla-central/rev/17b72e252d83