Open Bug 665392 Opened 8 years ago Updated 6 years ago

transform="translate" + animateMotion with curved path + rotate="auto" = disaster

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

REOPENED

People

(Reporter: netrolller.3d, Unassigned)

References

Details

(Keywords: testcase)

Attachments

(3 files)

When transform="translate(x, y)" is applied to an element that is animated using animateMotion along a curved path (e.g. a Bézier curve), with rotate="auto" set, the animation flickers severely and does not follow the prescribed path.
It seems that the transformation is wrongly taken into account when rotating the animated element (the element is first translated, then the translated element is rotated around the canvas's origin; instead of rotating first, then translating the rotated element).

Reproduction steps: Open the attached testcase.

Expected results: Green square moves along the black Bézier curve.

Actual results: Green square jumps off the curve, and does some acrobatics in mid-air, before following what appears to be a translated copy of the black curve.
Bump! Still a bug in Firefox 6.0.1
I can reproduce this on today's nightly:
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110913 Firefox/9.0a1

Chromium and Opera both get it right.

Firefox gets un-wonky if I make any of the following tweaks:
 - move the |translate| attribute up to a <g> ancestor
...or...
 - remove the rotate attribute (or change it to (rotate="0")

As noted in comment 0, we must be applying the rotation and the transform attribute in the wrong order, or something like that.
OS: Windows 7 → All
Hardware: x86 → All
Here's a testcase with a straight path and a fixed rotate-angle.

We've got identical animations on near-identical rects -- they only differ in that the red rect's transform is set via its <g> parent instead of on the <rect>.
Attached patch fix v1Splinter Review
I think I just got the matrix multiplication backwards here (back in bug 436418 which added animateMotion support).

Attached patch (with test) fixes this.  Will request review soon; I want to do one sanity-check in GDB first.
Comment on attachment 560176 [details] [diff] [review]
fix v1

Yup, sanity confirmed.  (I wanted to double-check that the matrix we're prepending to already includes the parent's transform (and it does, by way of nsSVGPathGeometryFrame::GetCanvasTM)).

Requesting review.
Attachment #560176 - Flags: review?(roc)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3bf76213a6c
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: in-testsuite+
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/c3bf76213a6c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Daniel, I think you're going to need to back this out. This change makes us fail http://www.w3.org/Graphics/SVG/Test/20061213/htmlEmbedHarness/full-animate-elem-24-t.html which we passed before.

Webkit are changing to what we used to do before your patch (https://bugs.webkit.org/show_bug.cgi?id=67601)

The whole situation is a mess I'm afraid: http://lists.w3.org/Archives/Public/www-svg/2010Dec/0033.html
D'oh!  Note to self: remember to check in a version of that w3c testcase...

I'm just about to leave on a weeklong camping trip, so I can't investigate thoroughly at the moment, but I've backed out as you suggest:
  https://hg.mozilla.org/mozilla-central/rev/06445f55f009
Status: RESOLVED → REOPENED
Flags: in-testsuite+ → in-testsuite?
Resolution: FIXED → ---
Target Milestone: mozilla9 → ---
This seems to work better in Opera.  Both test cases attached to the bug pass and the W3C test noted in comment 8 passes as well.
Is anyone still working on this bug? I've filed this almost 2 years ago as part of a series, but it appears to have been abandoned after an initial failed fix...
I'm not actively working on it, no.  --> Unassigning, to reflect reality.

And actually, as Robert hinted at in comment 8, Webkit intentionally switched to (roughly) match the Gecko behavior on this (to pass an test from the official testsuite).  So: since Opera is switching to (webkit-based) Blink, then very soon all SMIL-supporting browsers will be consistent on this.

I don't have the time at the moment to dig into the spec or the thread from comment 8 / comment 12 to be absolutely sure whether our current behavior on this is per spec or not, but I think there's a good chance it is...
Assignee: dholbert → nobody
You need to log in before you can comment on or make changes to this bug.