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

Status: REOPENED

(Core :: SVG, defect)

Description

[Gábor Stefanik]

Attachment: Testcase. Green square doesn't follow the black curve.

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.

Comment 1

[Gábor Stefanik]

Bump! Still a bug in Firefox 6.0.1

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2: no red should show

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

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v1

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 5

[Daniel Holbert [:dholbert]]

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.

Comment 6

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3bf76213a6c

Comment 7

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/c3bf76213a6c

Comment 8

[Robert Longson [:longsonr]]

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

Comment 9

[Daniel Holbert [:dholbert]]

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

Comment 10

[Bill Gianopoulos [:WG9s]]

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.

Comment 11

[Robert Longson [:longsonr]]

See http://lists.w3.org/Archives/Public/www-svg/2011Jun/0004.html and other w3c posts for more information.

Comment 12

[Gábor Stefanik]

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

Comment 13

[Daniel Holbert [:dholbert]]

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