Extend nsStyleAnimation to support CSS rect() values

RESOLVED FIXED in mozilla1.9.3a1

Status

()

defect
P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: dholbert, Assigned: dbaron)

Tracking

(Blocks 1 bug)

Trunk
mozilla1.9.3a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

10 years ago
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
Posted 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...
Reporter

Comment 3

10 years ago
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) :-)
Reporter

Comment 5

10 years ago
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.
Reporter

Comment 6

10 years ago
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
Reporter

Updated

10 years ago
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.
Reporter

Comment 8

10 years ago
That makes sense to me.
Posted patch patch — — Splinter 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
Last Resolved: 10 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.