Closed Bug 520485 Opened 10 years ago Closed 10 years ago

Extend nsStyleAnimation to support float values for SVG/SMIL-Animatable properties

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

8.29 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
14.71 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.59 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
4.76 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.52 KB, patch
roc
: review+
Details | Diff | Splinter Review
nsStyleAnimation needs to be extended to support float values (e.g. "opacity", "font-size-adjust")
This patch adds a pretty straightforward implementation for floats in nsStyleAnimation.  This covers all float-valued properties recognized in the SVG Property Index, except for "font-size-adjust".  (That is -- this patch adds support for the various opacity properties, plus the "stroke-miterlimit" property.)

"font-size-adjust" requires some special cases, so I'm giving it its own separate patch for simplicity.
Attachment #405233 - Flags: review?(dbaron)
Attachment #405233 - Attachment description: style-animation-float patch v1 → patch 1: style-animation-float
This patch updates the SMIL code to enable animation of float-valued properties, once nsStyleAnimation supports floats.

This patch just...
 - enables mochitests & reftests for animation of float-valued properties
 - uncomments the properties in sSMILCSSProperty::IsPropertyAnimatable()
 - adds boilerplate code to nsSMILCSSValueType

As above, I'll be handling "font-size-adjust" in a separate patch.
Attachment #405233 - Attachment description: patch 1: style-animation-float → patch 1: support floats in nsStyleAnimation
Attachment #405276 - Attachment is patch: true
Attachment #405276 - Attachment mime type: application/octet-stream → text/plain
This patch adds support for "font-size-adjust", which is a float-valued property that can also take on the value "none".

This patch just:
 - enables 'font-size-adjust' in nsCSSPropList.h
 - adds a special-case to recognize the "none" value in ExtractComputedValue (The existing style struct internally represents "none" with a 0 value, for this property)
 - adds code to handle "none" for single-valued properties UncomputeValue, as discussed a bit in bug 520396.
Attachment #405283 - Flags: review?(dbaron)
This patch layers on top of the others to support SMIL animation of the "font-size-adjust" property.

Patch is pretty simple.  The changes are:
 - Uncomment the property's line in IsPropertyAnimatable.
 - Add a special case to nsSMILCSSValueType::Add, to prevent us from adding "font-size-adjust" values, since it's an explicitly non-additive property.[1] (This effectively means we can't do "by" animation on it.)
 - Enable existing "from-to" mochitests
 - Add some "from-by" mochitests (in which the animation should have no effect, since it's a non-additive property).

[1] http://www.w3.org/TR/SVG/text.html#FontSizeAdjustProperty
and http://dev.w3.org/SVG/profiles/1.1F2/publish/propidx.html#note1
Attachment #405298 - Flags: review?(roc)
When adding the special case in nsSMILCSSValueType::Add in "patch 4", I noticed that the logic in that method can be simplified a bit.

Currently, this method allows for either of its nsSMILValue arguments (aDest & aValueToAdd) to be just-initialized "identity" values.

HOWEVER -- in actuality, I don't think this is needed for the "aValueToAdd" argument.  Whenever we add two values (in "from-by" animation, pure "by" animation, and in cumulative animation with repeats), the amount we're adding is going to be a value that we've parsed -- not a freshly-initialized identity-value.

So, it should be safe to assert that aValueToAdd is a parsed value (with meaningful values set in its ValueWrapper struct).  This patch includes an NS_ABORT_IF_FALSE to that effect, and it removes the special handling that's no longer required, given this assumption.  This passes reftests & mochitests.

(Note that this makes Add()'s logic more closely match that of ComputeDistance() and Interpolate(), in the nsSMILCSSValueType class.)
Attachment #405303 - Flags: review?(roc)
Summary: Extend nsStyleAnimation to support float values → Extend nsStyleAnimation to support float values for SVG/SMIL-Animatable properties
Comment on attachment 405233 [details] [diff] [review]
patch 1: support floats in nsStyleAnimation

r=dbaron, except you'll also need to change layout/style/test/test_transitions_per_property.html (adding appropriate entries for these properties) to avoid causing test failures (including on tinderbox, since transitions landed last night)
Attachment #405233 - Flags: review?(dbaron) → review+
Comment on attachment 405283 [details] [diff] [review]
patch 3: support "font-size-adjust" in nsStyleAnimation

>+    offsetof(nsStyleFont, mFont) + offsetof(nsFont, sizeAdjust),

This can just be offsetof(nsStyleFont, mFont.sizeAdjust).

>+      if (aProperty == eCSSProperty_font_size_adjust &&
>+          aComputedValue.GetFactorValue() == 0.0f) {
>+        // For 'font-size-adjust', Mozilla's CSS engine internally uses the "0"
>+        // value to represent the "none" keyword.  Here, we have to treat this
>+        // as a keyword instead of a float value, to make sure we don't end up
>+        // doing interpolation with it.
>+        aComputedValue.SetNoneValue();
>+      }

Don't talk about "Mozilla's CSS engine" as if it's some external piece of code.  Just make the first sentence "In nsStyleFont, we set mFont.sizeAdjust to 0 to represent font-size-adjust: none."



You'll also need to change layout/style/test/test_transitions_per_property.html for this too.

r=dbaron with that
Attachment #405283 - Flags: review?(dbaron) → review+
Per comment 6 & comment 7, this patch updates test_transitions_per_property.html to include the newly-animatable properties from this bug. (It also fixes a typo at the bottom of the test -- needed a s/length/color/.)

I've addressed the other review comments from comment 7 in my patch queue:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/51dc1e963f46
Attachment #405329 - Flags: review?(dbaron)
(oops -- header comment for "test_float_aboveOne_transition()" mentioned the wrong range.  Fixed now)
Attachment #405329 - Attachment is obsolete: true
Attachment #405331 - Flags: review?(dbaron)
Attachment #405329 - Flags: review?(dbaron)
Attachment #405331 - Flags: review?(dbaron) → review+
Comment on attachment 405331 [details] [diff] [review]
patch 6a: add newly-supported float properties to test_transitions_per_property.html

Looks good, except it's probably better to merg this into patches 1 (all but one line) and 3, so that the individual changesets each pass tests.
Ok -- I was planning on just pushing patches 1/3/5 all together as one push, so there'd still be no tinderbox bustage cycles.  But you're right that it's probably better to have each individual changeset pass tests.

I'll merge that in and land modified patches 1 & 3, and then will land 2, 4 & 5 when they get review.
(In reply to comment #11)
> Ok -- I was planning on just pushing patches 1/3/5 all together as one push
(Oops -- I meant 1/3/6, not 1/3/5)
Great stuff!
Assignee: nobody → dholbert
You need to log in before you can comment on or make changes to this bug.