Closed Bug 520486 Opened 11 years ago Closed 11 years ago

Extend nsStyleAnimation & SVG/SMIL to support enumerated values

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files, 2 obsolete files)

nsStyleAnimation needs to be extended to support enumerated values (e.g. to support animating properties like "pointer-events" and "display")
OS: Linux → All
Hardware: x86 → All
Depends on: 522320
Summary: Extend nsStyleAnimation to support enumerated values → Extend nsStyleAnimation & SVG/SMIL to support enumerated values
This patch makes adds a new animated type "eStyleAnimType_Enum8" for enumerated values that are stored as PRUint8's in their style-struct.
Attachment #406388 - Flags: review?(dbaron)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attached patch attached wrong file (obsolete) — Splinter Review
Here's "patch 1" again, rebased to apply on top of bug 522852 and bug 523193.
Attachment #407193 - Flags: review?(dbaron)
Attachment #406388 - Attachment is obsolete: true
Attachment #406388 - Flags: review?(dbaron)
(sorry, that last one was the wrong file -- reposting)
Attachment #407193 - Attachment is obsolete: true
Attachment #407194 - Flags: review?(dbaron)
Attachment #407193 - Flags: review?(dbaron)
Attachment #407193 - Attachment description: patch 1 (rebased) → attached wrong file
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.
Attachment #407194 - Flags: review?(dbaron) → review+
(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.)
Landed:
http://hg.mozilla.org/mozilla-central/rev/71af388ef07d
http://hg.mozilla.org/mozilla-central/rev/3745dd4c718c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 525099
You need to log in before you can comment on or make changes to this bug.