Bug 67601

Created attachment 106357 [details] Patch

Comment on attachment 106357 [details] Patch Looks reasonable, r=me. Is Shane CC'ed, btw? I'd like to let him know that this has changed again.

Comment on attachment 106357 [details] Patch Clearing flags on attachment: 106357 Committed r94558: <http://trac.webkit.org/changeset/94558>

All reviewed patches have been landed. Closing bug.

This is one of those funny situations where neither the specified behaviour nor the current behaviour of Opera, FF, Batik, etc. makes sense - there are solid examples of unexpected things happening in each case. With this in mind I elected to follow the specification rather than current practice of other browsers (I also did this because it appeared that the code was inadvertently doing the wrong thing due to the fact that WebKit's matrix multiply operator was using operands in the wrong order). I also emailed www-svg with a query about what was appropriate: http://lists.w3.org/Archives/Public/www-svg/2010Dec/0033.html. This email generated a pretty long discussion, which as far as I can tell is ongoing - this is a genuinely difficult situation to resolve. I don't oppose returning WebKit to the behaviour exemplified by the current patch, although I would prefer we await final resolution from the SVG WG. If you're interested in specific examples of what breaks each way, let me know and I'll spend some time putting something together for this bug.

Not sure if I should be reopening this or not, feel free to close if this was inappropriate.

(In reply to comment #5) > This is one of those funny situations where neither the specified behaviour nor the current behaviour of Opera, FF, Batik, etc. makes sense - there are solid examples of unexpected things happening in each case. > > With this in mind I elected to follow the specification rather than current practice of other browsers (I also did this because it appeared that the code was inadvertently doing the wrong thing due to the fact that WebKit's matrix multiply operator was using operands in the wrong order). I also emailed www-svg with a query about what was appropriate: http://lists.w3.org/Archives/Public/www-svg/2010Dec/0033.html. This email generated a pretty long discussion, which as far as I can tell is ongoing - this is a genuinely difficult situation to resolve. The discussion is still open. But their is no progress on it right now. But their seems to be at least one consensus: The specified behavior shouldn't be used. Other SVG viewers already followed the old behavior of WebKit. In a short testing it seems FireFox follows WebKit as well. The same for Abbra. Given the fact that Firefox is the SVG viewer with the most users on the desktop market, it makes sense to follow them here. We would get a lot of negative feedback if we do something different than Opera or Firefox for the simple test cases like the two tests on the official test suite -- even if we follow the specification. And I hope that we can get back to the discussion on www-svg with this change. Once all major SVG viewers reach a consensus, we can switch to any other behavior. And that is more important to follow the spec. All implementers are members of the SVG WG. Shouldn't be hard to change the spec at this point :) > > I don't oppose returning WebKit to the behaviour exemplified by the current patch, although I would prefer we await final resolution from the SVG WG. Like I said, I hope that we get progress on this topic again now. > > If you're interested in specific examples of what breaks each way, let me know and I'll spend some time putting something together for this bug. Oh of course. I am writing a mail to www-svg at the moment. I'll describe our current behavior in detail. If you see more differences across viewers, just post them on www-svg!

It's not really my fight, but my sense was that neither the specified behavior nor the tested behavior was optimal. It's a while since I've looked at this, but here goes: If I do: <g transform="translate(300, 30)"> <rect width="40" height="40" fill="green"></rect> <animateMotion dur="1s" repeatCount="1" rotate="auto" path="M 100,250 C 100,50 400,50 400,250"></animateMotion> </g> Then what firefox does is premultiply the supplemental transform from animateMotion. At the beginning of the path, this equates to a translate of (100, 250) and a rotation of approximately -90, so we get: translate(100, 250) rotate(-90) translate(300, 30) = translate(-50, 270) rotate(-90) Which is weird. On the other hand, if rotate="auto" is taken away, we get: translate(100, 250) translate(300, 30) = translate(400, 280) So the essential weirdness of premultiplication is that the container transform is used in the wrong place, which results in strange outcomes. I felt at the time that wildly different locations depending on whether rotate="auto" was set or not was worse than the alternatives. On the other hand, if you postmultiply the supplemental transform as currently specified, then something like: <g> <rect width="40" height="40" fill="green"></rect> <animateMotion dur="1s" repeatCount="1" path="M 100,250 C 100,50 400,50 400,250"></animateMotion> <animateTransform attributeName="transform" type="rotate" from="-30" to="0" begin="0s" dur="1s" fill="freeze"/> </g> behaves oddly because the animateTransform rotation affects the animateMotion translation even though it's specified after it. Neither the specified behavior nor the current favorite behavior (which is apparently an artifact of Adobe's that has spread to the other implementations) really does the right thing. Now what I'd like to see is for animateMotion to work off the CTM in the same way that animateTransform does, which would mean that there wouldn't be a supplemental transform to pre- or post- multiply, and this problem would go away. Apparently this is hard for other reasons but I've never seen a clear explanation of why.

(In reply to comment #8) > It's not really my fight, but my sense was that neither the specified behavior nor the tested behavior was optimal. It's a while since I've looked at this, but here goes: > > If I do: > > <g transform="translate(300, 30)"> > <rect width="40" height="40" fill="green"></rect> > <animateMotion dur="1s" repeatCount="1" rotate="auto" path="M > 100,250 C 100,50 400,50 400,250"></animateMotion> > </g> > > Then what firefox does is premultiply the supplemental transform from animateMotion. At the beginning of the path, this equates to a translate of (100, 250) and a rotation of approximately -90, so we get: > > translate(100, 250) rotate(-90) translate(300, 30) > = > translate(-50, 270) rotate(-90) Yes that is right: animate motion * animate rotation * transform. Even if I agree the premultiplication of animate motion is not ideally, Firefox and WebKit trunk animations look identical now. So we have at least three viewers (with Abbas) that perform identically. > > Which is weird. On the other hand, if rotate="auto" is taken away, we get: > > translate(100, 250) translate(300, 30) > = > translate(400, 280) Thats correct as well. > > So the essential weirdness of premultiplication is that the container transform is used in the wrong place, which results in strange outcomes. I felt at the time that wildly different locations depending on whether rotate="auto" was set or not was worse than the alternatives. > > On the other hand, if you postmultiply the supplemental transform as currently specified, then something like: > > <g> > <rect width="40" height="40" fill="green"></rect> > <animateMotion dur="1s" repeatCount="1" path="M > 100,250 C 100,50 400,50 400,250"></animateMotion> > <animateTransform attributeName="transform" type="rotate" from="-30" to="0" begin="0s" dur="1s" fill="freeze"/> > </g> > > behaves oddly because the animateTransform rotation affects the animateMotion translation even though it's specified after it. Neither the specified behavior nor the current favorite behavior (which is apparently an artifact of Adobe's that has spread to the other implementations) really does the right thing. > > Now what I'd like to see is for animateMotion to work off the CTM in the same way that animateTransform does, which would mean that there wouldn't be a supplemental transform to pre- or post- multiply, and this problem would go away. Apparently this is hard for other reasons but I've never seen a clear explanation of why. I totally agree! But right now a common solution is more important than different solutions across all viewers (where just one viewer has the current specified behavior). This would just lead into resigned web developers and they don't use even one of the solutions. We can just try to get this problem on the agenda of one of the next SVG telcons.

Sounds good :) BTW I think Opera's behavior is different to everyone else's - from memory they actually implement animateMotion in the CTM (i.e. our hypothetical desired solution), but I could be wrong.

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

(In reply to comment #9) > > Yes that is right: animate motion * animate rotation * transform. Even if I agree the premultiplication of animate motion is not ideally, Firefox and WebKit trunk animations look identical now. So we have at least three viewers (with Abbas) that perform identically. Not since firefox changed to match you last week https://bugzilla.mozilla.org/show_bug.cgi?id=665392 I expect we'll move back to the status quo again though. > I totally agree! But right now a common solution is more important than different solutions across all viewers (where just one viewer has the current specified behavior). This would just lead into resigned web developers and they don't use even one of the solutions. We can just try to get this problem on the agenda of one of the next SVG telcons. Indeed.

(In reply to comment #12) > (In reply to comment #9) > > > > Yes that is right: animate motion * animate rotation * transform. Even if I agree the premultiplication of animate motion is not ideally, Firefox and WebKit trunk animations look identical now. So we have at least three viewers (with Abbas) that perform identically. > > Not since firefox changed to match you last week https://bugzilla.mozilla.org/show_bug.cgi?id=665392 I expect we'll move back to the status quo again though. Oh, bad timing :). I checked the behavior of a Firefox nightly right before switching back to our previous behavior. As I noted on the www-svg mailing list, I do not prefer to premultiply animate motion + rotation. Our intermediate solution (the specified solution) is much easier to understand for web developers IMHO (see comment #8). The solution that I prefer most is following the sandwich model (like we do for every other animated type) and not to define a special order for multiplication. > > > I totally agree! But right now a common solution is more important than different solutions across all viewers (where just one viewer has the current specified behavior). This would just lead into resigned web developers and they don't use even one of the solutions. We can just try to get this problem on the agenda of one of the next SVG telcons. > > Indeed. I fear that the SVG WG is not willing to change the specification text on SVG animations and replace it by CSS animations completely instead.

(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #9) > > > > > > Yes that is right: animate motion * animate rotation * transform. Even if I agree the premultiplication of animate motion is not ideally, Firefox and WebKit trunk animations look identical now. So we have at least three viewers (with Abbas) that perform identically. > > > > Not since firefox changed to match you last week https://bugzilla.mozilla.org/show_bug.cgi?id=665392 I expect we'll move back to the status quo again though. > > Oh, bad timing :). I checked the behavior of a Firefox nightly right before switching back to our previous behavior. As I noted on the www-svg mailing list, I do not prefer to premultiply animate motion + rotation. Our intermediate solution (the specified solution) is much easier to understand for web developers IMHO (see comment #8). > The patch in https://bugzilla.mozilla.org/show_bug.cgi?id=665392 has just come back out of the firefox codebase. It only landed a few days ago. I still think firefox needs to change though so I don't think that's the end of it on our end.