520325 - Extend nsStyleAnimation to support "none" value (which has its own unit-type)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

I'm filing this bug on supporting the "none" CSS value (and its corresponding unit-type) in the nsStyleAnimation class.

In current mozilla-central, I think this change will only affect paint-valued properties ("fill" and "stroke").  But as we support more properties, they'll need to be able to use the "none" value as well.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: patch v1

Here's a patch to add support for "none".  It's taken from my patch queue, here:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/f8de24b9fc75/style-animation-none

This patch does three things:

 - Make Add/Interpolate/ComputeDistance fail when we're dealing with "none", since it's a non-additive/non-interpolatable keyword.

 - Make GetCommonUnit resolve any conflicts that involve eStyleUnit_None by always returning eStyleUnit_None.  This ensures that we'll fall into the above-mentioned failure clauses in Add/Interpolate/ComputeDistance, when one of our input values is "none".

 - Handle "none" paint-values in ExtractComputedValue and StoreComputedValue.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

How does this improve our behavior over simply not dealing with the values?

Comment 3

[Daniel Holbert [:dholbert]]

Well, the goal is that an animation endpoint of "none" will result in us having a computed value of "none" at the correct time.

Currently, this doesn't happen for a variety of reasons.

The most proximate problem (which prevents us from hitting the others) is that ExtractComputedValue will fail (return PR_FALSE) on "none" paint-values right now. SMIL code interprets to mean that something went wrong, and so it ignores the animation.

However, even if SMIL were to live with this failure and try to animate anyway, it'd only have an uninitialized nsStyleCoord (with null unit) for the "none" value.  When it passes this into e.g. Interpolate and UncomputeValue, those methods would both barf on this value.  For one thing, they call GetUnit() on it, which would assert.  Interpolate would spam a warning about being "Unable to find a common unit for interpolation".  UncomputeValue would simply return PR_FALSE without populating its string outparam.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Here's a testcase, to be viewed in a current mozilla-central build (since this afternoon's landing of bug 474049) with the "svg.smil.enabled" pref flipped on.

In mozilla-central, the right rect doesn't animate -- it just sticks with its default fill value of "red".

Opera 10 renders it correctly -- both rects blink between green and lime. (technically "none" for the right rect, which makes it transparent & show the lime background).

mozilla-central with this bug's patch renders it correctly, too.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ah, so calcMode="discrete" is for just toggling between values rather than actually doing any movement through the space between them?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So do you really need to change GetCommonUnit?  Can you make this work even if GetCommonUnit is unchanged?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

But removing the NS_WARNING in the general case might be good.  I don't think we should print warnings to the console for possibly-silly although maybe-ok content.  So if the point of the change was to stop the warning, maybe just remove the warning completely?

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> Ah, so calcMode="discrete" is for just toggling between values rather than
> actually doing any movement through the space between them?

Correct - I added that for simplicity, so that both animations would have the same expected behavior.  (It actually has no effect on the second animation -- that one should actually *always* do discrete animation, regardless of the calcMode attribute, since we can't interpolate to/from "none".)

(In reply to comment #6)
> So do you really need to change GetCommonUnit?  Can you make this work even if
> GetCommonUnit is unchanged?

Yes, aside from the potential warning-spam that you mention in comment 7.

(In reply to comment #7)
> But removing the NS_WARNING in the general case might be good.  I don't think
> we should print warnings to the console for possibly-silly although maybe-ok
> content.  So if the point of the change was to stop the warning, maybe just
> remove the warning completely?

Yeah, I guess it's reasonable to remove the warnings.

So with that change, GetCommonUnit() would return eStyleUnit_Null to mean "Skip any interpolation/addition/distance-computation, because the given type(s) don't support it."  That'll probably be the case for most units (and most unit-pairings), so it's probably good to make that the default fallback behavior, with no warnings.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, so I guess this looks good with two changes:

 1) reverting the changes to GetCommonUnit and instead removing the warning

 2) removing the additional "return PR_FALSE" in StoreComputedValue, and instead asserting for the PaintServer type that the unit is one of the two you expect, i.e.:

if (aComputedValue.GetUnit() == eStyleUnit_Color) {
  ...
} else {
  NS_ASSERTION(aComputedValue.GetUnit() == eStyleUnit_None, "unexpected unit");
  ...
}

Could you post a revised patch?

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: patch v2

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 404453 [details] [diff] [review]
patch v2

>       if (paint.mType == eStyleSVGPaintType_Color) {
>         aComputedValue.SetColorValue(paint.mPaint.mColor);
>         return PR_TRUE;
>+      } else if (paint.mType == eStyleSVGPaintType_None) {
>+        aComputedValue.SetNoneValue();
>+        return PR_TRUE;

Change the " else " into a newline; no need for else-after-return.

r=dbaron with that

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: patch v2a

Fixed.

I also removed the initial "However" in the comment above that line, since the note before it is now gone, so the "However" no longer makes sense.

Here's the fixed patch -- carrying forward r+.

When I land this, I'll also land this patch with tests:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/8d4782c1556d/smil_css_styleanimation_none
It adds two mochitests for "none", and enables a third which was included but disabled in bug 474049's patch.

Comment 13

[Daniel Holbert [:dholbert]]

Landed with tests:
http://hg.mozilla.org/mozilla-central/rev/5b2d368cfc9f
http://hg.mozilla.org/mozilla-central/rev/8fb10a0a12da

I also corrected a comment typo in the mochitest before landing. (The typo was present in the tests patch revision linked in comment 12)
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/diff/b55f1f0a031a/smil_css_styleanimation_none