523196 - implement support for *-shadow in nsStyleAnimation

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

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

Attachment: patch

This patch adds support for the the first complex value type to nsStyleAnimation so that we can animate text-shadow and -moz-box-shadow.  It depends on bug 522852 and bug 523193, and also (within the patch) fixes one lifetime issue within nsTransitionManager.  This appears to validate that things actually work with complex value types.

The rules for animation of shadows are described in:
http://dev.w3.org/csswg/css3-transitions/#animation-of-property-types-
and the third item in the summary of:
http://lists.w3.org/Archives/Public/www-style/2009Jul/0050.html

Comment 1

[Boris Zbarsky [:bzbarsky]]

> +  NS_ABORT_IF_FALSE(inset1 == inset2, "should match");

Why?

Comment 2

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

I suppose I chould change:

+  if (color1.GetUnit() != color2.GetUnit() ||
+      inset1.GetUnit() != inset2.GetUnit()) {
+    // We don't know how to animate between color and no-color, or
+    // between inset and not-inset.
+    return PR_FALSE;
+  }

to just check inset1 == inset2.  There are only two possible states of inset1 and inset2:
  GetUnit() == eCSSUnit_Null
  GetUnit() == eCSSUnit_Enumerated && GetIntValue() == NS_STYLE_BOX_SHADOW_INSET

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 407092 [details] [diff] [review]
patch

Ah, I'd assumed that the inset was a distance for some reason.

OK, with that either rewritten to be clearer or documented, r=me.

Comment 4

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

Attachment: patch addendum

This should get merged into the other patch before landing, but I kept it separate for review since the first patch is partly reviewed.  I really didn't have any excuse to avoid smart pointers, so I should use them.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 407092 [details] [diff] [review]
patch

>+        const nsCSSValue& color1 = array1->Item(4);
>+        const nsCSSValue& color2 = array2->Item(4);
>+        const nsCSSValue& inset1 = array1->Item(5);
>+        const nsCSSValue& inset2 = array2->Item(5);
[snip]
>+AddShadowItems(double aCoeff1, const nsCSSValue &aValue1,
>+               double aCoeff2, const nsCSSValue &aValue2,

One nit -- the patch isn't 100% consistent on placement of the "&" character in references.  I think both styles (hug-the-type and hug-the-variable-name) are used in a few places in the patch. Ideally, it'd be nice to keep that consistent.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 407092 [details] [diff] [review]
patch

Sorry -- one other minor thing I just noticed:

>@@ -623,16 +803,64 @@ nsStyleAnimation::ExtractComputedValue(n
>+    case eStyleAnimType_Shadow: {
>+      const nsCSSShadowArray *shadowArray =
>+        *static_cast<const nsRefPtr<nsCSSShadowArray>*>(
>+          StyleDataAtOffset(styleStruct, ssOffset));
>+      if (!shadowArray) {
>+        aComputedValue.SetCSSValueListValue(nsnull, eUnit_Shadow, PR_TRUE);
>+        return PR_TRUE;
>+      }

Should this be aComputedValue.SetNoneValue()?  (since a null shadow-array in the style struct represents the value "none"[1])  Otherwise, I think we'd be able to interpolate between "none" and other values, which we've been disallowing so far.[2]  (Perhaps you're intentionally choosing to deviate from that rule here, though?  If so, we should probably document that.)

[1] http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h#706
[2] e.g. 'font-size-adjust', 'fill', 'stroke' -- all interpolatable properties, but their "none" value is non-interpolatable.

Comment 8

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

(In reply to comment #7)
> Should this be aComputedValue.SetNoneValue()?  (since a null shadow-array in
> the style struct represents the value "none"[1])  Otherwise, I think we'd be
> able to interpolate between "none" and other values, which we've been
> disallowing so far.[2]  (Perhaps you're intentionally choosing to deviate from
> that rule here, though?  If so, we should probably document that.)

I actually do want to animate between 'none' and other shadow values; I documented "(may be null)" in nsStyleAnimation.h for eUnit_Shadow, though maybe I should also add something else.

For shadows, 'none' is really just the empty list, and we have an algorithm for handling list length mismatches that handles empty lists (padding the shorter list with 0-offset transparent shadows).

Comment 9

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

http://hg.mozilla.org/mozilla-central/rev/c548148943bd

I ended up with the fix for the variable-whitespace consistency in the dasharray patch instead of this one.

Comment 10

[Eric Shepherd [:sheppy]]

Listed as one of the animatable properties.