Closed Bug 523196 Opened 10 years ago Closed 10 years ago

implement support for *-shadow in nsStyleAnimation

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

Attached patch patchSplinter Review
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
Attachment #407092 - Flags: superreview?(bzbarsky)
Attachment #407092 - Flags: review?(dholbert)
> +  NS_ABORT_IF_FALSE(inset1 == inset2, "should match");

Why?
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 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.
Attachment #407092 - Flags: superreview?(bzbarsky) → superreview+
Attached patch patch addendumSplinter Review
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.
Attachment #407288 - Flags: superreview?(bzbarsky)
Attachment #407288 - Flags: review?(dholbert)
Attachment #407288 - Flags: superreview?(bzbarsky) → superreview+
Duplicate of this bug: 520490
Attachment #407092 - Flags: review?(dholbert) → review+
Attachment #407288 - Flags: review?(dholbert) → review+
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 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.
(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).
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Listed as one of the animatable properties.
Blocks: 1286151
You need to log in before you can comment on or make changes to this bug.