825254 - SMIL animateMotion fails to stay on path when the path itself is SMIL-animated

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[david.dailey]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20100101 Firefox/17.0
Build ID: 20121128204232

Steps to reproduce:

http://granite.sru.edu/~ddailey/svg/animoval8b.svg
or
http://cs.sru.edu/~ddailey/svg/animpath3.svg 


Actual results:

The moving object does not stay on the path as the path moves.


Expected results:

It should stay on the path. It does so in Opera, Chrome and Safari.

Comment 1

[Robert Longson [:longsonr]]

Is this a duplicate of bug 665392

Comment 2

[Daniel Holbert [:dholbert]]

Nope. An un-bitrotted version of that bug's patch doesn't seem to have any effect on this bug's testcases (but still fixes its own testcase).

I think the bug here is that we're working with the <path>'s base value, rather than its animated value, or something like that.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Here's a reduced testcase.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: reference testcase (w/ JS tweaks)

Comment 5

[Daniel Holbert [:dholbert]]

OK -- so we cache the flattened path in SVGMotionSMILAnimationFunction::mPath, and we track whether we need to update our cached flattened path with "mIsPathStale".

When the <path> element changes due to scripted modifications (as in "reference testcase"), we end up toggling mIsPathStale because the <mpath> element is a mutation observer on the <path> element, and it receives a call to AttributeChanged.

However, when the path is modified by SMIL animation (as in comment 0's URLs and my attached "testcase 1"), we don't get a mutation notification for some reason.

Comment 6

[Daniel Holbert [:dholbert]]

Yeah, looks like the path's frame is the only thing that receives a notification "d" is animated.

SVGAnimatedPathSegList::SetAnimValue() calls "aElement->DidAnimatePathSegList()"...
https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGAnimatedPathSegList.cpp#123
...which just calls AttributeChanged on our frame, if we have a frame:
https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGElement.cpp#1964

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: WIP

This makes DidAnimatePathSegList() fire AttributeChanged notifications to all listeners, not just to the frame.

That's probably overkill (and may have undesirable effects), so this isn't a finished patch, but it fixes all of the attached testcases (though it seems to cause some occasional weird flickering of the paths in the first URL from comment 0, for "animoval8b.svg").

Comment 8

[Robert Longson [:longsonr]]

I think a better approach is that mpath shouldn't use a mutation observer, it should use an nsSVGRenderingObserver instead.

Comment 9

[Daniel Holbert [:dholbert]]

Would that work even if the <path> is "display:none", though?

Comment 10

[Robert Longson [:longsonr]]

No, we don't do any animation with display:none as it all goes though frames. I've been of the opinion for a long time that the SVG specification is totally broken with respect to display:none.

Comment 11

[Robert Longson [:longsonr]]

Of course the problem with nsSVGIDRenderingObserver is that animation elements don't have frames at the moment so there's nothing to hook the mpath end to.

Comment 12

[david.dailey]

Another instance of this :
http://srufaculty.sru.edu/david.dailey/svg/bezierovals.svg

Comment 13

[david.dailey]

Am hoping to develop several new things using this based on http://srufaculty.sru.edu/david.dailey/cs337/follow.svg for upcoming presentations, so hope ya'll can work on it. cheers
D

Comment 14

[david.dailey]

It is interesting to note that animating the stroke-dasharray works fine in Firefox when the path is animated -- seems like the underlying logic in the browser might be somehow related since pathLength varies dynamically
http://cs.sru.edu/~ddailey/tangles/AnimTangleStroke.svg
and http://cs.sru.edu/~ddailey/tangles/tangleAnimStrokeAndPath.svg (latter may be more understandable, but former is simpler).

Comment 15

[Robert Longson [:longsonr]]

Attachment: Bug 825254 - SMIL animateMotion should react to the path itself being animated r=emilio

Comment 16

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/jobs?repo=try&revision=65cef303c267ff6eefc71290f25108f6504f2553&selectedTaskRun=Rhu-A2EbQq2ir2KakOYMsQ.0

Comment 17

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/jobs?repo=try&revision=7ed089da73b4f141ec7ecd7d3506e08152e6a4a8

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77043dead023
SMIL animateMotion should react to the path itself being animated r=emilio

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/41015 for changes under testing/web-platform/tests

Comment 20

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/77043dead023

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot