829085 - SVG: 'mouseover' event doesn't fire when expected if SVG attribute vector-effect="non-scaling-stroke" is used

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[jmack]

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.

Comment 1

[Robert Longson [:longsonr]]

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.

Comment 2

[Daniel Holbert [:dholbert]]

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

Comment 3

[jmack]

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

Comment 4

[Daniel Holbert [:dholbert]]

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.

Comment 5

[Robert Longson [:longsonr]]

Attachment: patch

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.

Comment 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[Daniel Holbert [:dholbert]]

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

Comment 8

[Robert Longson [:longsonr]]

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

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 1 (hovering the blue, especially upper-left corner, should turn it lime)

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.

Comment 10

[Daniel Holbert [:dholbert]]

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 11

[Daniel Holbert [:dholbert]]

Comment on attachment 731508 [details] [diff] [review]
patch

So: r- for now, since the patch seems to be incomplete or incorrect.

Comment 12

[Robert Longson [:longsonr]]

Attachment: patch

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 13

[Daniel Holbert [:dholbert]]

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)

Comment 14

[Daniel Holbert [:dholbert]]

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

Comment 15

[Daniel Holbert [:dholbert]]

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?

Comment 16

[Daniel Holbert [:dholbert]]

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 17

[Robert Longson [:longsonr]]

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.

Comment 18

[Robert Longson [:longsonr]]

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

Comment 19

[Daniel Holbert [:dholbert]]

(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" }

Comment 20

[Daniel Holbert [:dholbert]]

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

Comment 21

[Robert Longson [:longsonr]]

Attachment: testcase 2

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

Comment 22

[Daniel Holbert [:dholbert]]

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 23

[Daniel Holbert [:dholbert]]

Comment on attachment 766309 [details] [diff] [review]
patch

r=me w/ comment 22 addressed

Comment 24

[Robert Longson [:longsonr]]

Attachment: WIP

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.

Comment 25

[Daniel Holbert [:dholbert]]

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

Comment 26

[Daniel Holbert [:dholbert]]

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.

Comment 27

[Daniel Holbert [:dholbert]]

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

Comment 28

[Daniel Holbert [:dholbert]]

Attachment: mochitest patch to layer on top of wip

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.

Comment 29

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=8ca150b4ea29

Comment 30

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16f123ef89fa

Thanks for the help Daniel.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/16f123ef89fa