Extend nsStyleAnimation to support CSS rect() values

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
8 years ago
7 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

8 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)

Updated

8 years ago
Assignee: nobody → dbaron
(Assignee)

Comment 1

8 years ago
Created attachment 409467 [details] [diff] [review]
patch
Attachment #409467 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

8 years ago
I just realized SMIL will also need Auto support in UncomputeValue, but that can come in a later patch...
(Reporter)

Comment 3

8 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?
(Assignee)

Comment 4

8 years ago
That depends whether we support calc(0.6 * auto + 0.4 * 40px) :-)
(Reporter)

Comment 5

8 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

8 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

8 years ago
Blocks: 530983
(Assignee)

Comment 7

8 years ago
(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

8 years ago
That makes sense to me.
(Assignee)

Comment 9

8 years ago
Created attachment 414898 [details] [diff] [review]
patch

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+
(Assignee)

Comment 11

8 years ago
http://hg.mozilla.org/mozilla-central/rev/bfe4de8f0428
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Updated

7 years ago
Blocks: 537142
You need to log in before you can comment on or make changes to this bug.