Closed Bug 520488 Opened 11 years ago Closed 11 years ago

Extend nsStyleAnimation to support CSS rect() values

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: dholbert, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

nsStyleAnimation (and probably nsStyleCoord?) need to be extended to support CSS rect() values. (e.g. for the clip property, which takes values like "rect(1px, 2px, 3px, 4px)")
Assignee: nobody → dbaron
Attached patch patch (obsolete) — Splinter Review
Attachment #409467 - Flags: review?(bzbarsky)
I just realized SMIL will also need Auto support in UncomputeValue, but that can come in a later patch...
Comment on attachment 409467 [details] [diff] [review]
patch

>@@ -543,16 +586,55 @@ nsStyleAnimation::AddWeighted(double aCo
[SNIP]
>+      if (rect1->mTop.GetUnit() != rect2->mTop.GetUnit() ||
>+          rect1->mRight.GetUnit() != rect2->mRight.GetUnit() ||
>+          rect1->mBottom.GetUnit() != rect2->mBottom.GetUnit() ||
>+          rect1->mLeft.GetUnit() != rect2->mLeft.GetUnit()) {
>+        // At least until we have calc()
>+        return PR_FALSE;
>+      }


I'm not sure that comment is correct.

In other cases, unit mismatches mean we have <length> vs <percent>, which indeed can be fixed using e.g. 
>  calc(coeff1 * lengthVal + coeff2 * pctVal)

However, in the case of rect() units (at least for 'clip'), I think the mismatch would have to be <length> vs "auto", and calc() won't get us anywhere with that, right?
That depends whether we support calc(0.6 * auto + 0.4 * 40px) :-)
Right - if that's a possibility, then disregard comment 3. :)  I just wasn't sure whether "calc(0.6 * auto)" was meaningful.  I guess it could be, in principle.
Comment on attachment 409467 [details] [diff] [review]
patch

>@@ -543,16 +586,55 @@ nsStyleAnimation::AddWeighted(double aCo
>+    case eUnit_CSSRect: {
[snip]
>+        switch ((rect1->*member).GetUnit()) {
[snip]
>+          case eCSSUnit_Auto:
>+            (result->*member).SetAutoValue();
>+            break;
[snip]
>+      aResultValue.SetAndAdoptCSSRectValue(result.forget(), eUnit_CSSRect);
>+      break;

So, the above code lets us successfully add "auto" subcomponent-values to each other (and produce "auto").

This means that the following two SMIL animations will both succeed (and will affect the computed style of "clip" for their duration):
  <animate attributeName="clip" begin="0" dur="1"
           from="rect(10px, 10px, 10px, auto)"
           by="rect(5px, 5px, 5px, auto)" 
  <animate attributeName="clip"  begin="0" dur="1"
           from="rect(auto, auto, auto, auto)"
           by="rect(auto, auto, auto, auto)"

This outcome goes against what I was expecting -- I was assuming that "by-animations" that involve "auto" should fail. For comparison, consider this similar example with the "fill" property:
  <animate attributeName="fill"  begin="0" dur="1"
           from="none"
           by="none">
This "none + none" animation will fail (and hence won't affect the computed style of "fill").  This makes intuitive sense, because it's not sensible to add "none" -- not even to itself.[1]

Despite that, are we saying that it's sensible to add "auto" to itself?

[1] To be fair, in the case of fill:none, the spec also explicitly says "Only RGB color values are additive."
http://www.w3.org/TR/SVG11/animate.html#AnimationAttributesAndProperties
Blocks: 530983
(In reply to comment #6)
> So, the above code lets us successfully add "auto" subcomponent-values to each
> other (and produce "auto").

Hmmm.  So I guess I want to check that the weights add up to 1 before doing that, and fail if they don't.
That makes sense to me.
Attached patch patchSplinter Review
Revised, per the previous comments, to only support AddWeighted on auto components of a rect when the coefficients sum to 1.
Attachment #409467 - Attachment is obsolete: true
Attachment #414898 - Flags: review?(bzbarsky)
Attachment #409467 - Flags: review?(bzbarsky)
Comment on attachment 414898 [details] [diff] [review]
patch

r=bzbarsky.  Sorry for the lag...
Attachment #414898 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/mozilla-central/rev/bfe4de8f0428
Status: NEW → RESOLVED
Closed: 11 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.