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

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
Created attachment 414446 [details] [diff] [review]
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.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Comment 2

8 years ago
Created attachment 414447 [details] [diff] [review]
patch 1, diff -w version
Attachment #414447 - Flags: review?(dbaron)
(Assignee)

Comment 3

8 years ago
Created attachment 414448 [details] [diff] [review]
patch 2: enable 'clip' in SMIL code. (plus tests)
Attachment #414448 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #414448 - Flags: review?(roc)
(Assignee)

Comment 4

8 years ago
Created attachment 414450 [details] [diff] [review]
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.
Attachment #414448 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #414450 - Flags: review?(roc)
Attachment #414450 - Flags: review?(roc) → review+
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).
(Assignee)

Comment 6

8 years ago
(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+
(Assignee)

Comment 8

8 years ago
Thanks! Pushed patch 1 & patch 2:
http://hg.mozilla.org/mozilla-central/rev/6e0620377c26
http://hg.mozilla.org/mozilla-central/rev/a59a5f030021
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.