Bug 79790

Created attachment 129252 [details] Patch v1

Created attachment 129255 [details] Patch v2 Forgot to remove one line, after the SVGUseElement rewrite, the setNeedsStyleRecalc is no longer needed.

I hope Andreas/Antti can have a look, if the CSS stuff is fine?

Comment on attachment 129255 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=129255&action=review CSS parts look fine to me, though I'd avoid introducing new names containing "Decl" if possible. > Source/WebCore/svg/SVGAnimationElement.cpp:339 > + if (value.isNull()) { > + targetElement->overrideStyle()->removeProperty(attributeName.localName(), ASSERT_NO_EXCEPTION); > return; > + } > + targetElement->overrideStyle()->setProperty(attributeName.localName(), value, emptyString(), ASSERT_NO_EXCEPTION); Can't you operate on the StylePropertySet here instead of forcing the creation of CSSOM wrappers?

Comment on attachment 129255 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=129255&action=review CSS parts look fine to me, though I'd avoid introducing new names containing "Decl" if possible. > Source/WebCore/svg/SVGAnimationElement.cpp:339 > + if (value.isNull()) { > + targetElement->overrideStyle()->removeProperty(attributeName.localName(), ASSERT_NO_EXCEPTION); > return; > + } > + targetElement->overrideStyle()->setProperty(attributeName.localName(), value, emptyString(), ASSERT_NO_EXCEPTION); Can't you operate on the StylePropertySet here instead of forcing the creation of CSSOM wrappers?

Comment on attachment 129255 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=129255&action=review > Source/WebCore/svg/SVGElement.h:112 > + CSSStyleDeclaration* overrideStyle() { return ensureOverrideStyleDecl()->ensureInlineCSSStyleDeclaration(this); } This is wrong. OverrideStyle is not exposed in the DOM. There are no valid reasons to instantiate an external API object (CSSStyleDeclaration) for it.

(In reply to comment #5) Thanks Andreas/Antti for your quick comments! > CSS parts look fine to me, though I'd avoid introducing new names containing "Decl" if possible. I named it "overrideStylePropertySet". > Can't you operate on the StylePropertySet here instead of forcing the creation of CSSOM wrappers? .. > This is wrong. OverrideStyle is not exposed in the DOM. There are no valid reasons to instantiate an external API object (CSSStyleDeclaration) for it. Wonderful, I didn't realize how far you already are with the refactoring - no more CSSOM wrappers needed to set/remove properties - that's excellent! It also simplifies my destruction logic, as I can guarantee that no CSS OM wrappers are built for my override style, I don't need to call clearParentElement at all, as there are no wrappers that could need a clear. Uploading a new patch soon...

Created attachment 129412 [details] Patch v3

Comment on attachment 129412 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=129412&action=review r=me, with some tweaks. > Source/WebCore/css/CSSStyleSelector.cpp:765 > +inline bool CSSStyleSelector::addMatchedPropertiesIfPossible(MatchResult& result, StylePropertySet* properties) > +{ Something like addElementStyleProperties would be a better name. Instead of returning a bool, this should take a bool argument that tells if the property set is cacheable (setting result.isCacheable = false if needed). > Source/WebCore/css/CSSStyleSelector.h:295 > + inline bool addMatchedPropertiesIfPossible(MatchResult&, StylePropertySet*); inline keyword here does nothing and should be removed.

Comment on attachment 129412 [details] Patch v3 Attachment 129412 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11686300 New failing tests: fast/parser/nested-fragment-parser-crash.html editing/deleting/pruning-after-merge-1.html inspector/elements/elements-panel-styles.html fast/css/text-align-webkit-match-parent.html fast/css/inline-properties-important.html

Created attachment 129434 [details] Patch v4 Fixed Anttis comments & cr-ews.

Comment on attachment 129434 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=129434&action=review > Source/WebCore/svg/SVGElement.h:111 > + StylePropertySet* overrideStylePropertySet() const; > + StylePropertySet* ensureOverrideStylePropertySet(); I would prefer name "overrideStyleProperties" here. That's the pattern we are following elsewhere.

(In reply to comment #12) > I would prefer name "overrideStyleProperties" here. That's the pattern we are following elsewhere. Renamed all instances of this - will include it when landing the patch.

Niko, can we discuss this before you land it please? There are some points that I want to discuss before you land the patch. First override style is not the best approach! Even if SMIL talks about override style, that is definitely not what we want. Also this is on the agenda for the SVG WG meeting tomorrow. We would like to come up with a new definition that possibly goes into CSS3 Animation. I hope I don't have to r- the patch in the meantime to prevent you from landing as is. I'd like to take a look at it as well after a meeting.

If you are not landing this as a whole, please separate the CSSStyleSelector part and land that. That is a nice refactoring!

(In reply to comment #14) > Niko, can we discuss this before you land it please? There are some points that I want to discuss before you land the patch. > > First override style is not the best approach! Even if SMIL talks about override style, that is definitely not what we want. Also this is on the agenda for the SVG WG meeting tomorrow. We would like to come up with a new definition that possibly goes into CSS3 Animation. You're mixing up things. When SMIL talks about "override style" it means the CSS OM getOverrideStyle() method, that neither we nor Firefox implement. See firefox bug: https://bugzilla.mozilla.org/show_bug.cgi?id=45424. Daniel Holbert wrote here in 2008 "FWIW, this feature -- getOverrideStyle() -- is probably needed to support SVG/SMIL Animation of CSS properties.". New comment from 2012: "(Just to follow up on this -- we didn't end up needing this for SMIL -- we ended up going with a custom SMIL-specific override stylesheet that's not exposed via the DOM at all. For reference, see bug 474049 comment 10, first bullet-point.)" That's exactly the same solution that I've chosen - this is not related to SMILs getOverrideStyle(). We just need a separated style declaration where we can apply CSS properties to, to avoid mutating the target style directly - that's all. We can of course now decide when to apply these rules. SMIL says after the inline style declarations have been applied - so that's what I'm doing, consistent with both Opera & FF. > I hope I don't have to r- the patch in the meantime to prevent you from landing as is. I'd like to take a look at it as well after a meeting. Heh, no need to r-, the patch is still valid.

Comment on attachment 129434 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=129434&action=review Sorry, you changed the behavior without testing it. r- A great patch in general. But I am still skeptical of shouldApplyAnimation. This needs more testing and verifications. I would also like to discuss the role of attributeType on SVG WG first. > Source/WebCore/svg/SVGAnimationElement.cpp:409 > + // For attributeType="auto", try CSS first. If that fails, try XML. Difficult, this might not be future confident. We will move more attributes to presentation attributes like 'transform', for theses properties, it might be better to use XML as default (in theory). So that we can use the XML parser for parsing. Alternatively we could extend SVG only rules to the CSS parser. Not sure if the last suggestion is the best approach. > Source/WebCore/svg/SVGAnimationElement.cpp:414 > + // If attributeName is unknown and ttributeType is not 'auto', don't apply the animation. s/ttributeType/attributeType/ > Source/WebCore/svg/SVGAnimationElement.cpp:415 > + if (attributeType == AttributeTypeCSS) <animate attributeName="width" attributeType="CSS"> won't animate? Did you check this on other browsers? This is a change of the current behavior! Did you add tests for that?

(In reply to comment #16) > (In reply to comment #14) > > Niko, can we discuss this before you land it please? There are some points that I want to discuss before you land the patch. > > > > First override style is not the best approach! Even if SMIL talks about override style, that is definitely not what we want. Also this is on the agenda for the SVG WG meeting tomorrow. We would like to come up with a new definition that possibly goes into CSS3 Animation. > You're mixing up things. When SMIL talks about "override style" it means the CSS OM getOverrideStyle() method, that neither we nor Firefox implement. We are on the same side! I know what FF is doing and we definitely don't want to use override style in the sense of DOM2 and SMIL animation. But that is exactly the point. We should come up with another name. SMIL animation style is not that bad actually. CSS animation uses CSS Animation as well. So why not align with it? > See firefox bug: https://bugzilla.mozilla.org/show_bug.cgi?id=45424. > Daniel Holbert wrote here in 2008 "FWIW, this feature -- getOverrideStyle() -- is probably needed to support SVG/SMIL Animation of CSS properties.". > > New comment from 2012: "(Just to follow up on this -- we didn't end up needing this for SMIL -- we ended up going with a custom SMIL-specific override stylesheet that's not exposed via the DOM at all. For reference, see bug 474049 comment 10, first bullet-point.)" Yes, it is not specified at the moment. I would like to have you on the call tomorrow so that we get a resolution for that. In my eyes it should go to CSS3 Animations, at the end it is just necessary that we know what we want to do. > > That's exactly the same solution that I've chosen - this is not related to SMILs getOverrideStyle(). We just need a separated style declaration where we can apply CSS properties to, to avoid mutating the target style directly - that's all. That sounds like CSS animation overrides SMIL animations. That is what FF is doing. Because CSS animation/transition modify RenderStyle, it sounds hard to do it the other way around (SVG animation overrides CSS animations). That would be great to know. > > We can of course now decide when to apply these rules. SMIL says after the inline style declarations have been applied - so that's what I'm doing, consistent with both Opera & FF. You can just partly compare Opera here, since they don't have CSS animations and transitions for SVG CSS properties implemented. This should definitely get viewed in a bigger scope with the whole animation sandwich model. > > > I hope I don't have to r- the patch in the meantime to prevent you from landing as is. I'd like to take a look at it as well after a meeting. > Heh, no need to r-, the patch is still valid. It is not. Sorry. You changed the behavior without testing it and I am not sure if the new behavior is correct, or should be correct. This needs more verification (also verification what other vendors are doing). Just some examples: XML attributes get not animated anymore when you use attributeType="CSS" Animate the XML attribute on CSS properties when you use attributeType="XML" which is absolutely inconsistent with FF and Opera. (For FF they animate the XML attribute as well in this case, but getAttribute() will still give you the base value, not the animatedVal what it will for you know).

(In reply to comment #18) > (In reply to comment #16) > > (In reply to comment #14) > > > Niko, can we discuss this before you land it please? There are some points that I want to discuss before you land the patch. > > > > > > First override style is not the best approach! Even if SMIL talks about override style, that is definitely not what we want. Also this is on the agenda for the SVG WG meeting tomorrow. We would like to come up with a new definition that possibly goes into CSS3 Animation. > > You're mixing up things. When SMIL talks about "override style" it means the CSS OM getOverrideStyle() method, that neither we nor Firefox implement.\ > We are on the same side! I know what FF is doing and we definitely don't want to use override style in the sense of DOM2 and SMIL animation. But that is exactly the point. We should come up with another name. SMIL animation style is not that bad actually. CSS animation uses CSS Animation as well. So why not align with it? Okay with "smilAnimationStyle" ? > Yes, it is not specified at the moment. I would like to have you on the call tomorrow so that we get a resolution for that. In my eyes it should go to CSS3 Animations, at the end it is just necessary that we know what we want to do. go to css3 anims?? > > > > > That's exactly the same solution that I've chosen - this is not related to SMILs getOverrideStyle(). We just need a separated style declaration where we can apply CSS properties to, to avoid mutating the target style directly - that's all. > That sounds like CSS animation overrides SMIL animations. That is what FF is doing. Because CSS animation/transition modify RenderStyle, it sounds hard to do it the other way around (SVG animation overrides CSS animations). That would be great to know. I didn't change what gets preference: it stayed the same. I'm just splitting of the "SMIL animation style" from the regular style, and keep the order as-it-is-now. > > > > We can of course now decide when to apply these rules. SMIL says after the inline style declarations have been applied - so that's what I'm doing, consistent with both Opera & FF. > You can just partly compare Opera here, since they don't have CSS animations and transitions for SVG CSS properties implemented. This should definitely get viewed in a bigger scope with the whole animation sandwich model. Right, for now I'm just correcting old mistakes: mutating the DOM directly. This can be avoided with a separated set of style properties, which is what I'm doing here. When it should be applied, before or after CSS animations/transitions etc. is out of scope for this patch. > > > I hope I don't have to r- the patch in the meantime to prevent you from landing as is. I'd like to take a look at it as well after a meeting. > > Heh, no need to r-, the patch is still valid. > It is not. Sorry. You changed the behavior without testing it and I am not sure if the new behavior is correct, or should be correct. This needs more verification (also verification what other vendors are doing). Just some examples: > > XML attributes get not animated anymore when you use attributeType="CSS" Erm, this is per spec? http://www.w3.org/TR/SVG/animate.html#TargetAttributes > Animate the XML attribute on CSS properties when you use attributeType="XML" which is absolutely inconsistent with FF and Opera. (For FF they animate the XML attribute as well in this case, but getAttribute() will still give you the base value, not the animatedVal what it will for you know). This is not correct, pleaser reread 19.2.5: attributeType = "CSS | XML | auto" Specifies the namespace in which the target attribute and its associated values are defined. The attribute value is one of the following (values are case-sensitive): CSS This specifies that the value of ‘attributeName’ is the name of a CSS property defined as animatable in this specification. XML This specifies that the value of ‘attributeName’ is the name of an XML attribute defined in the default XML namespace for the target element. If the value for ‘attributeName’ has an XMLNS prefix, the implementation must use the associated namespace as defined in the scope of the target element. The attribute must be defined as animatable in this specification. auto The implementation should match the ‘attributeName’ to an attribute for the target element. The implementation must first search through the list of CSS properties for a matching property name, and if none is found, search the default XML namespace for the element. The new shouldApplyAnimation is a transcription from this.

(In reply to comment #18) > It is not. Sorry. You changed the behavior without testing it and I am not sure if the new behavior is correct, or should be correct. This needs more verification (also verification what other vendors are doing). Just some examples: I still don't see where I changed behaviour? shouldApplyAnimation was refactoring existing behavior. Can you give an example?

Comment on attachment 129434 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=129434&action=review > Source/WebCore/svg/SVGAnimateElement.h:68 > + virtual void endedActiveInterval(); This name of this passive form is strange to me. activeIntervalEnded? > Source/WebCore/svg/SVGElementRareData.h:89 > + m_overrideStylePropertySet = 0; I think it has a .clear() method.

(In reply to comment #21) > (From update of attachment 129434 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129434&action=review > > > Source/WebCore/svg/SVGAnimateElement.h:68 > > + virtual void endedActiveInterval(); > > This name of this passive form is strange to me. activeIntervalEnded? I'd need to rename startedActiveInterval as well - I'd prefer to do this in another patch. > > Source/WebCore/svg/SVGElementRareData.h:89 > I think it has a .clear() method. Fixed. New patch with better ChangeLog coming soon.

Created attachment 129670 [details] Patch v5 Add new testcase covering attributeTypes, to demonstrate that I didn't change behaviour - works just like in trunk. Should please Dirks concerns, also adresses Zoltans suggestions.

Comment on attachment 129670 [details] Patch v5 thanks for the fixes. r=me

Committed r109342: <http://trac.webkit.org/changeset/109342>

The last patch did not consider any of my doubts! I have strongly to protest against the r+ from Zoltan. Zoltan you can not r+ a patch without reading the concerns of previews reviewers that denied a patch! 1. The patch changes behavior without layout tests 2. The patch changes the behavior in a significant way that we might not want to have in WebKit! It is potentially doing the opposite of what other viewers are doing! This either needs to get fixed as soon as possible or it needs to get rolled out! I vote for the later one, because the patch violates the rules of WebKit. Also a reviewer that adds concerns should have time to comment on a bug. You should give him at least a day. In the past we had the unwritten rule that if a reviewer denies a patch than the reviewer must have the chance to review any new patch. This is definitely not the case here! I reopen this bug, since it violates WebKit rules in any way. And Niko should know of these rules.

(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #16) > > > (In reply to comment #14) > > > > Niko, can we discuss this before you land it please? There are some points that I want to discuss before you land the patch. > > > > > > > > First override style is not the best approach! Even if SMIL talks about override style, that is definitely not what we want. Also this is on the agenda for the SVG WG meeting tomorrow. We would like to come up with a new definition that possibly goes into CSS3 Animation. > > > You're mixing up things. When SMIL talks about "override style" it means the CSS OM getOverrideStyle() method, that neither we nor Firefox implement.\ > > > We are on the same side! I know what FF is doing and we definitely don't want to use override style in the sense of DOM2 and SMIL animation. But that is exactly the point. We should come up with another name. SMIL animation style is not that bad actually. CSS animation uses CSS Animation as well. So why not align with it? > Okay with "smilAnimationStyle" ? Definitely! > > > Yes, it is not specified at the moment. I would like to have you on the call tomorrow so that we get a resolution for that. In my eyes it should go to CSS3 Animations, at the end it is just necessary that we know what we want to do. > go to css3 anims?? > > > > > > > > > That's exactly the same solution that I've chosen - this is not related to SMILs getOverrideStyle(). We just need a separated style declaration where we can apply CSS properties to, to avoid mutating the target style directly - that's all. > > That sounds like CSS animation overrides SMIL animations. That is what FF is doing. Because CSS animation/transition modify RenderStyle, it sounds hard to do it the other way around (SVG animation overrides CSS animations). That would be great to know. > I didn't change what gets preference: it stayed the same. I'm just splitting of the "SMIL animation style" from the regular style, and keep the order as-it-is-now. > > > > > > > We can of course now decide when to apply these rules. SMIL says after the inline style declarations have been applied - so that's what I'm doing, consistent with both Opera & FF. > > You can just partly compare Opera here, since they don't have CSS animations and transitions for SVG CSS properties implemented. This should definitely get viewed in a bigger scope with the whole animation sandwich model. > Right, for now I'm just correcting old mistakes: mutating the DOM directly. This can be avoided with a separated set of style properties, which is what I'm doing here. When it should be applied, before or after CSS animations/transitions etc. is out of scope for this patch. At first you wanted to correct the CSSOM mutation if I understand it correctly. And even if we definitely should not animate the inline style and even if it is the correct approach to add a new style for SMIL animations, it definitely changes behavior. Just changing the tests to use computed style instead of asking inline style is not enough! We need tests that the inline style stays the same. We need tests what happens if you set the inline style during an animation, for the both cases that we use additive="sum" for animations and that we don't use it (even if it might not matter since the base value is the intrinsic style). This needs comparison to other browsers. And if you add a new style sheet you have of course to know how it affects CSS animations or transitions! So we need a test case for this as well. See mail from Daniel Holbert about potential risks of "circle of death"[1]. This is definitely in the scope of this patch! > > > > > I hope I don't have to r- the patch in the meantime to prevent you from landing as is. I'd like to take a look at it as well after a meeting. > > > Heh, no need to r-, the patch is still valid. > > It is not. Sorry. You changed the behavior without testing it and I am not sure if the new behavior is correct, or should be correct. This needs more verification (also verification what other vendors are doing). Just some examples: > > > > XML attributes get not animated anymore when you use attributeType="CSS" > Erm, this is per spec? http://www.w3.org/TR/SVG/animate.html#TargetAttributes At first this patch is changing behavior in multiple ways. And you did not add tests for that even if I asked you to do that in my review. That you land the patch without giving the reviewer the change to re-review it who denies the patch is not fair play in WebKit. Even if I don't know if we have a strict rule that this should get reviewed by this reviewer again, we have at least an unwritten rule to give him the chance to review again. You know that! Second this part of the spec gets interpreted in different ways, why we have different implementations. Even with your patch you do it not according to the specification, because for attributeType="XML" you overwrite the presentation attribute value. The user can not get the base value during the animation. No browser is doing that as far as I know. Even Firefox does it different, which has the best implementation according to this rule as far as I can say. > > > Animate the XML attribute on CSS properties when you use attributeType="XML" which is absolutely inconsistent with FF and Opera. (For FF they animate the XML attribute as well in this case, but getAttribute() will still give you the base value, not the animatedVal what it will for you know). > > This is not correct, pleaser reread 19.2.5: > attributeType = "CSS | XML | auto" > Specifies the namespace in which the target attribute and its associated values are defined. The attribute value is one of the following (values are case-sensitive): > > CSS > This specifies that the value of ‘attributeName’ is the name of a CSS property defined as animatable in this specification. > > XML > This specifies that the value of ‘attributeName’ is the name of an XML attribute defined in the default XML namespace for the target element. If the value for ‘attributeName’ has an XMLNS prefix, the implementation must use the associated namespace as defined in the scope of the target element. The attribute must be defined as animatable in this specification. > > auto > The implementation should match the ‘attributeName’ to an attribute for the target element. The implementation must first search through the list of CSS properties for a matching property name, and if none is found, search the default XML namespace for the element. > > The new shouldApplyAnimation is a transcription from this. Now to the second part of your patch that should get separated from you original patch, since it has nothing to do with the mutation of CSS style (even if you interpret it that way). This is second time that you change the behavior without adding results. And it might also break existing content (even if it would be very rare). Int the past we did the following: bool attributeIsCSSProperty = isTargetAttributeCSSProperty(targetElement, attributeName); // Stop animation, if attributeType is set to CSS by the user, but the attribute itself is not a CSS property. if (!attributeIsCSSProperty && attributeType() == AttributeTypeCSS) return; ExceptionCode ec; if (attributeIsCSSProperty) { // FIXME: This should set the override style, not the inline style. // Sadly override styles are not yet implemented. targetElement->style()->setProperty(attributeName.localName(), value, "", ec); } else { // FIXME: This should set the 'presentation' value, not the actual // attribute value. Whatever that means in practice. targetElement->setAttribute(attributeName, value); } To the first if: We returned earlier, if the property is an XML attribute (no CSS property) but the user specified attributeType="CSS" which is ok. After that we don't care about what the other has specified. If the user specifies XML for an CSS property, we still animated the CSS property and not the presentation attribute. If the other specifies the attributeType CSS for an pure XML attribute, we still animate the attribute. You changes this significantly with the excuse of http://www.w3.org/TR/SVG/animate.html#TargetAttributes. Which is partly OK. But First: You changed the behavior according to the spec without testing it! Second: We don't support a lot of cases now, which we did support before. The behavior in the spec for CSS property and attributeType XML is quite unspecified. What should get animated for an SVG attribute if we don't have SVG DOM (baseVal and animVal) which is the case for presentation attributes.You animate the attribute directly, which overwrites the content of the attribute, which is not correct and against the behavior for SVG attributes with SVG DOM, against any existing implementation and definitely not specified that way! Third: SVG attributes (no CSS properties) can not animated if attributeType is CSS anymore. No test on your patch covers that. I bet you didn't even check what FF or Opera is doing. SVG Animation does not say that we shouldn't animate the attribute for this case! So you changed another peace without any technical reference! The second part of the patch makes me think we should roll out the patch and reland the first part but with enough tests! [1] http://lists.w3.org/Archives/Public/www-svg/2012Feb/0088.html

(In reply to comment #26) > The last patch did not consider any of my doubts! I have strongly to protest against the r+ from Zoltan. > > Zoltan you can not r+ a patch without reading the concerns of previews reviewers that denied a patch! Calm down a bit and relax :-) > > 1. The patch changes behavior without layout tests I added a testcase covering attributeType, to prove nothing changed compared to trunk. > 2. The patch changes the behavior in a significant way that we might not want to have in WebKit! It is potentially doing the opposite of what other viewers are doing! I don't see that but okay. > > This either needs to get fixed as soon as possible or it needs to get rolled out! I vote for the later one, because the patch violates the rules of WebKit. > > Also a reviewer that adds concerns should have time to comment on a bug. You should give him at least a day. In the past we had the unwritten rule that if a reviewer denies a patch than the reviewer must have the chance to review any new patch. This is definitely not the case here! Well we both thought that we adressed your comments, otherwise Zoltan wouldn't have r+ it, right? > I reopen this bug, since it violates WebKit rules in any way. And Niko should know of these rules. Sorry, we didn't know you still had concerns. Going to answer your next comment seperated

(In reply to comment #27) > > Okay with "smilAnimationStyle" ? > Definitely! Phew :-) > At first you wanted to correct the CSSOM mutation if I understand it correctly. And even if we definitely should not animate the inline style and even if it is the correct approach to add a new style for SMIL animations, it definitely changes behavior. Just changing the tests to use computed style instead of asking inline style is not enough! This patch should mark just the beginning of this work - I could do this in a branch if you prefer? I really don't mean to ship a version of WebKit with this, I should have been more clear on this. > We need tests that the inline style stays the same. I'm testing that everywhere. > We need tests what happens if you set the inline style during an animation, for the both cases that we use additive="sum" for animations and that we don't use it (even if it might not matter since the base value is the intrinsic style). Completely correct! We don't have testcases at the moment for this, but I started on this as you might have noticed: the anim-elem-0*-t-drt.js testcase framework. > This needs comparison to other browsers. It can be run in Opera/FF as well now, the whole harness! I compared against those of course, I should have written this down. > And if you add a new style sheet you have of course to know how it affects CSS animations or transitions! So we need a test case for this as well. Yes sure! But my patch doesn't change that, and as we have no test coverage for this, I prefered to let it this way for now! As you also know it's yet undecided what role CSS anims/transitions play, so I didn't want to include this here in this patch. >See mail from Daniel Holbert about potential risks of "circle of death"[1]. This is definitely in the scope of this patch! I read that as well :-) But that's really unrelated to the refactoring of these style properties IMHO. It for sure needs to be considred, but why in this patch? (To restate: the pure introduction of the SMIL animation style does NOT affect how CSS trafos/anims are applied, even the order stayed just the same). But you're right that I could have written a testcase for that as well to include here, to raise your worries. > > At first this patch is changing behavior in multiple ways. And you did not add tests for that even if I asked you to do that in my review. That you land the patch without giving the reviewer the change to re-review it who denies the patch is not fair play in WebKit. Even if I don't know if we have a strict rule that this should get reviewed by this reviewer again, we have at least an unwritten rule to give him the chance to review again. You know that! Really sorry that you feel this way - didn't mean to make you angry. Your initial comment was that you're concerned about the attributeType change, most specifically shouldApplyAnimation: "A great patch in general. But I am still skeptical of shouldApplyAnimation. This needs more testing and verifications. I would also like to discuss the role of attributeType on SVG WG first." And I added a test just for attributeType, as you asked for. So I felt I adressed your concerns, okay? > Second this part of the spec gets interpreted in different ways, why we have different implementations. Even with your patch you do it not according to the specification, because for attributeType="XML" you overwrite the presentation attribute value. The user can not get the base value during the animation. No browser is doing that as far as I know. Even Firefox does it different, which has the best implementation according to this rule as far as I can say. I can't follow here. For attributeType="XML" everything stayed the same? attributeType="XML" and animating eg. fill="" still goes through setAttribute() - so there's no change in behaviour. Sure that's wrong but I didn't change it. All our svg/animation testcases are runable in Opera as well, and work just fine there - we share the same behaviour now after this patch - I compared most of them manually. > > > Animate the XML attribute on CSS properties when you use attributeType="XML" which is absolutely inconsistent with FF and Opera. (For FF they animate the XML attribute as well in this case, but getAttribute() will still give you the base value, not the animatedVal what it will for you know). Animate the XML attribute on CSS properties when you use attributeType="XML" is as demanded by the spec. getAttribute() doesn't work correctly for us in that case, as we override the DOM value, and thus the 'baseVal'. I'm adressing that in a seperated patch series which enables animVal. For me these issues are completly de-coupled, and I wanted to minimize patch size. To restate again, please check again in detail, I didn't change attributeType="" behaviour at all. If I did there's a bug! Otherwise I would have added testcases for that... > Now to the second part of your patch that should get separated from you original patch, since it has nothing to do with the mutation of CSS style (even if you interpret it that way). This is second time that you change the behavior without adding results. And it might also break existing content (even if it would be very rare). Second part of my patch? > > Int the past we did the following: > > bool attributeIsCSSProperty = isTargetAttributeCSSProperty(targetElement, attributeName); > // Stop animation, if attributeType is set to CSS by the user, but the attribute itself is not a CSS property. > if (!attributeIsCSSProperty && attributeType() == AttributeTypeCSS) > return; > > ExceptionCode ec; > if (attributeIsCSSProperty) { > // FIXME: This should set the override style, not the inline style. > // Sadly override styles are not yet implemented. > targetElement->style()->setProperty(attributeName.localName(), value, "", ec); > } else { > // FIXME: This should set the 'presentation' value, not the actual > // attribute value. Whatever that means in practice. > targetElement->setAttribute(attributeName, value); > } > > To the first if: We returned earlier, if the property is an XML attribute (no CSS property) but the user specified attributeType="CSS" which is ok. > > After that we don't care about what the other has specified. If the user specifies XML for an CSS property, we still animated the CSS property and not the presentation attribute. If the other specifies the attributeType CSS for an pure XML attribute, we still animate the attribute. You changes this significantly with the excuse of http://www.w3.org/TR/SVG/animate.html#TargetAttributes. Which is partly OK. > First: You changed the behavior according to the spec without testing it! .. it's just a bug, I didn't mean to change behavior! You make it sound like I'm intentionally not writing testcases -- I also make mistakes, just to remind you ;-) > Second: We don't support a lot of cases now, which we did support before. The behavior in the spec for CSS property and attributeType XML is quite unspecified. What should get animated for an SVG attribute if we don't have SVG DOM (baseVal and animVal) which is the case for presentation attributes.You animate the attribute directly, which overwrites the content of the attribute, which is not correct and against the behavior for SVG attributes with SVG DOM, against any existing implementation and definitely not specified that way! Errrm, that's how we used to implement SMIL since the beginning? I'm just changing that! This patch covers animating CSS properties, and the animVal patch (to be uploaded next) covers the XML part. > Third: SVG attributes (no CSS properties) can not animated if attributeType is CSS anymore. No test on your patch covers that. I bet you didn't even check what FF or Opera is doing. SVG Animation does not say that we shouldn't animate the attribute for this case! So you changed another peace without any technical reference! I checked with Opera, but I seems I'm hit by a bug. I was under the impression that this was actually specified, so it's definitely wrong! Good catch after all, that's the part changing behavior that you meant. Anyhow, clearly a miscommunication, I thought you had concerns regarding attributeType="" that I covered with a testcase, but I missed the problematic part. Do you want to roll it-out or shall I post follow-up fixes? -- P.S. I don't understand your frustration though, we both thought your comments were adressed.

This has started to crash in Chromium debug layout tests: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2FW3C-SVG-1.1%2Fanimate-elem-32-t.svg

(In reply to comment #30) > This has started to crash in Chromium debug layout tests: > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2FW3C-SVG-1.1%2Fanimate-elem-32-t.svg Here's a sample stack trace: [44225:44225:16152245608809:ERROR:process_util_posix.cc(142)] Received signal 11 base::debug::StackTrace::StackTrace() [0x735fc6] base::(anonymous namespace)::StackDumpSignalHandler() [0x6efba9] 0x7f6527079af0 WebCore::SVGElementInstance::invalidateAllInstancesOfElement() [0x1b6b521] WebCore::SVGElementInstance::InvalidationGuard::~InvalidationGuard() [0x1b3e7af] WebCore::SVGCircleElement::svgAttributeChanged() [0x1b4c86d] WebCore::SVGElement::attributeChanged() [0x1b6661c] WebCore::Element::didModifyAttribute() [0xd5203d] WebCore::Element::setAttributeInternal() [0xd54fe8] WebCore::Element::setAttribute() [0xd4cc43] WebCore::setTargetAttributeAnimatedXMLValue() [0x1b49440] WebCore::SVGAnimationElement::setTargetAttributeAnimatedValue() [0x1b494f6] WebCore::SVGAnimateElement::applyResultsToTarget() [0x1b42a23] WebCore::SMILTimeContainer::updateAnimations() [0x1bfcae7] WebCore::SMILTimeContainer::timerFired() [0x1bfc0eb] WebCore::Timer<>::fired() [0x1bd7f5a] WebCore::ThreadTimers::sharedTimerFiredInternal() [0xf60dac] WebCore::ThreadTimers::sharedTimerFired() [0xf60ce3] webkit_glue::WebKitPlatformSupportImpl::DoTimeout() [0x1cb51ec] base::BaseTimer<>::TimerTask::Run() [0x1cb58f7] base::internal::RunnableAdapter<>::Run() [0x1f09f39] base::internal::InvokeHelper<>::MakeItSo() [0x1f09ebd] base::internal::Invoker<>::Run() [0x1f09e69] base::Callback<>::Run() [0x6ac077] MessageLoop::RunTask() [0x6c74e4] MessageLoop::DeferOrRunPendingTask() [0x6c75fb] MessageLoop::DoWork() [0x6c7e1d] base::MessagePumpGlib::RunWithDispatcher() [0x727108] base::MessagePumpGlib::Run() [0x7274e8] MessageLoop::RunInternal() [0x6c71ab] MessageLoop::RunHandler() [0x6c705e] MessageLoop::Run() [0x6c6993] webkit_support::RunMessageLoop() [0x631eab] TestShell::waitTestFinished() [0x4723ca] TestShell::runFileTest() [0x46aa4f] runTest() [0x433784] main [0x43428b] 0x7f6527064c4d 0x41fad9 None

(In reply to comment #31) > (In reply to comment #30) > > This has started to crash in Chromium debug layout tests: > > > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2FW3C-SVG-1.1%2Fanimate-elem-32-t.svg > > Here's a sample stack trace: Sorry, this was missing the assertion at the top: ASSERTION FAILED: (*it)->shadowTreeElement()->correspondingElement() third_party/WebKit/Source/WebCore/svg/SVGElementInstance.cpp(124) : static void WebCore::SVGElementInstance::invalidateAllInstancesOfElement(WebCore::SVGElement*)

Filed assertion failure as https://bugs.webkit.org/show_bug.cgi?id=80053

(In reply to comment #18) After some hours of good sleep, a summary of this patch, future work, and mistakes: > That sounds like CSS animation overrides SMIL animations. That is what FF is doing. Because CSS animation/transition modify RenderStyle, it sounds hard to do it the other way around (SVG animation overrides CSS animations). That would be great to know. Fair enough, I'll explain you what I have/had in mind. First of all lets crawl through matchAllRules() in CSSStyleSelector to precisely understand the order of cascades: (I'll mark steps irrelevant to our discussion with braces (..) ) - User-Agent rules are matched first (eg. our default svg.css) (- Then user style sheets (eg. injected by Safari, if I want to override defaults, or set completely custom styles applied to every page)) - Presentation attributes mapped from HTML/SVG XML DOM attributes to CSS properties (- "Additional" style declarations (only used by HTMLTable*Element) ) (- Another special case for HTML elements only: text-direction) - Author rules (all style sheets for this Document, etc.) - Inline style declarations - SMIL animation override style matchAllRules() is used by CSSStyleSelector::styleForElement(). It returns a new PassRefPtr<RenderStyle> corresponding to the matched style. This doesn't contain any information about CSS animations/transitions at this point. To outline how CSS animations/transitions are applied, a (simplified!) example is given: A CSS animation is running on a <div> element animating the 'left' CSS property from 0 to 100 in 4s, starting at 0s. Let's look at any time t > 0, and see how CSS animations are applied. The AnimationControllers animation timer fires (AnimationControllerPrivate::animationTimerFired) and calls AnimationControllerPrivate::updateAnimations(). Among other things, the most important to notice is that the <div> is marked for "setNeedsStyleRecalc(SyntheticStyleChange)" and then the Documents style is updated (document()->updateStyleIfNeeded()). This will cause the whole recalcStyle() machinery to run. Once it reaches the <div> it will create a new RenderStyle for it by asking CSSStyleSelector::styleForElement for it (this contains the whole user/author rules, inline rules, etc. see above). Then it calls renderer->setAnimatableStyle(newStyle). void RenderObject::setAnimatableStyle(PassRefPtr<RenderStyle> style) { if (!isText() && style) setStyle(animation()->updateAnimations(this, style.get())); else setStyle(style); } The new style is then passed to AnimationController::updateAnimations(), which will call style->setLeft(100 * currentProgress) on the passed in RenderStyle. CSS transitions/animations thus only affect the computed value (aka. the _current_ RenderStyle belonging to a certain renderer, reachable from CSS OM via getComputedStyle). I've simplified the whole process a bit, to only show the important things. Here's a sample backtrace that should be understandable with above comments: #0 WebCore::AnimationBase::blendProperties (anim=0x1082905e0, prop=1082, dst=0x108241dd0, a=0x1082906f0, b=0x1082908f0, progress=0.12588589191436766) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/page/animation/AnimationBase.cpp:1233 #1 0x0000000102488732 in WebCore::KeyframeAnimation::animate (this=0x1082905e0, targetStyle=0x10829ba30, animatedStyle=@0x7fff5fbfd848) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/page/animation/KeyframeAnimation.cpp:191 #2 0x00000001015d6b99 in WebCore::CompositeAnimation::animate (this=0x1082904a0, renderer=0x108290268, currentStyle=0x10852b590, targetStyle=0x10829ba30) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/page/animation/CompositeAnimation.cpp:303 #3 0x000000010146c2b3 in WebCore::AnimationController::updateAnimations (this=0x108828898, renderer=0x108290268, newStyle=0x10829ba30) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/page/animation/AnimationController.cpp:530 #4 0x0000000102789a0d in WebCore::RenderObject::setAnimatableStyle (this=0x108290268, style=@0x7fff5fbfd9c0) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/rendering/RenderObject.cpp:1681 #5 0x000000010254a963 in WebCore::Node::setRenderStyle (this=0x10828fa90, s=@0x7fff5fbfdb08) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/dom/Node.cpp:1452 #6 0x0000000101a44b14 in WebCore::Element::recalcStyle (this=0x10828fa90, change=WebCore::Node::NoChange) .... To summarize: I kept stating that my patch doesn't affect the behavior of how CSS animation/transitions are applied. The patch only changes things in the CSSStyleSelector stage -- CSS anims/transitions always come last in WebKit . They don't "override" a style, they only act on the RenderStyles currently set in the RenderObjects, which is completely different from SMIL. Another SMIL1/2/3/ quote: When animation is applied to CSS properties of a particular element, the base value to be animated is read using the (readonly) getComputedStyle() method on that element. The values produced by the animation are written into an override stylesheet for that element, which may be obtained using the getOverrideStyle() method. Note that it is assumed that before reading the value, the override stylesheet is cleared so that the animation works on the original document value. These new values then affect the cascade and are reflected in a new computed value (and thus, modified presentation). This means that the effect of animation overrides all style sheet rules, except for user rules with the !important property. This enables !important user style settings to have priority over animations, an important requirement for accessibility. Note that the animation may have side effects upon the document layout. -- Most noticeable: "which may be obtained using the getOverrideStyle() method" - it doesn't say you need to use getOverrideStyle() for this. Both my patch & FF implement this using an override style sheet, but not obtained through getOverrideStyle() - it's just not needed, and not implemented for both of us. Next thing: "These new values then affect the cascade and are reflected in a new computed value (and thus, modified presentation)." That's exactly what I've done: The SMIL animation style is hooked into the CSSStyleSelector part, thus affecting the RenderStyle currently present in the renderers, and thus also the computed style". Now you could argue, why did I apply the SMIL animation style after the inline style declarations? "This means that the effect of animation overrides all style sheet rules..." It overrides all style sheet rules, but it doesn't override CSS anims/transitions, as they're applied at a later stage. I think, despite bugs of course, the new design is correct. NOTE: It would be costly, but possible to apply SMIL animations after CSS anims, but I think we should settle down on the former, as FF already does it that way, and we follow. Of course things like Daniels death-circle still need to be considered and fixed, no doubt. I hope that sheds some more light in the dark. I've realized that I should have written this mail _before_ submitting my patch for review, that would have saved the trouble. > > We can of course now decide when to apply these rules. SMIL says after the inline style declarations have been applied - so that's what I'm doing, consistent with both Opera & FF. > You can just partly compare Opera here, since they don't have CSS animations and transitions for SVG CSS properties implemented. This should definitely get viewed in a bigger scope with the whole animation sandwich model. Exactly! I've quoted http://www.w3.org/TR/smil-animation/#AnimationSandwichModel above. It stayed roughly the same across SMIL1-3. The CSS property part I'm referring to at least - it's really also consistent this way, I like the FF solution, and followed it. Wow, I've completely missed those two comments below! I didn't realize you actually posted these before I submitted patch v5. > XML attributes get not animated anymore when you use attributeType="CSS" Gotcha, so sad we have no test coverage for this. I'm happy that the new harness makes it easy, fortunately, to write such tests - and compare with other browsers (which was not possible before). > Animate the XML attribute on CSS properties when you use attributeType="XML" which is absolutely inconsistent with FF and Opera. (For FF they animate the XML attribute as well in this case, but getAttribute() will still give you the base value, not the animatedVal what it will for you know). Ditto. I made attributeType mandatory, which is wrong. I'll address this ASAP. Phew, tricky topic. Long post.

(In reply to comment #34) > > XML attributes get not animated anymore when you use attributeType="CSS" > Gotcha, so sad we have no test coverage for this. I'm happy that the new harness makes it easy, fortunately, to write such tests - and compare with other browsers (which was not possible before). Erm, I should have added that the testcase svg/animations/attributeTypes.svg that I've added in trunk, covers this: <!-- 'width' is a XML attribute, attributeType is set to "CSS". 'width' is not a presentation attribute, so this animation won't run. --> <rect x="150" width="100" height="100" fill="green"> <animate id="an2" attributeType="CSS" attributeName="width" fill="freeze" from="100" to="10" begin="0s" dur="4s"/> </rect> You're right that neither SVG nor SMIL say that the animation should stop if the attributeType is wrong. But that's actually what both Opera & FireFox nighties are doing! And I didn't change that behavior at all, see, your own snippet: Int the past we did the following: bool attributeIsCSSProperty = isTargetAttributeCSSProperty(targetElement, attributeName); // Stop animation, if attributeType is set to CSS by the user, but the attribute itself is not a CSS property. if (!attributeIsCSSProperty && attributeType() == AttributeTypeCSS) return; My example above animates 'width', which is not a css property, so attributeIsCSSProperty=false. And as attributeType is set to CSS, the animation is not applied. I preserved this behavior in my patch, proofed with the new testcase. Do you agree? (Did you misread the new code?) > > Animate the XML attribute on CSS properties when you use attributeType="XML" which is absolutely inconsistent with FF and Opera. (For FF they animate the XML attribute as well in this case, but getAttribute() will still give you the base value, not the animatedVal what it will for you know). I misread, about this. I still think this is a fix. When attributeType="XML" is specified, we're currently using the old legacy setAttribute() code path which mutates the DOM. Once animVal is enabled, setAttribute() call would go away, and the underlying base value wouldn't be affected. If you want, my patch aligns us more with FF, we can only fix it fully if we enable animVal, and avoid DOM mutations for XML attribute changes. (I have a local patch which differentiates between ApplyCSSAnimation/ApplySVGAnimation, where the former operates on the SMIL animation style and the latter purely on SVGLength, SVGLengthList, etc. objects in the SVG DOM - this is the ultimate goal of all of this). > Ditto. I made attributeType mandatory, which is wrong. I'll address this ASAP. Reply to myself: Nothing to adress, it's correct as-is. Unless you can proof me wrong :-)

I don't reply line by line on every single statement, otherwise the bug report gets to long. Your general description is covered by the graphic at [1]. An image is sometimes better than a long description :) But two things where I still oppose. You are not doing it like Firefox: 1) CSS properties with attributeType CSS: FF has an override style and it comes after the intrinsic style[2] directly, just like you described it. BUT CSS Animations _override_ SVG animations, they are not applied both, but just one of both. And both interact directly on intrinsic style! If you specify both, than CSS animations wins. 2) CSS properties with attributeType XML don't animate the XML attribute but just the presentation attribute style[1]. You won't see the animated value with getAttribute() till the end of an animation. And it will just affect the presentation attribute value at the end of an animation if fill="freeze" was set. And the second point makes me sad. I even dislike the behavior of FF, but your new introduced behavior is even worst. I wouldn't over interpret the description of attributeType in the spec. I assume that it was introduced to help viewers to decide if an animated property is CSS or XML. Talking to some of the authors here at Adobe, that was really the intention. The authors did not respect that we have to check the type ourselves anyway and we can't trust the user input. Nowadays I would interpret the property as "For XML use the SVG parser to parse the values, for CSS use the CSS parser to parse values and follow the animation rules of SMIL 3.0 Animation" which always animates the the intrinsic style. And that you changed the code to fallback to setAttribute animation is completely wrong for me. I would like that you change that back. Opera for instance doesn't animate the presentation attribute nor its style. That is the best approach in my eyes. Otherwise all animations get override by author styles, but we still have to animate the value which is stupid and a totally overhead. I will try to get this on the agenda of the next week SVG WG meeting. I still think you should go back to our old behavior for now, so that other SVG guys don't assume that we think that it is the way to go. Because I don't think that it was the intention of the SVG spec authors nor that it is the desired behavior. But we definitely need to have test of all new behaviors. And please verify that your patch does not cause the assertions. Thanks. [1] http://www.w3.org/Graphics/fx/wiki/SVG_attribute_to_presentation_attribute [2] http://www.w3.org/TR/css3-animations/#animation-behavior-

(In reply to comment #36) > 1) CSS properties with attributeType CSS: FF has an override style and it comes after the intrinsic style[2] directly, just like you described it. BUT CSS Animations _override_ SVG animations, they are not applied both, but just one of both. And both interact directly on intrinsic style! If you specify both, than CSS animations wins. The same is true in WebKit, with and without my patch. > 2) CSS properties with attributeType XML don't animate the XML attribute but just the presentation attribute style[1]. You won't see the animated value with getAttribute() till the end of an animation. And it will just affect the presentation attribute value at the end of an animation if fill="freeze" was set. Correct. > And the second point makes me sad. I even dislike the behavior of FF, but your new introduced behavior is even worst. Which new behavior? I have to ask again, because you're not replying on this. I am consistent with Opera & FF. > I wouldn't over interpret the description of attributeType in the spec. I assume that it was introduced to help viewers to decide if an animated property is CSS or XML. Talking to some of the authors here at Adobe, that was really the intention. The authors did not respect that we have to check the type ourselves anyway and we can't trust the user input. Nowadays I would interpret the property as "For XML use the SVG parser to parse the values, for CSS use the CSS parser to parse values and follow the animation rules of SMIL 3.0 Animation" which always animates the the intrinsic style. > And that you changed the code to fallback to setAttribute animation is completely wrong for me. I would like that you change that back. *SIGH* Again, which change? You've stated several times that I changed something, and I replied several times that I see no change. Please create a testcase which proves that my patch has changed something. Otherwise this is getting nowhere. > Opera for instance doesn't animate the presentation attribute nor its style. That is the best approach in my eyes. Otherwise all animations get override by author styles, but we still have to animate the value which is stupid and a totally overhead. > > I will try to get this on the agenda of the next week SVG WG meeting. I still think you should go back to our old behavior for now, so that other SVG guys don't assume that we think that it is the way to go. Because I don't think that it was the intention of the SVG spec authors nor that it is the desired behavior. Please proof with links. > But we definitely need to have test of all new behaviors. And please verify that your patch does not cause the assertions. Thanks. Will do.

(In reply to comment #37) > > 2) CSS properties with attributeType XML don't animate the XML attribute but just the presentation attribute style[1]. You won't see the animated value with getAttribute() till the end of an animation. And it will just affect the presentation attribute value at the end of an animation if fill="freeze" was set. Do you mean this is now happening after my patch, or before the patch, or shall this be the correct behaviour? ... > > And the second point makes me sad. I even dislike the behavior of FF, but your new introduced behavior is even worst. <rect width="100" height="100" fill="red"> <animate attributeName="fill" attributeType="XML" from="red" to="green" dur="3s" fill="freeze" /> </rect> This example is using ApplyXMLAnimation, as attributeType="XML", and thus uses setAttribute() in both the old trunk before my patch, and now. Behaviour didn't change for this. (Of course that should only mutate the animVal, never the baseVal, but that's another topic)

You're for sure thinking of http://www.w3.org/Graphics/fx/wiki/SVG_attribute_to_presentation_attribute: Issue 5: Different behavior on attributeType="XML" and attributeType="CSS" <rect width="100" height="100" fill="green"> <animate attributeName="fill" attributeType="XML|CSS" from="red" to="green" dur="3s" /> </rect> Solutions: (Recommend) No difference between CSS and XML, both animate the animated presentation attribute style (see Issue 4) -- For attributeType="CSS" we'd animate the pres attr style, and for attributeType="XML" we'd use the XML attribute at present. This can be changed of course, but this is the behaviour that also existed before my patch....

(In reply to comment #39) > You're for sure thinking of http://www.w3.org/Graphics/fx/wiki/SVG_attribute_to_presentation_attribute: > Issue 5: Different behavior on attributeType="XML" and attributeType="CSS" > > <rect width="100" height="100" fill="green"> > <animate attributeName="fill" attributeType="XML|CSS" from="red" to="green" dur="3s" /> > </rect> > > Solutions: (Recommend) No difference between CSS and XML, both animate the animated presentation attribute style (see Issue 4) > -- > > For attributeType="CSS" we'd animate the pres attr style, and for attributeType="XML" we'd use the XML attribute at present. This can be changed of course, but this is the behaviour that also existed before my patch.... if (!attributeIsCSSProperty && attributeType() == AttributeTypeCSS) return; ExceptionCode ec; if (attributeIsCSSProperty) { // FIXME: This should set the override style, not the inline style. // Sadly override styles are not yet implemented. targetElement->style()->setProperty(attributeName.localName(), value, "", ec); } else { // FIXME: This should set the 'presentation' value, not the actual // attribute value. Whatever that means in practice. targetElement->setAttribute(attributeName, value); } Again the same code snippet. No, it did not exist before. We always animated the inline style, never the attribute! It is new behavior in WebKit that you introduced. I can copy the same snippet a third time if zoo want. But it will result in the same conclusion: For SVG presentation attributes we _always_ animated the inline style, but never the attribute value.

(In reply to comment #37) > (In reply to comment #36) > > > 1) CSS properties with attributeType CSS: FF has an override style and it comes after the intrinsic style[2] directly, just like you described it. BUT CSS Animations _override_ SVG animations, they are not applied both, but just one of both. And both interact directly on intrinsic style! If you specify both, than CSS animations wins. > The same is true in WebKit, with and without my patch. That would be sad, because than your patch would do nothing ;) You created the new style which FF calls override style, even if it is not override style. You called in smilAnimationStyle. But it is not the same like the old code. And more, you are contrary to your previous comment. In your long description you said that UA styles, author styles and smile animation get to the computed style in RenderStyle. This is what CSS animations take into account. So SMIL animations are not overridden but belong to the "base value" for CSS animations (my tests seem to verify that). This is different to Firefox. In Firefox the result on the screen is as if you would not have SMIL animations applied, even if they are running in the background.

(In reply to comment #40) > No, it did not exist before. We always animated the inline style, never the attribute! It is new behavior in WebKit that you introduced. I can copy the same snippet a third time if zoo want. But it will result in the same conclusion: For SVG presentation attributes we _always_ animated the inline style, but never the attribute value. I agree that this is the desired behaviour. Let's rephrase: Consider we have proper SMIL support, that never mutates the DOM, but only "presentation values" (aka. SMIL override style for CSS, and animVal for any SVGLength, etc. animation). Once we have that, we have no option but animating the inline style for presentation attributes like "fill". Now why did I change this? Before we animated the fill CSS property, which gets mapped to the fill presentation attribute, and vis-a-vis. Before my patch, getAttribute("fill") was actually reflecting the current animation state, while animating, due the mapping. Now with the SMIL override style, that doesn't happen anymore. So in order to keep the current behaviour, I decided to animate the fill XML attribute, so that the changes are still visible. I am aware that this is not correct, but I wanted to preserve _existing_ behaviour. *sigh* I should work faster on the animVal patch, which resolves all of this.

(In reply to comment #41) > (In reply to comment #37) > > (In reply to comment #36) > > > > > 1) CSS properties with attributeType CSS: FF has an override style and it comes after the intrinsic style[2] directly, just like you described it. BUT CSS Animations _override_ SVG animations, they are not applied both, but just one of both. And both interact directly on intrinsic style! If you specify both, than CSS animations wins. > > The same is true in WebKit, with and without my patch. > That would be sad, because than your patch would do nothing ;) You created the new style which FF calls override style, even if it is not override style. You called in smilAnimationStyle. But it is not the same like the old code. And more, you are contrary to your previous comment. In your long description you said that UA styles, author styles and smile animation get to the computed style in RenderStyle. Get to the? You mean contribute to the computed style? If so, that's true. > This is what CSS animations take into account. So SMIL animations are not overridden but belong to the "base value" for CSS animations (my tests seem to verify that). Exactly! And that behaviour stayed the same. Previously the SMIL animation were part of the "author rules" now they're in their own smil animation rules set, applied _after_ the inline decls, before it was applied before the inline style decls. That's the only change. It does NOT affect CSS anims/trafos in any way. The behaviour stayed the same. > This is different to Firefox. In Firefox the result on the screen is as if you would not have SMIL animations applied, even if they are running in the background. I didn't compare this to FF, I only assured that my patch preserves current WebKit behavior. If you don't see that my patch doesn't affect CSS animation/transitions, you should re-read the long description and/or debug yourself, to convince yourself, that this stayed the same.

Ok, let's move forward with this. A concrete proposal: 1. I'll bake out the controversial ApplyXMLAnimation change, to let presentation attributes with attributeType="XML" animated through CSS again. (even this has some downsides until animVal is fully done) 2. My animVal patch is done for SVGLength & SVGLengthList, the two hardest candidates. In general all properties & properties lists (except SVGPathSegList, another topic) are ready for animVal locally now. I plan to turn on animVal support step by step, one type after the other. I'll put up a fully working patch, covering only SVGLength (for simplicity), to bug 12437 (animations update baseVal instead of animVal) to show you how everything falls together. What this fixes: I've contained testcases that change the baseValue of an animation while the animation is running, inspecting the SVG DOM effects, when either fill="remove" or fill="freeze" is set. An ideal candidate for testing baseValue mutations by script, while a SMIL animation is running, is additive="sum" + values animation. <rect x="10"> <animate dur="6s" additive="sum" values="0; 50; 100"/>.. At t=0s (before anim runs), x is 10 At t>0s <= 3s it animates from x=10 to x=60 (originally from 0..50 but 10 has to be added as its the base value). At t>3s <= 6s it animates from x=60 to x=110 This already works fine in WebKit trunk w/o any of these patches. What we don't support is changing the baseValue like rect.setAttribute("x", "666") while the animation is running, it had no effect on additive="sum"+values animation, as we cache the base values only once in SMILTimeContainer, and never see any changes to that. Using my animVal patch I don't need to utilize this code path anymore ( as restoring to base value happens automatically when animation ends (updateAnimVal logic)). Example: normally if we reach t=5s, we're at x = 100/(6s)*(5s) + 10 = 93.333333. If I now change the base value via rect.x.baseVal.value to 100 (while the additive-values animation is running) it properly recalculates x = 100 / (6s)*(5s) + 100 = 183.33333. This already works in FF & Opera, now we support it as well. ... Once we agree the draft patch is the way to go.... 3) Split up the draft patch into smaller patches, preparing animVal support (for for properties, then lists, then turn on SVGLength + change/add tests, then turn on SVGLengthList + change/add tests, etc..) Good plan?

(In reply to comment #43) > > This is different to Firefox. In Firefox the result on the screen is as if you would not have SMIL animations applied, even if they are running in the background. > I didn't compare this to FF, I only assured that my patch preserves current WebKit behavior. > > If you don't see that my patch doesn't affect CSS animation/transitions, you should re-read the long description and/or debug yourself, to convince yourself, that this stayed the same. That is correct, but we run in to consensus here before. That is not what I am worry anymore beside the missing description in the Changelog. But don't say that this is what FF is doing, because it isn't. (In reply to comment #42) > (In reply to comment #40) > > No, it did not exist before. We always animated the inline style, never the attribute! It is new behavior in WebKit that you introduced. I can copy the same snippet a third time if zoo want. But it will result in the same conclusion: For SVG presentation attributes we _always_ animated the inline style, but never the attribute value. > > I agree that this is the desired behaviour. > > Let's rephrase: > Consider we have proper SMIL support, that never mutates the DOM, but only "presentation values" (aka. SMIL override style for CSS, and animVal for any SVGLength, etc. animation). > > Once we have that, we have no option but animating the inline style for presentation attributes like "fill". > > Now why did I change this? > Before we animated the fill CSS property, which gets mapped to the fill presentation attribute, and vis-a-vis. Before my patch, getAttribute("fill") was actually reflecting the current animation state, while animating, due the mapping. > > Now with the SMIL override style, that doesn't happen anymore. So in order to keep the current behaviour, I decided to animate the fill XML attribute, so that the changes are still visible. > > I am aware that this is not correct, but I wanted to preserve _existing_ behaviour. > *sigh* I should work faster on the animVal patch, which resolves all of this. NO IT IS NOT EXISTING BEHAVIOR! We never mutated DOM for presentation attributes! At least not as far as I understand you comment that we changed the value of the attribute. What we did is element.style.property = animValue; but independent of the attributeType value! And getAttribute("fill") did never reflect animate values but the base value of the attribute, because the attribute points to the presentation attribute style and animations are done on the inline style! I don't know how often I should repeat that. It seems that you did not understand how animation of presentation attributes worked. (In reply to comment #44) > Ok, let's move forward with this. A concrete proposal: > 1. I'll bake out the controversial ApplyXMLAnimation change, to let presentation attributes with attributeType="XML" animated through CSS again. (even this has some downsides until animVal is fully done) Which are the downsides? You can access the base value of the presentation *attribute* and with the new style it does not affect inline style. > > 2. My animVal patch is done for SVGLength & SVGLengthList, the two hardest candidates. In general all properties & properties lists (except SVGPathSegList, another topic) are ready for animVal locally now. I plan to turn on animVal support step by step, one type after the other. That is great. Remember that current presentation attributes don't have SVG DOM interfaces (no animVal or baseVal). But it is a good investment in the future to think about that now. > > I'll put up a fully working patch, covering only SVGLength (for simplicity), to bug 12437 (animations update baseVal instead of animVal) to show you how everything falls together. Great. > > What this fixes: > I've contained testcases that change the baseValue of an animation while the animation is running, inspecting the SVG DOM effects, when either fill="remove" or fill="freeze" is set. > An ideal candidate for testing baseValue mutations by script, while a SMIL animation is running, is additive="sum" + values animation. I agree. > > <rect x="10"> <animate dur="6s" additive="sum" values="0; 50; 100"/>.. > At t=0s (before anim runs), x is 10 > At t>0s <= 3s it animates from x=10 to x=60 (originally from 0..50 but 10 has to be added as its the base value). > At t>3s <= 6s it animates from x=60 to x=110 > > This already works fine in WebKit trunk w/o any of these patches. What we don't support is changing the baseValue like rect.setAttribute("x", "666") while the animation is running, it had no effect on additive="sum"+values animation, as we cache the base values only once in SMILTimeContainer, and never see any changes to that. Using my animVal patch I don't need to utilize this code path anymore ( as restoring to base value happens automatically when animation ends (updateAnimVal logic)). > > Example: normally if we reach t=5s, we're at x = 100/(6s)*(5s) + 10 = 93.333333. > If I now change the base value via rect.x.baseVal.value to 100 (while the additive-values animation is running) it properly recalculates x = 100 / (6s)*(5s) + 100 = 183.33333. > > This already works in FF & Opera, now we support it as well. > ... > Once we agree the draft patch is the way to go.... Point two is great, nevertheless I don't see the relation to our current discussion for presentation attributes. > > 3) Split up the draft patch into smaller patches, preparing animVal support (for for properties, then lists, then turn on SVGLength + change/add tests, then turn on SVGLengthList + change/add tests, etc..) > > Good plan? It is. But not what I criticize on this patch. To summaries the presentation attribute changes, since animVal and baseVal are unrelated to you SVG DOM changes: 1) I do not question the biggest change, the new style for SMIL animations. That is great! But it needs testing. As far as I can see there is no test that inline style gets not changed during SMIL animations. Which must be tested, because of the behavior change. Correct me if I am wrong. 2) Brake out the changes for ApplyXMLAnimation, or at least use your SMIL animation style independent of the attributeType value for presentation attributes. Mutating the DOM attribute is not desired, against the specification and not done by any other SVG viewer.

(In reply to comment #45) This is really wrong: > > Before we animated the fill CSS property, which gets mapped to the fill presentation attribute, and vis-a-vis. Before my patch, getAttribute("fill") was actually reflecting the current animation state, while animating, due the mapping. > > Now with the SMIL override style, that doesn't happen anymore. So in order to keep the current behaviour, I decided to animate the fill XML attribute, so that the changes are still visible. .... I realize now that my getAttribute() example from above only works for non-presentation attributes. Especially for fill, you are right, we never animated any XML property - for eg. "x" attribute of <rect> we did that. Now why didn't I spot this logic error when you pasted the code? 346 bool attributeIsCSSProperty = isTargetAttributeCSSProperty(targetElement, attributeName); 347 // Stop animation, if attributeType is set to CSS by the user, but the attribute itself is not a CSS property. 348 if (!attributeIsCSSProperty && attributeType() == AttributeTypeCSS) 349 return; I somehow had in mind that isTargetAttributeCSSProperty always returned false for attributeType="XML". That's my wrong assumption, which lead to the whole confusion. SCNR... > I don't know how often I should repeat that. It seems that you did not understand how animation of presentation attributes worked. I did understand that, but not together with attributeType="XML". For that we now get it wrong with my patch, agreed. I'll switch presentation attributes animations back to CSS, no matter what attributeType says. 410 case AttributeTypeCSS: 411 // If attributeName is a known animatable SVG CSS property, apply the animation. 412 if (isTargetAttributeCSSProperty(targetElement, attributeName)) 413 return ApplyCSSAnimation; 414 // If attributeName is unknown and ttributeType is not 'auto', don't apply the animation. 415 if (attributeType == AttributeTypeCSS) 416 return DontApplyAnimation; 417 // For attributeType="auto", try XML animation now. 418 case AttributeTypeXML: 419 return ApplyXMLAnimation; It's just a matter of moving the case AttributeTypeXML below the AttributeTypeCSS case. > 1) I do not question the biggest change, the new style for SMIL animations. That is great! But it needs testing. As far as I can see there is no test that inline style gets not changed during SMIL animations. Which must be tested, because of the behavior change. Correct me if I am wrong. That is tested in my patch, correct me if I'm wrong. I'm checking both getComputedStyle(rect) and rect.style.fill, to verify only the former gets affected, not the latter. > 2) Brake out the changes for ApplyXMLAnimation, or at least use your SMIL animation style independent of the attributeType value for presentation attributes. Mutating the DOM attribute is not desired, against the specification and not done by any other SVG viewer. I'd like you to understand that I had a misconception about animating presentation attributes with attributeType="XML" set. I assumed that would go through the setAttribute() code path already, so that's why I said I'm only preserving "existing behaviour". That's of course wrong. We only used the setAttribute() code path for NON presentation attributes, and attributeType="XML" set. I was aware before that _any_ DOM mutation is actually wrong. But as I was under the impression we mutated DOM for presentation attributes, I tried to keep this behaviour in the patch.

(In reply to comment #46) > (In reply to comment #45) > > This is really wrong: > > > Before we animated the fill CSS property, which gets mapped to the fill presentation attribute, and vis-a-vis. Before my patch, getAttribute("fill") was actually reflecting the current animation state, while animating, due the mapping. > > > Now with the SMIL override style, that doesn't happen anymore. So in order to keep the current behaviour, I decided to animate the fill XML attribute, so that the changes are still visible. > .... > > I realize now that my getAttribute() example from above only works for non-presentation attributes. Especially for fill, you are right, we never animated any XML property - for eg. "x" attribute of <rect> we did that. > > Now why didn't I spot this logic error when you pasted the code? > 346 bool attributeIsCSSProperty = isTargetAttributeCSSProperty(targetElement, attributeName); > 347 // Stop animation, if attributeType is set to CSS by the user, but the attribute itself is not a CSS property. > 348 if (!attributeIsCSSProperty && attributeType() == AttributeTypeCSS) > 349 return; > > > I somehow had in mind that isTargetAttributeCSSProperty always returned false for attributeType="XML". That's my wrong assumption, which lead to the whole confusion. SCNR... > > > I don't know how often I should repeat that. It seems that you did not understand how animation of presentation attributes worked. > I did understand that, but not together with attributeType="XML". For that we now get it wrong with my patch, agreed. > > I'll switch presentation attributes animations back to CSS, no matter what attributeType says. > > 410 case AttributeTypeCSS: > 411 // If attributeName is a known animatable SVG CSS property, apply the animation. > 412 if (isTargetAttributeCSSProperty(targetElement, attributeName)) > 413 return ApplyCSSAnimation; > 414 // If attributeName is unknown and ttributeType is not 'auto', don't apply the animation. > 415 if (attributeType == AttributeTypeCSS) > 416 return DontApplyAnimation; > 417 // For attributeType="auto", try XML animation now. > 418 case AttributeTypeXML: > 419 return ApplyXMLAnimation; > > It's just a matter of moving the case AttributeTypeXML below the AttributeTypeCSS case. > > > 1) I do not question the biggest change, the new style for SMIL animations. That is great! But it needs testing. As far as I can see there is no test that inline style gets not changed during SMIL animations. Which must be tested, because of the behavior change. Correct me if I am wrong. > That is tested in my patch, correct me if I'm wrong. I'm checking both getComputedStyle(rect) and rect.style.fill, to verify only the former gets affected, not the latter. Just checked it again. You are right, computed style vs. style.property get checked. > > > 2) Brake out the changes for ApplyXMLAnimation, or at least use your SMIL animation style independent of the attributeType value for presentation attributes. Mutating the DOM attribute is not desired, against the specification and not done by any other SVG viewer. > > I'd like you to understand that I had a misconception about animating presentation attributes with attributeType="XML" set. I assumed that would go through the setAttribute() code path already, so that's why I said I'm only preserving "existing behaviour". That's of course wrong. We only used the setAttribute() code path for NON presentation attributes, and attributeType="XML" set. > > I was aware before that _any_ DOM mutation is actually wrong. But as I was under the impression we mutated DOM for presentation attributes, I tried to keep this behaviour in the patch. Great! So the follow up patch will be quite small. Btw. Have you seen the problems in comment 31? Is that caused by your patch?

(In reply to comment #47) > Great! So the follow up patch will be quite small. Yes uploading it now, with extended tests. > Btw. Have you seen the problems in comment 31? Is that caused by your patch? Will check that as well today.

Created attachment 130025 [details] Follow-up patch v1

Created attachment 130026 [details] Follow-up patch v2 Follow-up patch v2 also fixes the regression Adam found.

Comment on attachment 130026 [details] Follow-up patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=130026&action=review > Source/WebCore/svg/SVGAnimationElement.cpp:362 > + if (targetElement->isStyled()) > + static_cast<SVGStyledElement*>(targetElement)->setInstanceUpdatesBlocked(true); > + In the past we had the early return after this line: 343 if (targetElement->isStyled()) 344 static_cast<SVGStyledElement*>(targetElement)->setInstanceUpdatesBlocked(true); 345 346 bool attributeIsCSSProperty = isTargetAttributeCSSProperty(targetElement, attributeName); 347 // Stop animation, if attributeType is set to CSS by the user, but the attribute itself is not a CSS property. 348 if (!attributeIsCSSProperty && attributeType() == AttributeTypeCSS) 349 return; Does this matter?

(In reply to comment #51) > In the past we had the early return after this line > Does this matter? Well we kept blocking the instance updates if an animation couldn't be applied. I could construct test cases where it mattered - the old behavior is just wrong, we should only ever block instance updates if we call setAttribute, not if we return early. Note: the follow-up patch introduces another regression, I'll have to re-update it.

Created attachment 130027 [details] Follow-up patch v3 Final patch, no regressions.

(In reply to comment #52) > (In reply to comment #51) > > In the past we had the early return after this line > > Does this matter? > > Well we kept blocking the instance updates if an animation couldn't be applied. I could construct test cases where it mattered - the old behavior is just wrong, we should only ever block instance updates if we call setAttribute, not if we return early. > > Note: the follow-up patch introduces another regression, I'll have to re-update it. Can you add the test that shows the old failure with instance update to your new patch?

*** Bug 80053 has been marked as a duplicate of this bug. ***

(In reply to comment #54) > Can you add the test that shows the old failure with instance update to your new patch? It's animate-elem-31/32-t.svg - pixel test fails with patch v2, works again with v3. Should be sufficient, no?

Comment on attachment 130027 [details] Follow-up patch v3 LGTM. r=me.

(In reply to comment #57) > (From update of attachment 130027 [details]) > LGTM. r=me. Thanks! Next time I'll hurry less, to make sure we get it right from the beginning, and we're all on the same track!

Committed r109679: <http://trac.webkit.org/changeset/109679>