If we have animateMotion that refers to an external path (mpath), and that paths is not rendered, and the group containing the path and animation target is transformed, then the target rarely renders. By which I mean it flickers really badly and is invisible 99% of the time. Steps to reproduce: 1. Load the attached test case 2. Watch the animation Expected results: The triangle shapes moves in an arc (with a bit of backwards and forwards). Actual results: You very occasionally see the triangle flicker once or twice but mostly it doesn't appear. Curiously, if you make the animation path rendered (even just setting stroke to something other than none and opacity to 0.01) it displays fine. From what I can tell this only affects Windows. Also, disabling direct2d doesn't seem to make a difference, I think.

Just running this test case with a trunk build and the arrow doesn't appear to move but if you alt+tab back and forth to force a repaint the arrow will move and sometimes flicker.

I see same behavior as comment 2 on 64-bit linux. Looks like a (lack-of) invalidation bug. It's possible this'll be fixed by DLBI -- it'd be worth trying this in a trunk build from a few days ago, before DLBI was backed out.

(In reply to Daniel Holbert [:dholbert] from comment #3) > I see same behavior as comment 2 on 64-bit linux. Interestingly, it seemed to work (prior to reducing the test case at least) on Mac.

I get the buggy behaviour on Mac with my June 27 nightly.

(In reply to Daniel Holbert [:dholbert] from comment #3) > It's possible this'll be fixed by DLBI -- it'd be worth trying this in a > trunk build from a few days ago, before DLBI was backed out. Just tested with a build from July 3 which was built before DLBI was backed out and I'm still seeing similar problems.

(In reply to Cameron McCormack (:heycam) from comment #5) > I get the buggy behaviour on Mac with my June 27 nightly. That's good to know. It may be that the non-reduced test case featured something else that triggered invalidation on Mac (it had a big bitmap background and all sorts of other elements sharing the same space).

If I enable paint flashing and load the buggy testcase, I see mostly-offscreen painted rects appearing at the top-left and top-right of the window. They seem to correspond to the position of the arrow, but upwards by a few hundred pixels. So -- it looks like we might be invalidating but we're not applying the correct transform when doing so, or something like that.

I forgot to mention that this is a regression. The regression range is somewhere between Firefox 12 and current Aurora (i.e. 15).

I tested with attachment 639218 [details]. Triangle flickers/disappears . Regression window(cached m-c) Good: http://hg.mozilla.org/mozilla-central/rev/f2b2b99108a2 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120517030029 Bad: http://hg.mozilla.org/mozilla-central/rev/895e12563245 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120517110214 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f2b2b99108a2&tochange=895e12563245 Regression window(cached m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/37f2536e975e Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120516204627 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/2c3647738e81 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120516230826 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=37f2536e975e&tochange=2c3647738e81 In local build: Last Good:d3b11e443f04 First Bad: 05a339620439 Triggered by: 05a339620439 Jonathan Watt — Bug 734082 - Compute and store bounds and visual overflow bounds for both SVG leaf and container frames. r=roc.

(In reply to Alice0775 White from comment #10) > I tested with attachment 639218 [details]. > Triangle flickers/disappears . > ... > In local build: > Last Good:d3b11e443f04 > First Bad: 05a339620439 > > Triggered by: > 05a339620439 Jonathan Watt — Bug 734082 - Compute and store bounds and > visual overflow bounds for both SVG leaf and container frames. r=roc. Alice0775 White, thank you so much. That's a huge help.

Comment on attachment 639846 [details] [diff] [review] patch Review of attachment 639846 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks Jonathan. The main issue is I discovered that none of the tests in the motion directory are being run. i.e. reftests/svg/smil/reftest.list doesn't include them. Would you mind adding "include motion/reftest.list" to reftests/svg/smil/reftest.list ? The good news is that pretty much none of those tests pass without this patch applied (i.e. we would have caught this regression a lot sooner if we were running those tests). The bad news is I am still seeing failures for animateMotion-rotate-2.svg even with this patch applied. One pixel is rgb(1,255,0) instead of (0,255,0)--could be a display driver thing since I get a few test failures like this in other places. r=me assuming the animateMotion-rotate-2.svg is not related and passes on try ::: layout/reftests/svg/smil/motion/animateMotion-simple-ref.svg @@ +1,3 @@ > +<svg xmlns="http://www.w3.org/2000/svg"> > + <rect width="100" height="100" fill="blue"/> > +</svg> I'd rather we used standard reference files where possible. I guess the lime.svg which fills the viewport isn't helpful in this case? Going forward I think a 100x100 green rect is probably the most generally-useful reference image (since filling the viewport sometimes makes it hard to test positioning). For now, perhaps we can just rename this to green-rect.svg? (and make it green :) ) ::: layout/reftests/svg/smil/motion/animateMotion-simple.svg @@ +1,1 @@ > +<!-- All the tests in this directory have a number at the end (I guess so variations on a theme are easy to identify). We should rename this to animateMotion-simple-1.svg. ::: layout/reftests/svg/smil/motion/reftest.list @@ +1,4 @@ > # Tests related to SVG Animation (using SMIL), focusing on the animateMotion > # element. > > +== animateMotion-simple.svg animateMotion-simple-ref.svg This should maintain alphabetical order. ::: layout/svg/base/src/nsSVGContainerFrame.cpp @@ +164,5 @@ > nsSVGElement *content = static_cast<nsSVGElement*>(mContent); > const SVGAnimatedTransformList *list = content->GetAnimatedTransformList(); > + const gfxMatrix* animateMotionTransform = > + content->GetAnimateMotionTransform(); > + if ((list && !list->GetAnimValue().IsEmpty()) || animateMotionTransform) { Nit: Can we put the animateMotionTransform test first since it's less expensive. i.e. if (animateMotionTransform || (list && !list->GetAnimValue().IsEmpty())) { ::: layout/svg/base/src/nsSVGPathGeometryFrame.cpp @@ +94,5 @@ > nsSVGElement *content = static_cast<nsSVGElement*>(mContent); > const SVGAnimatedTransformList *list = content->GetAnimatedTransformList(); > + const gfxMatrix* animateMotionTransform = > + content->GetAnimateMotionTransform(); > + if ((list && !list->GetAnimValue().IsEmpty()) || animateMotionTransform) { Likewise here.

(In reply to Brian Birtles (:birtles) from comment #13) > The main issue is I discovered that none of the tests in the motion > directory are being run. i.e. reftests/svg/smil/reftest.list doesn't include > them. Doh! How did we miss that? I'll actually just remove the new test that I added, given that - if we actually run the existing tests - most of them fail prior to this patch. I only added the new test because it seemed like they were passing and therefore not catching this bug. > The bad news is I am still seeing failures for animateMotion-rotate-2.svg Actually, that's due to a bug in the test. I'll fix that as a "part 2" patch. > Would you mind adding "include motion/reftest.list" to > reftests/svg/smil/reftest.list ? Sure, I'll do that as a "part 3" patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d651a39e115b https://hg.mozilla.org/integration/mozilla-inbound/rev/bcda78cc9bed https://hg.mozilla.org/integration/mozilla-inbound/rev/60fc6a0b66dc

(In reply to Brian Birtles (:birtles) from comment #13) > Nit: Can we put the animateMotionTransform test first since it's less > expensive. > i.e. if (animateMotionTransform || (list && > !list->GetAnimValue().IsEmpty())) { I didn't do this since it's a lot rarer to have an animateMotion than a simple transform. What I did instead is move the GetAnimateMotionTransform virtual call into the if so we only evaluate it when necessary. That seemed like the better course of action if perf is a concern.

(In reply to Jonathan Watt [:jwatt] from comment #14) > (In reply to Brian Birtles (:birtles) from comment #13) > > The main issue is I discovered that none of the tests in the motion > > directory are being run. i.e. reftests/svg/smil/reftest.list doesn't include > > them. > > Doh! How did we miss that? Yikes! Looks like that reftest.list tweak was missing from my original animateMotion tests checkin, somehow: http://hg.mozilla.org/mozilla-central/rev/2d5ccb713cd2 Not sure what happened there. Thanks for catching & fixing that!

(In reply to Jonathan Watt [:jwatt] from comment #16) > I didn't do this since it's a lot rarer to have an animateMotion than a > simple transform. What I did instead is move the GetAnimateMotionTransform > virtual call into the if so we only evaluate it when necessary. That seemed > like the better course of action if perf is a concern. Ok, sounds good. Thanks Jonathan!

https://hg.mozilla.org/mozilla-central/rev/d651a39e115b https://hg.mozilla.org/mozilla-central/rev/bcda78cc9bed https://hg.mozilla.org/mozilla-central/rev/60fc6a0b66dc

(In reply to Daniel Holbert [:dholbert] from comment #17) > Looks like that reftest.list tweak was missing from my original > animateMotion tests checkin It would be nice if our tools would warn us about that. :)

Comment on attachment 639846 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 734082 User impact if declined: Animation along a path (one of the more interesting types of animation) is broken. Testing completed (on m-c, etc.): Baked on m-c. Risk to taking this patch (and alternatives if risky): Very low - it just fixes the area that is repainted. String or UUID changes made by this patch: none

Comment on attachment 639846 [details] [diff] [review] patch [Triage Comment] Low risk FF15 regression fix, approved for Aurora.

Pushed https://hg.mozilla.org/releases/mozilla-aurora/rev/9aa8643ad4ba

Verified fixed loading the test cases on FF 15b4 on Win 7/64, Ubuntu 12.04 and Mac OS X 10.6.