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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

nsStyleAnimation needs to be extended to support float values (e.g. "opacity", "font-size-adjust")

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: patch 1: support floats in nsStyleAnimation

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.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch 2: support floats in SMIL code & enable tests

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.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: patch 3: support "font-size-adjust" in nsStyleAnimation

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.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch 4: support "font-size-adjust" in SMIL code & enable tests

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

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: patch 5: simplify logic in nsSMILCSSValueType::Add

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.)

Comment 6

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

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)

Comment 7

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

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

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: patch 6: add newly-supported float properties to test_transitions_per_property.html

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

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: patch 6a: add newly-supported float properties to test_transitions_per_property.html

(oops -- header comment for "test_float_aboveOne_transition()" mentioned the wrong range.  Fixed now)

Comment 10

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

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.

Comment 11

[Daniel Holbert [:dholbert]]

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.

Comment 12

[Daniel Holbert [:dholbert]]

(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)

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Great stuff!

Comment 14

[Daniel Holbert [:dholbert]]

Thanks for the reviews!

Merged patch v6 into patches 1 and 3 per comment 10 & 11:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/309282d857d7

and landed patches 1 through 5 (with review fixes, already noted above):
  http://hg.mozilla.org/mozilla-central/rev/95e9942fd51d
  http://hg.mozilla.org/mozilla-central/rev/ad31fa9481e5
  http://hg.mozilla.org/mozilla-central/rev/376c52889be7
  http://hg.mozilla.org/mozilla-central/rev/b5bc4eff9e68
  http://hg.mozilla.org/mozilla-central/rev/bd0cf8eea1ca