520486 - Extend nsStyleAnimation & SVG/SMIL to support enumerated values

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

nsStyleAnimation needs to be extended to support enumerated values (e.g. to support animating properties like "pointer-events" and "display")

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: patch 1: support animating enumerated values that are stored as PRUint8

This patch makes adds a new animated type "eStyleAnimType_Enum8" for enumerated values that are stored as PRUint8's in their style-struct.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch 2: enable SMIL animation of enum-valued properties (with tests)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: attached wrong file

Here's "patch 1" again, rebased to apply on top of bug 522852 and bug 523193.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch 1 (rebased)

(sorry, that last one was the wrong file -- reposting)

Comment 5

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

Comment on attachment 407194 [details] [diff] [review]
patch 1 (rebased)

Could you call it eStyleAnimType_EnumU8 since it's PRUint8?

Also, in the comment for eStyleAnimType_EnumU8, you should comment that
it requires that *all* of the enumerated values be accepted as an 
eCSSUnit_Enumerated value, which requires that they be in the kwtable_ 
entry in nsCSSPropList.h.  (This also means that this patch depends on
bug 522320 for some properties: display:none, font-style:normal,
font-variant:normal, pointer-events:none, text-decoration:none, 
color-interpolation:auto, color-interpolation-filters:auto, 
dominant-baseline:auto, image-rendering:auto, shape-rendering:auto, 
text-rendering:auto.  I'll need to check that all of those are fixed 
there.)

Is it an issue that mTextDecoration is a bitfield rather than a set of 
values?  Probably not.

r=dbaron with that.

Sorry for the delay:  shouldn't have made you rebase it.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> (From update of attachment 407194 [details] [diff] [review])
> Could you call it eStyleAnimType_EnumU8 since it's PRUint8?

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/06010a0b17ca

> Also, in the comment for eStyleAnimType_EnumU8, you should comment that
> it requires that *all* of the enumerated values be accepted as an 
> eCSSUnit_Enumerated value, which requires that they be in the kwtable_ 
> entry in nsCSSPropList.h.

Comment added:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/37279e2814af

> (This also means that this patch depends on bug 522320

Correct -- it depends on the first patch on that bug.  (I'd thought I added a comment here to that effect, but I guess I just set the dependency field -- sorry if that wasn't clear.)

> Is it an issue that mTextDecoration is a bitfield rather than a set of 
> values?  Probably not.

Nope, that's not an issue.  We're just serializing the bitfield that we get from the style system (in ComputeValue/ExtractComputedValue), and then later deserializing it as a string (in UncomputeValue).

The serialization is trivial ("copy the bitfield"). The deserialization is less trivial, but it's handled correctly by nsCSSDeclaration::AppendCSSValueToString (with a minor tweak, in bug 522320).

> r=dbaron with that.

Thanks for the review!  I'll land this once the first patch on Bug 522320 is clear for landing.

> Sorry for the delay:  shouldn't have made you rebase it.

No problem -- as rebases go, this one was easy. :)  (mostly taken care of by a simple s/eStyleUnit_/eUnit_/ in the patch file.)

Comment 7

[Daniel Holbert [:dholbert]]

Landed:
http://hg.mozilla.org/mozilla-central/rev/71af388ef07d
http://hg.mozilla.org/mozilla-central/rev/3745dd4c718c