530983 - SVG/SMIL: Enable support for animating 'clip' property

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Filing this bug on enabling support for animating the "clip" property in SVG/SMIL.

This depends on the patch in bug 520488, as well as a follow-up described in bug 520488 comment 2:
> I just realized SMIL will also need Auto support in UncomputeValue, but that
> can come in a later patch...

I've got a patches for both, which I can post here.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: patch 1: represent "clip: auto" the same way CSSParserImpl does.

Here's a patch that makes "clip: auto" work in UncomputeValue.  In the spirit of bug 524539 comment 4, I think the cleanest strategy is to have nsStyleAnimation represent "clip: auto" the same way that the parser does -- with a nsCSSRect whose values all have unit "eCSSUnit_RectIsAuto", instead of a standard nsStyleAnimation::Value(eUnit_Auto).  With this change, we don't actually have to touch UncomputeValue.

NOTE: An alternate solution would be to keep using eUnit_Auto in the nsStyleAnimation::Value struct, and instead tweak UncomputeValue to generate a temporary nsCSSRect whose values all have the unit "eCSSUnit_RectIsAuto". I've got an alternative patch that does this here:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/ba69c4377577/styleanimation_rect_additions_option1

I think I prefer the strategy in the attached patch, though I'm open to using the other one instead, or doing something completely different.
(Note that we don't use the "eUnit_Auto" unit anywhere else yet, and I'd prefer that we don't introduce a single special-case use of that unit just for 'clip', if we don't have to. Hopefully we can just remove that unit altogether, along with eUnit_Normal, since it looks like we won't actually need either of them.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch 1, diff -w version

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: patch 2: enable 'clip' in SMIL code. (plus tests)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch 2: enable 'clip' in SMIL code (plus tests)

Tweaked patch 2 slightly.  Since this bug brings us up to being able to animate all Mozilla-suported properties in the SVG property index, I'm simplifying the switch statement in IsPropertyAnimatable() so that the shorthand and rect-valued properties aren't separated from the rest anymore.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why can't you leave it using _Auto?  It seems like it wouldn't break anything if we made eUnit_Auto and similar types not support interpolation in general (I hope not, anyway).

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> Why can't you leave it using _Auto?

As I mentioned in Comment 1, that's an alternative option, but it means we need to add some special-case code to UncomputeValue, to populate whatever structs that we expect eUnit_Auto to be converted back into.  (I've got a patch linked in that comment which does this).

We need that fix because the lower UncomputeValue -- and AppendStorageToValue, which it calls -- both depend on kTypeTable[aProperty].  For 'clip', they'll use a nsCSSRect.  So the upper UncomputeValue needs to handle converting "eUnit_Auto" into a nsCSSRect.

> It seems like it wouldn't break anything
> if we made eUnit_Auto and similar types not support interpolation in general 

The only thing that would break is UncomputeValue for non-nsCSSValued properties that these units, as described above -- we'd need to add special-case code to UncomputeValue to for the corresponding types.

The attached patch feels a little cleaner/safer to me, but I don't feel particularly strongly about it.  I'm happy to go with the other strategy (in the patch linked in comment 1) if you prefer.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 414447 [details] [diff] [review]
patch 1, diff -w version

ok, r=dbaron.  Sorry for the delay.

Comment 8

[Daniel Holbert [:dholbert]]

Thanks! Pushed patch 1 & patch 2:
http://hg.mozilla.org/mozilla-central/rev/6e0620377c26
http://hg.mozilla.org/mozilla-central/rev/a59a5f030021