Closed Bug 676001 Opened 8 years ago Closed 4 years ago

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

Categories

(Core :: SVG, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: tomasz, Assigned: twointofive)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(8 files, 2 obsolete files)

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.
Please can you use the add an attachment link above to add your testcase rather than pasting it inline in the bug.
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.
Summary: Svg line not clickable → Hit detection (for click & hover) is broken for scaled-up very-thin lines.
Version: 5 Branch → Trunk
Confirming.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Linux → All
Summary: Hit detection (for click & hover) is broken for scaled-up very-thin lines. → Hit detection (for click & hover) is broken for some lines
Let me notice that with firefox 3.6.19 (newest from 3 series) it works pretty well.
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.
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
Duplicate of this bug: 719385
Seems like in nsSVGPathGeometryFrame::GetFrameForPoint, some of the time the gfxContext::PointInStroke call fails to return true when it should.
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.
Summary: Hit detection (for click & hover) is broken for some lines → Stroke hit-testing is buggy
Duplicate of this bug: 729935
Duplicate of this bug: 766041
Blocks: 542605
Keywords: regression
Duplicate of this bug: 766876
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.
It could be helped by the fix on bug 613193
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*.
Thank you. I will look into this work-around and respond back with success / failure.
I tried adding the patch in bug 743578 to my build. Unfortunately it had no effect on the testcases in this bug.
I can also confirm that this patch did not help the test cases.
Any progress on this one?  The problem is still present in FF 22.0, 2 years after the initial report.  :-(
Duplicate of this bug: 901958
Duplicate of this bug: 911764
Duplicate of this bug: 926826
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.
Depends on: 922942
(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.
Duplicate of this bug: 932718
Another test case show it's not working. 
http://jsfiddle.net/qe4kX/6/
(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.
Depends on: 703159
(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.
Duplicate of this bug: 1016533
Still present in Firefox 32 on OSX
My patch for bug 988808 will have fixed this for v33, but I want to leave this open for further testing.
Assignee: nobody → jwatt
Looks good to me in 33.0a1 (2014-07-15).
Nope, still present for me in 33.0b7.
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.)
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
Attached file 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?
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)`.
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.
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Assignee: jwatt → twointofive
Attachment #8669105 - Flags: review?(jmuizelaar)
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.
Attached patch Patch v2 WIP (obsolete) — Splinter Review
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.
Attachment #8669105 - Attachment is obsolete: true
Attachment #8669105 - Flags: review?(jmuizelaar)
Attached patch Patch v3Splinter Review
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).
Attachment #8669263 - Attachment is obsolete: true
Attachment #8669816 - Flags: review?(jmuizelaar)
Summary: Stroke hit-testing is buggy → SVG stroke hit-testing is buggy when cairo is the Moz2D backend
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.
Attachment #8669816 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/4656964dbed4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Duplicate of this bug: 672750
You need to log in before you can comment on or make changes to this bug.