The default bug view has changed. See this FAQ.

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

7 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

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

Comment 5

7 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

7 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

7 years ago
Blocks: 530983
(Assignee)

Comment 7

7 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

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

Comment 9

7 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

7 years ago
http://hg.mozilla.org/mozilla-central/rev/bfe4de8f0428
Status: NEW → RESOLVED
Last Resolved: 7 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.