Tobias Schneider [:tobytailor] Assignee
	Description • 12 years ago

In addition to isPointInPath, the Canvas API should also provide isPointInStroke for hit testing. Right now there is no efficient way to test if a point lies inside the filled AND stroked region of a path. (except of checking for nontransparend pixels using getImageData, which is awful slow if 2D rendering is hw accelerated).

	Jeff Muizelaar [:jrmuizel]
	Comment 5 • 12 years ago

Comment on attachment 673128 [details] [diff] [review]
Patch v5

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

::: content/canvas/test/test_mozIsPointInStroke.html
@@ +33,5 @@
> +
> +ok(ctx.mozIsPointInStroke(20, 20) === false);
> +ok(ctx.mozIsPointInStroke(80, 20) === false);
> +ok(ctx.mozIsPointInStroke(80, 30) === false);
> +ok(ctx.mozIsPointInStroke(20, 30) === false);

This test should include curves and transformed paths/transformed userspace. It might also be worth testing different cap modes.

	Jeff Muizelaar [:jrmuizel]
	Comment 7 • 12 years ago

Comment on attachment 675015 [details] [diff] [review]
Patch v6

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

::: content/canvas/test/test_mozIsPointInStroke.html
@@ +191,5 @@
> +ok(ctx.mozIsPointInStroke(14, 15) === true, 'ctx.mozIsPointInStroke(14, 15) === true');
> +ok(ctx.mozIsPointInStroke(81, 15) === true, 'ctx.mozIsPointInStroke(81, 15) === true');
> +ok(ctx.mozIsPointInStroke(79, 41) === true, 'ctx.mozIsPointInStroke(79, 41) === true');
> +ok(ctx.mozIsPointInStroke(73, 21) === true, 'ctx.mozIsPointInStroke(73, 21) === true');
> +

How about a final test that does a curve scaled by 3x?

Also you should test changing the scale during path construction.

::: gfx/2d/PathSkia.cpp
@@ +184,5 @@
> +                    int32_t(SkFloatToScalar(transformed.y + 1)));
> +
> +  SkRegion pathRegion;
> +
> +  return pathRegion.setPath(strokePath, pointRect);

This should use SkPathContainsPoint if possible.

	Tobias Schneider [:tobytailor] Assignee
	Comment 8 • 12 years ago

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> 
> How about a final test that does a curve scaled by 3x?
>
> Also you should test changing the scale during path construction.

Sounds reasonable, will add those tests.

> > +  return pathRegion.setPath(strokePath, pointRect);
> 
> This should use SkPathContainsPoint if possible.

Don't think SkPathContainsPoint is a standard Skia API, is it? Seems to be an internal helper defined and used in Chromium (http://code.google.com/p/chromium/source/search?q=SkPathContainsPoint&origq=SkPathContainsPoint&btnG=Search+Trunk) and itself uses setPath().

	Jeff Muizelaar [:jrmuizel]
	Comment 9 • 12 years ago

(In reply to Tobias Schneider from comment #8)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
>
> Don't think SkPathContainsPoint is a standard Skia API, is it? Seems to be
> an internal helper defined and used in Chromium
> (http://code.google.com/p/chromium/source/
> search?q=SkPathContainsPoint&origq=SkPathContainsPoint&btnG=Search+Trunk)
> and itself uses setPath().

True. I thought they had fixed this in Skia.

	Jeff Muizelaar [:jrmuizel]
	Comment 10 • 12 years ago

(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> (In reply to Tobias Schneider from comment #8)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> >
> > Don't think SkPathContainsPoint is a standard Skia API, is it? Seems to be
> > an internal helper defined and used in Chromium
> > (http://code.google.com/p/chromium/source/
> > search?q=SkPathContainsPoint&origq=SkPathContainsPoint&btnG=Search+Trunk)
> > and itself uses setPath().
> 
> True. I thought they had fixed this in Skia.

Actually, the function is SkPath::contains(x,y)

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 12 years ago

Pushed to Try since I don't see any results posted here.
https://tbpl.mozilla.org/?tree=Try&rev=079dc1ebac54

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 12 years ago

Unfortunately, this causes hangs in android armv6 builds.
https://tbpl.mozilla.org/?tree=Try&rev=7f236ae2ee94

	Jeff Muizelaar [:jrmuizel]
	Comment 14 • 12 years ago

I wonder if it still hangs on armv6 without the test changes. I find it pretty unlikely that this would cause all the mochitests to fail...

	Tobias Schneider [:tobytailor] Assignee
	Comment 15 • 12 years ago

Didn't happen in https://tbpl.mozilla.org/?tree=Try&rev=8cab7ad6c0bf.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/69c7ee7683e0

	Ed Morley [:emorley]
	Comment 17 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/69c7ee7683e0

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 18 • 12 years ago

Was this ever even suggested on a standards list?

	Jeff Muizelaar [:jrmuizel]
	Comment 19 • 12 years ago

(In reply to :Ms2ger from comment #18)
> Was this ever even suggested on a standards list?

I don't believe it was. Would you be willing to do so?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 20 • 12 years ago

I have no idea what this API is good for; it's probably better to let someone who does suggest it.

	Jeff Muizelaar [:jrmuizel]
	Comment 21 • 12 years ago

http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2012-November/037911.html

	Hixie (not reading bugmail)
	Comment 22 • 12 years ago

There's no need for this to be prefixed, by the way. It'll either be added, in which case I'll ensure it's compatible and so it won't conflict, or it won't, in which case nothing with this name will be added and it still won't conflict.

The idea seems pretty sound, the main thing that would stop me from adding it to the spec right now is lack of interest from other vendors. I'll ask around, but if you can convince another vendor to indicate an interest that would certainly help.

	Henri Sivonen (:hsivonen)
	Comment 23 • 12 years ago

Can this be unprefixed, please? As Hixie says, the prefix is useless and unprefixing later will cause breakage later.

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/eed5cbd0b622

	Ed Morley [:emorley]
	Comment 26 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/eed5cbd0b622

	Masatoshi Kimura [:emk]
	Comment 27 • 12 years ago

Comment on attachment 684393 [details] [diff] [review]
Patch to remove prefix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: proliferating more -moz prefixes
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): maybe low, simple renaming
String or UUID changes made by this patch: none

	bhavana bajaj [:bajaj]
	Comment 28 • 12 years ago

Comment on attachment 684393 [details] [diff] [review]
Patch to remove prefix

Approving based on the m-c testing.

	Masatoshi Kimura [:emk]
	Comment 29 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/a55bc8cb8a8e

	Florian Scholz (Open Web Docs)
	Comment 30 • 11 years ago

Documented:
https://developer.mozilla.org/en-US/docs/DOM/CanvasRenderingContext2D#isPointInStroke%28%29
https://developer.mozilla.org/en-US/docs/Firefox_19_for_developers#DOM