Closed Bug 530983 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
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.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
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.
Attachment #414448 - Attachment is obsolete: true
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).
(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 on attachment 414447 [details] [diff] [review]
patch 1, diff -w version

ok, r=dbaron.  Sorry for the delay.
Attachment #414447 - Flags: review?(dbaron) → review+
You need to log in before you can comment on or make changes to this bug.