676001 - SVG stroke hit-testing is buggy when cairo is the Moz2D backend

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Tomasz Kołodziejski [:tomasz]]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Steps to reproduce:

I use svg in my program. But important part of it is to select lines. But sometimes there happen lines which are almost impossible to click. (I also tested nighlty)

Interestingly in code below, for example, if you change  x2="100" to 10 it always works.

Example code:
<svg xmlns="http://www.w3.org/2000/svg" width="100mm" height="100mm" viewBox="0 10 10 10">
	<script type="text/javascript">
	/*document.onclick = function(e){
		console.log(e.target) || alert(e.target)
	}*/
	onload = function(){
		document.getElementById('line').addEventListener('click', function(){
			alert('clicked')
		}, true);
	}
	</script>
	<style type="text/css">
	line{ stroke: #f00; stroke-width: .5mm }
	</style>
	<line id="line" x1="0" y1="17" x2="100" y2="15"/>
</svg>

I clicked line. At least I see I'm clicking it.


Actual results:

Nothing.


Expected results:

It should've fired click action.

Comment 1

[Robert Longson [:longsonr]]

Please can you use the add an attachment link above to add your testcase rather than pasting it inline in the bug.

Comment 2

[Tomasz Kołodziejski [:tomasz]]

Attachment: Example of line which is difficult to click

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (on hover, line should turn lime)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (thicker - hover doesn't work on top half of line)

Here's a variant of the previous testcase, with a wider 'stroke' value. In this case, hovers over the bottom half of the line seem to have an effect, but not those over the top half of the line.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 4 (thicker still, & no viewbox)

Comment 6

[Daniel Holbert [:dholbert]]

Confirming.

Comment 7

[Tomasz Kołodziejski [:tomasz]]

Let me notice that with firefox 3.6.19 (newest from 3 series) it works pretty well.

Comment 8

[Robert Longson [:longsonr]]

Seems pretty similar to 672750? Anyone care to check whether the regression range is the same (bug 672750 comment 4) as I think this bug could be marked as a duplicate of that one.

Comment 9

[Alice0775 White]

Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/2968d19b0165
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100426 Minefield/3.7a5pre ID:20100426040533
Fails(reproduced):
http://hg.mozilla.org/mozilla-central/rev/f236632a9747
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100426 Minefield/3.7a5pre ID:20100426084628
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2968d19b0165&tochange=f236632a9747

In local build(m-c repo)
build from f236632a9747 : reproduced
build from e3e80935c165 : not reproduced

Triggered by:
f236632a9747	Jeff Muizelaar — Bug 542605. Update cairo to 12d521df8acc483b2daa844d4f05dc2fe2765ba6. r=vlad,jwatt,bas Reland after fixing quartz related clipping bug and a bunch of other ones

Comment 11

[Jonathan Watt [:jwatt]]

Seems like in nsSVGPathGeometryFrame::GetFrameForPoint, some of the time the gfxContext::PointInStroke call fails to return true when it should.

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: testcase with stroke on circle

Also note that it's not just for lines that stroke hit-testing is failing. The right hand side of the circle in this testcase also has the same problem.

Comment 16

[Steve Mahoney]

Bump.  Is there any update on the status of this?  Is there a work-around in the meantime?  Interactive SVGs are near-useless because of this bug.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

It could be helped by the fix on bug 613193

Comment 18

[Steve Mahoney]

I appreciate the comment, but I don't think this is applicable.  The issue here does not cause any JS errors.  I believe it's the hit detection within the SVG engine *speculation*.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

I meant bug 743578

Comment 20

[Steve Mahoney]

Thank you. I will look into this work-around and respond back with success / failure.

Comment 21

[Robert Longson [:longsonr]]

I tried adding the patch in bug 743578 to my build. Unfortunately it had no effect on the testcases in this bug.

Comment 22

[Steve Mahoney]

I can also confirm that this patch did not help the test cases.

Comment 23

[Anton Berezin]

Any progress on this one?  The problem is still present in FF 22.0, 2 years after the initial report.  :-(

Comment 27

[Jonathan Watt [:jwatt]]

This is a cairo bug. The reason that we hit this on all platforms is that we're drawing into a temporary surface, and those are currently cairo backed on all platforms right now. Bug 922942 will change that so that we use Moz2D backed temporary gfxContexts. That should fix this bug on platforms where Moz2D is not backed by cairo, which means everywhere except Windows XP, Linux and systems with blacklisted drivers.

Comment 28

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #27)
> That should fix this bug on platforms where
> Moz2D is not backed by cairo

That is, assuming the non-cairo backends get hit-testing right.

Comment 30

[John]

Another test case show it's not working. 
http://jsfiddle.net/qe4kX/6/

Comment 31

[John]

(In reply to Jonathan Watt [:jwatt] from comment #27)
> This is a cairo bug. The reason that we hit this on all platforms is that
> we're drawing into a temporary surface, and those are currently cairo backed
> on all platforms right now. Bug 922942 will change that so that we use Moz2D
> backed temporary gfxContexts. That should fix this bug on platforms where
> Moz2D is not backed by cairo, which means everywhere except Windows XP,
> Linux and systems with blacklisted drivers.

Do you mean it's already fixed? As I tested on Win7 of FF25, it's not working.

Comment 32

[Jonathan Watt [:jwatt]]

(In reply to John from comment #31)
> Do you mean it's already fixed?

No, I mean than _when_ bug 922942 is fixed this issue will be fixed for most people. But that bug has not been fixed yet.

Comment 35

[Daniel Kozlowski]

Still present in Firefox 32 on OSX

Comment 36

[Jonathan Watt [:jwatt]]

My patch for bug 988808 will have fixed this for v33, but I want to leave this open for further testing.

Comment 37

[John Firebaugh]

Looks good to me in 33.0a1 (2014-07-15).

Comment 38

[pmos]

Nope, still present for me in 33.0b7.

Comment 39

[Daniel Holbert [:dholbert]]

Definitely still broken for me as well, in Nightly 35.0a1 (2014-09-27).
Mozilla/5.0 (X11; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0

I tried testcases 2, 3, and 4; all are still broken. (Large areas of the lines there don't apply the hover effect, when hovered.)

jwatt's testcase 5 is broken for me as well, though it's harder to hit there. (As I move my cursor horizontally across the cross section of the circle's stroke, about midway up on the right side, there's a point where the hover effect is lost. That point is about 1/4 of the way from the inner edge of the stroke. I see similar issues on the slanted line above the circle, too -- certain points lose the hover effect.)

Comment 40

[Alexandre Folle de Menezes]

I hit this problem all the time on OpenStreetMap's iD editor.  There's even a bug open on their side: https://github.com/openstreetmap/iD/issues/2494

Comment 41

[Tomasz Kołodziejski [:tomasz]]

Attachment: test.html

I see the exact same buggy behaviour with canvas and `isPointInStroke` method. Does that say anything more about where the bug can be?

Comment 42

[Tomasz Kołodziejski [:tomasz]]

Attachment: It seems that cairo_in_stroke works (c++).

Adding one more test, this time in c++ and cairo. It seems to work, however. I've found the base code here: http://marc.info/?l=cairo&m=130652935028220.

I'm compiling it with `g++ test.c $(pkg-config --cflags --libs gtk+-2.0)`.

Comment 43

[Tomasz Kołodziejski [:tomasz]]

With the above test in mind it should be worth adding some debug stuff here: https://hg.mozilla.org/mozilla-central/file/50b95032152c/gfx/2d/PathCairo.cpp#l209, I think.

Comment 44

[Tom Klein]

Attachment: Patch v1

Sorry for stealing this one Jonathan - I would have asked but I actually overlooked that you were assigned (I had started working on bug 672750, which is going to turn out to be a dup of this one).  Also I grabbed your mochitest from bug 719385.

I'll add a longer explanation in my next comment shortly, but there are two changes:
1) The first change in cairo-gstate.c:_cairo_gstate_in_stroke changes the limit bounds around the point (x, y) to be hit-tested from
  limit.p1.x = _cairo_fixed_from_double (x) - 5;
  limit.p1.y = _cairo_fixed_from_double (y) - 5;
  limit.p2.x = limit.p1.x + 5;
  limit.p2.y = limit.p1.y + 5;
to "+ 10"s for limit.p2, which I think is clearly what was intended.  That change fixes most of the issues except for lines that have a shallow slope.

2) The second change is to _add_clipped_edge - I'll add a longer explanation below, but basically the current
    if (left_y == right_y) /* horizontal within bounds */
        continue;
check is where things are failing for shallow lines.  For shallow lines like testcase 4 here, when the top or bottom edge of the line is being added to create the polygon representing the line on which hit testing will be checked, the slope of the edge between p1 and p2 is small enough and the limit box to which the edge is being clipped is small enough that in the computation of
  left_y = _cairo_edge_compute_intersection_y_for_x (p1, p2, limits->p1.x);
  right_y = _cairo_edge_compute_intersection_y_for_x (p1, p2, limits->p2.x);
there's a floor operation to make sure the returned _cairo_edge_compute_intersection_y_for_x is an int, and we get the same left_y and right_y y-values on the line, even though the line isn't horizontal and limits->p1.x != limits->p2.x.  And then the check actually erroneously returns - the return is because you're not supposed to add horizontal lines in the representation of a polygon, but in fact in our testcases the condition left_y == right_y does _not_ lead to the adding of a horizontal line, it leads to the addition of a vertical  line (more explanation below), and in any case the check is superfluous because all of the _add_edge calls below are already guarded by bot_y != top_y checks.

The -5, +5 in 1) was introduced the last time cairo was imported - the current cairo still uses the same values they used back then, which are -1, +2.  And in the current cairo, _add_clipped_edge has been completely rewritten, so the check I'm removing no longer exists.  So I'm not submitting anything to cairo.

Comment 45

[Tom Klein]

I don't know if this will help or not, but for the record, here's some more explanation on what's going on in change 2) from comment 44.  I have testcase 4 in mind here.

When cairo tries to find out if a point (x, y) is inside a stroked line, it first builds a small limit box around (x, y) (that's what's happening in change 1) from comment 44), and then it builds a polygonal representation of the line clipped to that limit box, and then it sees if the point is in the polygonal (well, the polygon becomes trapezoids first) clipped representation of the line.

So to build the limit-box-clipped polygon for the line, it projects each of the four sides of the line onto the limit box.  But because(?) it never adds horizontal lines to representations of polygons (https://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-polygon.c?rev=a98c00e70590#390), when it projects an edge of the line to the limit box it projects horizontally.  (So any edge of the line completely above or below the limit box automatically fails to get added - fortunately only two edges actually need to get added to the polygon for the hit test to work.)

So the test that was failing was the case where, for example, the point to be tested, (x,y), had its y with p1.y < y < p2.y, where p1 and p2 are (say) the bottom corners of the line.  In that case, cairo tries to project the line p1<->p2 onto the small limit box around (x,y) in the function _add_clipped_edge, where in this case we fall down to the else case inside the for loop.  Then left_y is computed to be the y-value on the line p1<->p2 corresponding to the x-value limit.p1.x (the left edge of the limit box) and right_y is computed similarly for limit.p2.x (the right edge of the limit box).  But as described above, since the line has a shallow slope, and limit.p1.x is close to limit.p2.x, and there's a floor operation involved, it turns out that left_y == right_y, and so cairo erroneously returns.  But in the particular case of testcase 4 (and more or less for similar shallow lines), for any given (x,y) there are only two edges of the line that even have a chance of projecting horizontally onto the limit box (the other two edges miss the limit box when they project horizontally), and now one of them has just failed, so only one edge ever gets added to the polygon representing the line and the hit test fails.  Also note that in that case, the line p1<->p2 actually projects onto the limit box as a vertical line (which it must, right?), so the if check was the wrong check anyway.

Comment 46

[Tom Klein]

Attachment: Patch v2 WIP

Canceling the review on v1, I was too quick.  Sorry about that Jeff if you already started looking at it.

I'm still confident that the bug fails the way I described, but simply dropping the "if left_y == right_y" check, while (seemingly) fixing the bug, can lead to adding an edge to the polygon that lies outside of limits (so it fixes the bug, but in a sometimes unclean, possibly buggy under other circumstances way).

I think v2 corrects that, but I want to take some more time now to check this more thoroughly, so I'll re-request a review when that's done.  Of course if there are any comments in the meantime feel free to let me know.

Comment 47

[Tom Klein]

Attachment: Patch v3

The new
+	    if (p1->x < p2->x) {
check in _add_clipped_edge assumes that p1->y < p2->y - that's explicitly checked for in _cairo_polygon_add_edge, and though it's not explicitly checked in _cairo_polygon_add_line, the if checks preceding the call to _add_clipped_edge imply that that's the case (also the subsequent checks being done in the original version of _add_clipped_edge following that if check don't make sense if p1->y > p2->y).

Comment 48

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8669816 [details] [diff] [review]
Patch v3

Review of attachment 8669816 [details] [diff] [review]:
-----------------------------------------------------------------

I'm very sorry this review took so long.

Comment 49

[Tom Klein]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a634215c9c94

Comment 50

[Tom Klein]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4656964dbed41a499bb9d59e09aea36d7eeafbad
Bug 676001 - Fix for stroke hit testing on cairo. r=jrmuizel

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4656964dbed4