1129147 - Implement path option for hit regions

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Florian Scholz (Open Web Docs)]

One option of the addHitregion method is a Path2D path: 

partial dictionary HitRegionOptions {
  Path2D? path = null;
}
which is specified as 
"A Path2D object that describes the pixels that form part of the region. If this member is not provided or is set to null, the current default path is used instead."

See also https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D.addHitRegion#Options

So, code like this would create a hit region for the given path p: 
ctx = c.getContext('2d'), 
p = new Path2D();
p.arc(75, 75, 50, 0, 2 * Math.PI);
ctx.fill(p);
ctx.addHitRegion({id: 'p', path: p});

This is already in blink:
https://chromium.googlesource.com/chromium/blink/+/master/Source/core/html/canvas/HitRegionOptions.idl

Comment 1

[Benoit Girard (:BenWa)]

AFAIK :roc is the most involved around canvas standardization. I assume it's something we want, if so bounce it to Milan who might be able to schedule someone to implement this.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Should be trivial to implement, and sounds reasonable, though it's not all that important either.

Comment 3

[Benoit Girard (:BenWa)]

I wont have time. Looks like we don't really have a canvas 2D owner. To :milan for scheduling.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Florian, do you have a pointer to the standard for this?  In particular, I'm not clear what happens to the current path in this scenario.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Never mind, I see the link to the standard, but it's the same I found - I guess I'm assuming that if there is a current path, it would just be discarded at this point and replaced with the one in addHitRegion...

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 1. CanvasPath into a separate file, to avoid circular dependency. r=roc

This is just a prep, but was the most work for this issue - without CanvasPath in a separate file, we get a circular dependency between CanvasRenderingContext2D.h and (generated) CanvasRenderingContext2DBinding.h that I couldn't get rid of any other way.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 2. Add path option to addHitRegion. r=gw280

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8571535 [details] [diff] [review]
Part 1. CanvasPath into a separate file, to avoid circular dependency. r=roc

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

It would be slightly better to make dom/canvas/CanvasPath.h an "hg copy" of CanvasRenderingContext2D.h.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 1. Take CanvasPath into a separate file, to avoid circular dependency. Carry r=roc

Same file, but CanvasPath.h is now an hg copy of CanvasRenderingContext2D.h

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 11

[Carsten Book [:Tomcat]]

needs dom peer review for the changes in dom/webidl/CanvasRenderingContext2D.webidl

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8571998 [details] [diff] [review]
Part 1. Take CanvasPath into a separate file, to avoid circular dependency.  Carry r=roc

webidl file modified, need dom peer review.

Comment 13

[(no longer active)]

Comment on attachment 8571536 [details] [diff] [review]
Part 2. Add path option to addHitRegion. r=gw280

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

Please send an intent to implement/ship email to dev-platform.  Thanks!

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #13)
> Comment on attachment 8571536 [details] [diff] [review]
> Part 2. Add path option to addHitRegion. r=gw280
> 
> Review of attachment 8571536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please send an intent to implement/ship email to dev-platform.  Thanks!

It's been about a week on dev-platform, no comments so far.

Comment 15

[Rik Cabanier]

(In reply to Milan Sreckovic [:milan] from comment #14)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #13)
> > Comment on attachment 8571536 [details] [diff] [review]
> > Part 2. Add path option to addHitRegion. r=gw280
> > 
> > Review of attachment 8571536 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please send an intent to implement/ship email to dev-platform.  Thanks!
> 
> It's been about a week on dev-platform, no comments so far.

That means you're good to go! :-)

Comment 16

[(no longer active)]

Indeed!

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e27def3ae3a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f2b97191ec1

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e27def3ae3a3
https://hg.mozilla.org/mozilla-central/rev/6f2b97191ec1

Comment 19

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/39#Interfaces.2FAPIs.2FDOM
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/addHitRegion