603917 - SMIL animation on "fill" fails when base value is a paint server (even when base value isn't ultimately needed)

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[jonathan chetwynd]

when using xsltProcessor.transformToFragment styles in the root document are followed by mozilla, however when using set they are not. wfm Opera and Safari

Comment 1

[jonathan chetwynd]

URL testcase demonstrates styles are used, blackStoneFill is styled in root document, however they should have been 'set' to whiteStoneFill.

Comment 2

[jonathan chetwynd]

Attachment: xml file

Comment 3

[jonathan chetwynd]

Attachment: xslt stylesheet

Comment 4

[jonathan chetwynd]

Attachment: root document

Comment 5

[jonathan chetwynd]

http://www.peepo.com is the use case.

I have not attempted to link the three attachments, as in the past this did not work for me when using xsltProcessor.transformToFragment

Comment 6

[jonathan chetwynd]

in fact set is broken for styles in fragments, amending testcases

Comment 7

[jonathan chetwynd]

now the blackStoneFill is correctly loaded from root document,
but 'set' fails to use style in xsl stylesheet.

in fact neither style in root or xsl stylesheet are used.

finally note visibility is used to show that 'set' does effect visibility

Comment 8

[jonathan chetwynd]

Attachment: xsl stylesheet

Comment 9

[jonathan chetwynd]

Attachment: xslt stylesheet

Comment 10

[jonathan chetwynd]

changed style to 'red' to simplify testcase

Comment 11

[Robert Longson [:longsonr]]

What do I do with the attached files?

Furthermore why is this an SVG bug and not XSLT. If you can't produce a single file SVG example then it probably doesn't belong to CORE->SVG.

Comment 12

[Robert Longson [:longsonr]]

You seem to have produced an SVG file that has set nodes underneath the g this is not a valid SVG file. Do you want to retarget this bug to some other component or shall I close it as invalid?

Comment 13

[jonathan chetwynd]

Robert,

http://honte.eu/bugs/mozilla/set-xslt/test-all.svg

validates at w3c, and is similar to the result of the transform,

Comment 14

[jonathan chetwynd]

#11
set fill is the only part that is broken
set visibility is fine.

I cannot see how this is part of xslt, as the transform is correct,

might this be related to the issue that set is initiated onload,
whereas the xslt is added later? this has come up before.

Comment 15

[Robert Longson [:longsonr]]

Use DOM inspector to look at the generated code. It looks like

<g>
   <set />
   <set />
   <set />
   <set />
</g>

Whatever you've generated is not SVG so this is not an SVG bug.

Comment 16

[Robert Longson [:longsonr]]

Hmm wait a minute maybe I'm wrong there. I'll take another look.

Comment 17

[jonathan chetwynd]

please try the url:

http://honte.eu/bugs/mozilla/set-xslt/test.svg

 and compare in different browsers

Comment 18

[jonathan chetwynd]

mozilla displays the result fine, it is visible, just the style is wrong, it has not been set to red

Comment 19

[jonathan chetwynd]

bug 386856 is the one I remembered #14 which may be relevant.

the potential relation being that set refers to timing, hence onload which  is addressed there.

Comment 20

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Hi Jonathan,
Sorry, I don't have time to look at this now, I'll look tomorrow. But have you tried using the syntax 'rgb(255,0,0)' instead of 'red'

Comment 21

[jonathan chetwynd]

Attachment: xsl stylesheet - reduced

Comment 22

[jonathan chetwynd]

Attachment: xsl stylesheet - reduced

Comment 23

[jonathan chetwynd]

Attachment: xml file

Comment 24

[jonathan chetwynd]

Attachment: xml file

Comment 25

[jonathan chetwynd]

Attachment: root document

Comment 26

[jonathan chetwynd]

excuse bug-spam, now the root document attachment should show 2 green circles.
the URL has an additional test 'redder' with the linearGradient in the xslt stylesheet, that mozilla also fails

There seems to be more than one way to break the fill styling, these are just an example. clearly set visibility and fill, work in some circumstances. in my experiments it was not clear that this was limited to linearGradient.

Comment 27

[jonathan chetwynd]

added test to url, dark green circle should be changed to #reddish

Comment 28

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Reduced test case

Reduced test case.

Comment 29

[Brian Birtles (:birtles, Feb 26~ parental leave)]

The problem appears to be that if the base fill is a gradient GetBaseValue fails when we try to animate it. When that happens the compositor simply bails out:

http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILCompositor.cpp#110

However, for <set> animation it shouldn't even be necessary to read the base value since <set> never interpolates from a base value, nor is it ever additive.

Comment 30

[Brian Birtles (:birtles, Feb 26~ parental leave)]

This seems fixable with a bit of special casing in nsSMILCompositor.
e.g.
a) In ComposeAttribute, get the first function to affect the sandwich (we already do this)
b) If WillReplace() returns true on that function, then proceed with a null smil value of some sort.

We'll need special handling later on and in nsSMILAnimationFunction plus a few assertions to make sure that null value doesn't crop up where we're not expecting it, plus I'm not sure yet how we'll work out what type it should be in advance, but at a glance anyway, it seems fixable and worth doing.

Comment 31

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1a

First attempt at fixing this.

This patch reworks our compositing so that null base values are only checked at the last moment. It means that in cases where we're replacing the base value we don't need to worry if that base value is able to be represented as an nsSMILValue or not.

This patches also leverages this flexibility to skip fetching the base value if we won't need it which I imagine is a useful optimisation when animating CSS properties as it saves us the unset style/get base value/reset style step we normally do there.

It does however mean that if the first animation function in the sandwich fails unexpectedly the next animation will operate on a null base value rather than the actual base value.

Until error handling in SVG is more widely agreed upon I think this is reasonable behaviour. (Currently SVG's error handling is quite strict and most vendors don't adhere, sometimes importing SVG 1.2's more lax behaviour.) It is a trivial matter to remove this optimisation later if need be.

Note that all the behaviour in the original test case is not covered by this patch. Particularly the behaviour in comment 27 of setting a gradient fill still will not work (only animating over the top of a gradient fill). I will file a separate bug for this (although bug 552087 is perhaps the same?)

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 486250 [details] [diff] [review]
Patch v1a

>+++ b/content/smil/nsSMILCSSProperty.cpp
>@@ -106,24 +106,21 @@ nsSMILCSSProperty::GetBaseValue() const
>     // We can't look up the base (computed-style) value of shorthand
>     // properties, because they aren't guaranteed to have a consistent computed
>     // value.  However, that's not a problem, because it turns out the caller
>     // isn't going to end up using the value we return anyway. Base values only
>     // get used when there's interpolation or addition, and the shorthand
>     // properties we know about don't support those operations. So, we can just
>-    // return a dummy value (initialized with the right type, so as not to
>-    // indicate failure).
>+    // return a null value.
[...]
>-    nsSMILValue tmpVal(&nsSMILCSSValueType::sSingleton);
>-    baseValue.Swap(tmpVal);
>     return baseValue;

I have mixed feelings about this chunk, because the documentation in nsISMILAttr.h for GetBaseValue still says that a null-typed return-value indicates failure (even if we care less about failure now).  So as long as this method intends to return "success", it should be returning a nsSMILValue with a non-null type.

Alternately -- since (with your patch) GetBaseValue() failure isn't as big of a deal now, you could leave this as-is but tweak the comment above it to effectively say "Can't get shorthand property's base value -- FAIL! (but failure won't cause problems because this value won't get used)"

You should do one or the other of these things -- either keep it nsSMILCSSValueType-typed (to indicate weak "success"), or make it Null-typed & edit the comment to indicate that we're failing & that it's OK.

>diff --git a/content/smil/nsSMILCompositor.cpp b/content/smil/nsSMILCompositor.cpp
>+++ b/layout/reftests/svg/smil/anim-fill-overpaintserver-2.svg
>+    <animate attributeName="fill" to="lime" dur="10s"/>

Could you increase 10s there to, say, 500s?

I'm probably being paranoid, but I think it's possible (though pretty unlikely) that 11s might elapse between pageload & the snapshot-time (maybe on a mobile device under huge load), and we don't want sporadic failures from that.  500s is a safer "never-actually-reached" animation duration for a reftest, I think.

r=dholbert with the above addressed.

Comment 33

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1b, r=dholbert

Address review feedback. Thanks Daniel!

Comment 34

[Daniel Holbert [:dholbert]]

Comment on attachment 487466 [details] [diff] [review]
Patch v1b, r=dholbert

>@@ -101,29 +101,27 @@ nsSMILCSSProperty::GetBaseValue() const
>+    // In either case, simply returning null to indicate failure is acceptable
>+    // because the caller only uses base values when there's interpolation or
>+    // addition, and for both the shorthand properties we know about and the
>+    // display property those operations aren't supported.
>     return baseValue;

Nit: s/returning null/returning a null-typed nsSMILValue/

Otherwise looks good! :)

Comment 35

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1c; r=dholbert

Fix nit from comment 34.

Comment 36

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2 - Fix transform assertion

The original patch (v1c) was causing assertions on TryServer.

Specifically, the assertions described in bug 535691. Rather than getting 0-2 assertions as expected we were getting up to 10 in some cases.

The problem appears to be that nsSVGTransformSMILAttr::ClearAnimValue was occasionally running into trouble when notifying observers.

In that file it seems we normally call DidAnimateTransform on the SVG element that owns the transform, except in the case of clearing the transform where we call Will/DidModify on the transform. Occasionally we were doing this in a state where the observable wasn't known. I haven't analysed exactly what circumstances this arises in.

With the original patch (v1c) we end up calling ClearAnimValue more frequently and hence run into this assertion more frequently.

I'm attaching a new patch, Part 2, which revises nsSVGTransformSMILAttr::ClearAnimValue to use DidAnimateTransform instead. This patch fixes the assertions for me (including fixing bug 535691 if we decide to go with this).

Will run it through TryServer now to check it works on other platforms too.

Comment 37

[Daniel Holbert [:dholbert]]

Comment on attachment 489085 [details] [diff] [review]
Part 2 - Fix transform assertion

Looks good to me.

Comment 38

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/edf3071a4fdb
http://hg.mozilla.org/mozilla-central/rev/4e06966e8dfe

Comment 39

[Robert Longson [:longsonr]]

Did part 2 fix bug 531550 too?

Comment 40

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #39)
> Did part 2 fix bug 531550 too?

I'm afraid not :(

Comment 41

[jonathan chetwynd]

will this be in tomorrow's trunk nightly?

Comment 42

[Daniel Holbert [:dholbert]]

Yes.

(Since I've seen you ask that question on a few bugs now -- for future reference, the answer to that question is basically always "yes", if you see that a bug has had its patch checked in to mozilla-central & has been marked "fixed".  Unless of course it gets backed out, but if that were to happen, you'd see another comment mentioning the backout.)

Comment 43

[jonathan chetwynd]

are there tests at w3c for xslt transforms?

if not please make sure you cover the bases, rather than relying on bug files.
I also have things to do....

I already stated that I had only included 'some' of the possible variations.
obviously at some stage it is likely that someone(s) will get around to it.
in practice it just needs a little careful thought.

in #27 I refer to a test: 
http://honte.eu/bugs/mozilla/set-xslt/test.svg
that remains broken for latest mozilla builds, 
number 4,
though the other tests now pass.

in fact this is the test in the original description, 
hence my decision to reopen.

There remains a small suite of further tests that need to be checked,
anyway, please check before closing my bugs,
not sure whether thanks are due ~:"

Comment 44

[Robert Longson [:longsonr]]

This bug has a fix that has landed. The rules are that it can only be reopened if the fix is backed out. If you still have issues you must create another bug.

Comment 45

[jonathan chetwynd]

the fix needs to be backed out, or should not have landed.

whatever may be fixed it is not this bug.

Robert, please back-off.

you may be well intentioned but...

Comment 46

[jonathan chetwynd]

description of bug clearly states:

when using xsltProcessor.transformToFragment styles in the root document are
followed by mozilla, however when using set they are not. wfm Opera and Safari

this has not been fixed at all.

what has been fixed is where the style is in the style sheet, which is a different matter.

Comment 47

[Robert Longson [:longsonr]]

Do not reopen this bug. Raise a new one.

Comment 48

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #43)
> in #27 I refer to a test: 
> http://honte.eu/bugs/mozilla/set-xslt/test.svg
> that remains broken for latest mozilla builds, 
> number 4,
> though the other tests now pass.

Jonathan, please note my comment 31:

> Note that all the behaviour in the original test case is not covered by this
> patch. Particularly the behaviour in comment 27 of setting a gradient fill
> still will not work (only animating over the top of a gradient fill). I will
> file a separate bug for this (although bug 552087 is perhaps the same?)

Specifically, the separate bug I filed is bug 607537 on which you're cc'ed. I think that bugs covers the remaining missing functionality right?

Comment 49

[jonathan chetwynd]

Brian,

many thanks, for filing a bug, I had not received any notice of this, which is poor communication.

There are a considerable number of people at Mozilla who consider they are above filing bugs, I consider this grossly irresponsible.

Daniel and Robert's responses insisting I file a bug, when I had already created a broad bug with a variety of tests and explanation, and the fact that I was unaware that you had filed a bug had led me to consider dropping Mozilla bug filing and support until 2012.

Mozilla needs to consider this, as I for one find the hierarchical systems embedded by Mozilla most unreasonable.

Comment 50

[Robert Longson [:longsonr]]

Broad bugs are unlikely to be completely fixed in all cases. We cannot rewrite everything all at once.

There are a considerable number of volunteers who give up their free time to help you it's unfair of you to demand that they put any more time in than they have for this free service to you.

We're not above filing bugs either the point is that if you file the bug you will get notified when things happen on it and fling bugs should pretty straigtforward for someone that's been around as long as you have.

Comment 51

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Hi Jonathan,

Sorry you didn't get the bugmail for that bug I filed, I've got no idea why that would be. At any rate, you're right, it would have been good to mention that bug in the comments here. I'll make sure to do so next time. Sorry about that.