User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 Build ID: 20130104151925 Steps to reproduce: See example: http://jsfiddle.net/jmackpacketstorm/2mPUP/ Created a mouseover event listener for 2 SVG lines, a one with SVG attribute: vector-effect="non-scaling-stroke", and one without. Actual results: See example: http://jsfiddle.net/jmackpacketstorm/2mPUP/ A Mouse cursor scrolled over either line should turn the line red. However, on Firefox v18, the mouseover event doesn't work as expected on the blue line, with attribute vector-effect="non-scaling-stroke". Instead, the mouseover event is fired when the mouse cursor is offset down and to the right of the blue line. The black line doesn't have attribute vector-effect="non-scaling-stroke" and continues to work as expected. This also fails on polylines with attribute vector-effect="non-scaling-stroke". Expected results: See example: http://jsfiddle.net/jmackpacketstorm/2mPUP/ The mouseover event should be fired for any line when the mouse cursor is actually over the line.

Did this used to work in some previous version of Firefox? If so then finding a regression range would help a lot if you're willing to do that. See https://quality.mozilla.org/docs/bugzilla/guide-to-triaging-bugs-for-firefox/finding-a-regression-window/ type about:buildconfig in the address bar in the last good and subsequent first bad build and post the Built from link please.

http://mozilla.github.com/mozregression/ is a handy tool that automates the regression-finding process (and prints out a link at the end, with the last-good and first-bad changesets built in).

Using http://mozilla.github.com/mozregression/ I isolated the regression to: Last good nightly: 2012-05-18 First bad nightly: 2012-05-19 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e794cef56df6&tochange=642d1a36702f

That range includes: > de36a918add7 Robert Longson — Bug 528332 - Implement non-scaling-stroke vector-effect. Part 2 - SVG changes r=dholbert > 173ec720780e Robert Longson — Bug 528332 - Implement non-scaling-stroke vector-effect. Part 1 - style system changes r=dbaron So it looks like this has been broken since we added support for non-scaling-stroke.

I tried to write a mochitest for this based around getBoundingClientRect but I just can't get it to fail. Must be something specific to hit testing itself that's causing the problem.

Sorry for the delayed response here -- I skimmed over this in my inbox and then forgot about it until I recently got a bugzilla nag email. [I need to sanity-check my review queue more frequently] So: I don't immediately understand why this is the right fix... In particular, if this is a bug in GetStrokeTransform, why doesn't this affect painting? We use GetStrokeTransform in nsSVGUtils::SetupCairoStrokeGeometry, which appears to be where we prepare to draw, so I'd expect this bug (and/or this patch) to affect behavior there, too.

(In reply to Robert Longson from comment #5) > I tried to write a mochitest for this based around getBoundingClientRect but > I just can't get it to fail. Must be something specific to hit testing > itself that's causing the problem. That's a bit too hand-wavy for me to feel comfortable about this -- I'd like to understand the bug (& why, or whether, it only affects hit-testing) a little more to be sure we're actually fixing the right thing, in the right spot. (If we need to, I think we *can* test hit-testing by synthesizing mouse events, too, though that takes a bit more work to code up.)

Seems to be just hit testing. I don't understand why either.

Here's a testcase using (:hover to change color) where the attached patch doesn't affect our behavior, from my testing. (From inspection in GDB, transform.x0 and transform.y0 are _already_ 0 when I hover the area and it fails to change color, in an un-patched build. So, setting them to 0 in a patch build has no effect.

So: I suspect the attached patch only incidentally fixed the original testcase, and the actual bug (or additional components of the bug) lie elsewhere.

Comment on attachment 731508 [details] [diff] [review] patch So: r- for now, since the patch seems to be incomplete or incorrect.

The real issue is that we need to compare the point and the region with the same transformations applied. This passes the testcase in comment 0 and Daniel's attachment.

Comment on attachment 766309 [details] [diff] [review] patch > if (!isHit && (hitTestFlags & SVG_HIT_TEST_STROKE)) { > nsSVGUtils::SetupCairoStrokeHitGeometry(this, tmpCtx); >+ userSpacePoint = >+ nsSVGUtils::GetStrokeTransform(this).Invert().Transform(userSpacePoint); A comment would probably be helpful here. I don't 100% grok why we have to transform the point yet, but I'm going to do a quick gdb step-through to convince myself that it's correct. >diff --git a/layout/svg/nsSVGUseFrame.cpp b/layout/svg/nsSVGUseFrame.h >copy from layout/svg/nsSVGUseFrame.cpp >copy to layout/svg/nsSVGUseFrame.h This seems like something you probably didn't mean to include in the patch. Also: we should include an automated testcase for this. (which probably needs to involve synthesizing mouse events)

OK, I think I understand why we need the GetStrokeTransform() call -- correct me if I'm right. It looks like what we do is: (a) create a reference surface, with gfxContext 'tmpCtx' (b) convert aPoint into coordinates on that reference surface ('userSpacePoint') (c) generate our path on the reference surface (d) test if userSpacePoint is inside that path (to see if it hits the fill) (e) Modify tmpCtx as if we were about to draw the stroke (generating the path, etc) **IMPORTANT: this makes a persistent change to the transforms applied to tmpCtx** (f) We query tmpCtx to see if userSpacePoint is inside our stroke. ...and the problem is that 'userSpacePoint' is still in "native" reference surface coordinates, but tmpCtx has now been scaled, and when we call PointInStroke, it treats the queried point accordingly. Given that, it makes sense to me that we need to transform 'userSpacePoint' before passing it to PointInStroke().

So, the part that I wasn't getting in comment 13 was that tmpCtx interprets points *differently* (with a different matrix applied) before vs. after the SetupCairoStrokeHitGeometry call, and so we have to transform our point in order to keep it in the same coordinate space as our changed-out-from-under-us context. I don't like that; it's non-obvious. Rather than transforming 'userSpacePoint' to accomodate the changing tmpCtx, I'd rather that we do the opposite -- transform tmpCtx back, so that it ends up back in the original coordinate space. That makes this easier to follow, IMHO. We can do this (according to my local testing, at least) by adding aa gfxContextMatrixAutoSaveRestore that goes into scope *just* for the SetupCairoStrokeHitGeometry call, like this: // Use a gfxContextAutoSaveRestore to undo any transforms that // SetupCairoStrokeHitGeometry leaves applied to tmpCtx: { gfxContextMatrixAutoSaveRestore ctxMatrixRestore(tmpCtx); nsSVGUtils::SetupCairoStrokeHitGeometry(this, tmpCtx); } isHit = tmpCtx->PointInStroke(userSpacePoint); (Note the new layer of curly-braces, for scoping purposes. How does that sound?

Actually, I wonder if SetupCairoStrokeHitGeometry should have a top-level gfxContextMatrixAutoSaveRestore inside of it, to clean this up itself... How does that sound? Reasoning: Intuitively, if I have a point on a surface and a gfxContext for the surface, I'd expect that "SetupCairoStrokeHitGeometry" should leave the context in a state where I can hit-test my point. I wouldn't expect that I need to do something weird to the point, also (as your patch does), or do something additional to the gfxContext (as my suggestion in comment 15 does). Note also that the documentation for SetupCairoStrokeHitGeometry doesn't suggest that it changes the context such that points need to be transformed by GetStrokeTransform() in order to be usefully hit-tested. (It probably should, if we're going to keep its current behavior.)

Comment 14 is right, but after that I think you may be going wrong. I don't think your ideas will work with rotations (only scaling and translation). See bug 463934 comment 1 (and onwards). I'll see if I can produce a rotation testcase that works my way and doesn't work yours.

Any tips on synthesizing mouse events for a testcase would be appreciated.

(In reply to Robert Longson from comment #18) > Any tips on synthesizing mouse events for a testcase would be appreciated. It looks like you can use synthesizeMouseAtPoint() from EventUtils.js in a mochitest: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js?rev=3e2047c6e723#229 with aEvent being { type: "mousemove", clickCount: "0" }

(That, combined with a mouseover event listener (like from the original testcase here) that sets a success condition, should probably be workable for a mochitest.)

Daniel, your solution fails on this testcase, mine passes. Where do you want to go from here?

Gah, you're right. Thanks for making that testcase to demonstrate. OK -- given that, I'm all right with the GetStrokeTransform() solution -- please add a comment to explain why it's there, though (basically, mentioning that tmpCtx's matrix has been transformed by SetupCairoStrokeGeometry*, and that we need to transform userSpacePoint to keep it valid). (Remember to revert the nsSVGUseFrame thing, too.) * (SetupCairoStrokeHitGeometry has already been renamed to SetupCairoStrokeGeometry on inbound, so your comment should use the new name, and you might want to wait until it's merged (or just import the cset locally) when making your patch.)

Comment on attachment 766309 [details] [diff] [review] patch r=me w/ comment 22 addressed

This displays a lime rect with the patch and a red rect without it which might seem fine except that the test passes in both cases i.e. the mouseover handler always goes off.

nit: if I run the test directly, with ./mach mochitest-plain content/svg/content/test/test_non-scaling-stroke.html and then hover over the rect, then I get mochitest errors about "SimpleTest.finish()] this test already called finish!" Please add a bool "isFinished" or something that guards the finish() call, to prevent multiple calls to finish().

nit #2: if "mouseOver" is never triggered, the test never calls finish() -- so the failure condition is currently "the test times out", which isn't ideal. (In reply to Robert Longson from comment #24) > This displays a lime rect with the patch and a red rect without it Actually: when I load the mochitest, without the patch, I actually see a lime rect for a bit (sometimes just a brief flash), and then a red rect. There are a few wonky things that are contributing to cause this: - From my testing, it looks like the sendMouseEvent call will move the mouse from its original position (which might be 0,0 or something) to the new position, and if that crosses over the (incorrectly-positioned) hit region along the way, then we'll fire the listener. - You can address this by calling sendMouseEvent() *before* you set up the listener, too, to initialize the mouse position, and then calling it again afterwards. But it appears to be async, so you need a sprinkling of SimpleTest.executeSoon() to give it a chance to be run. I've got what I think is a working version of the mochitest, locally. I'll clean it up a little and post it in a minute.

I *thought* I had a working version of the mochitest, but it looks like it was a bit flaky. I think "synthesizeMouseExpectEvent" is exactly what we want, though, if we can get it to behave. http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js?rev=2f54529528a9#616

This layers on top of the WIP, to make the testcase fail reliably without the patch and pass reliably with the patch. The numbers are a little magic - I picked them through a bit of trial and error. I cribbed a bit of the additional boilerplate from http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/test_sanityEventUtils.html Robert: if this passes/fails reliably for you as it does for me, feel free to take it and tweak it as you like & include it as part of your final patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/16f123ef89fa Thanks for the help Daniel.