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

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

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

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

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

changed style to 'red' to simplify testcase

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.

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?

Robert, http://honte.eu/bugs/mozilla/set-xslt/test-all.svg validates at w3c, and is similar to the result of the transform,

#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.

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.

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

please try the url: http://honte.eu/bugs/mozilla/set-xslt/test.svg and compare in different browsers

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

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.

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'

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.

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

Reduced test case.

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.

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.

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 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.

Address review feedback. Thanks Daniel!

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! :)

Fix nit from comment 34.

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 on attachment 489085 [details] [diff] [review] Part 2 - Fix transform assertion Looks good to me.

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

Did part 2 fix bug 531550 too?

(In reply to comment #39) > Did part 2 fix bug 531550 too? I'm afraid not :(

will this be in tomorrow's trunk nightly?

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

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 ~:"

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.

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...

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.

Do not reopen this bug. Raise a new one.

(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?

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.

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.

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.