Last Comment Bug 520488 - Extend nsStyleAnimation to support CSS rect() values
: Extend nsStyleAnimation to support CSS rect() values
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P2 normal with 1 vote (vote)
: mozilla1.9.3a1
Assigned To: David Baron :dbaron: ⌚️UTC-10
: Jet Villegas (:jet)
Depends on: 504652
Blocks: 537142 530983
  Show dependency treegraph
Reported: 2009-10-04 17:51 PDT by Daniel Holbert [:dholbert]
Modified: 2009-12-29 15:12 PST (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (25.16 KB, patch)
2009-10-30 20:26 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch (25.36 KB, patch)
2009-11-27 22:28 PST, David Baron :dbaron: ⌚️UTC-10
bzbarsky: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2009-10-04 17:51:18 PDT
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)")
Comment 1 David Baron :dbaron: ⌚️UTC-10 2009-10-30 20:26:43 PDT
Created attachment 409467 [details] [diff] [review]
Comment 2 David Baron :dbaron: ⌚️UTC-10 2009-10-30 20:31:43 PDT
I just realized SMIL will also need Auto support in UncomputeValue, but that can come in a later patch...
Comment 3 Daniel Holbert [:dholbert] 2009-11-24 15:50:24 PST
Comment on attachment 409467 [details] [diff] [review]

>@@ -543,16 +586,55 @@ nsStyleAnimation::AddWeighted(double aCo
>+      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?
Comment 4 David Baron :dbaron: ⌚️UTC-10 2009-11-24 15:54:15 PST
That depends whether we support calc(0.6 * auto + 0.4 * 40px) :-)
Comment 5 Daniel Holbert [:dholbert] 2009-11-24 16:00:12 PST
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 6 Daniel Holbert [:dholbert] 2009-11-24 19:45:20 PST
Comment on attachment 409467 [details] [diff] [review]

>@@ -543,16 +586,55 @@ nsStyleAnimation::AddWeighted(double aCo
>+    case eUnit_CSSRect: {
>+        switch ((rect1->*member).GetUnit()) {
>+          case eCSSUnit_Auto:
>+            (result->*member).SetAutoValue();
>+            break;
>+      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"
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."
Comment 7 David Baron :dbaron: ⌚️UTC-10 2009-11-24 20:05:48 PST
(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.
Comment 8 Daniel Holbert [:dholbert] 2009-11-24 20:18:18 PST
That makes sense to me.
Comment 9 David Baron :dbaron: ⌚️UTC-10 2009-11-27 22:28:42 PST
Created attachment 414898 [details] [diff] [review]

Revised, per the previous comments, to only support AddWeighted on auto components of a rect when the coefficients sum to 1.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2009-12-17 16:51:02 PST
Comment on attachment 414898 [details] [diff] [review]

r=bzbarsky.  Sorry for the lag...
Comment 11 David Baron :dbaron: ⌚️UTC-10 2009-12-21 14:01:58 PST

Note You need to log in before you can comment on or make changes to this bug.