Closed Bug 1129147 Opened 9 years ago Closed 9 years ago

Implement path option for hit regions

Categories

(Core :: Graphics: Canvas2D, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: fs, Assigned: milan)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [parity-blink][gfx-noted])

Attachments

(2 files, 1 obsolete file)

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
Keywords: dev-doc-needed
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.
Flags: needinfo?(roc)
Whiteboard: [parity-blink] → [parity-blink][gfx-noted]
Should be trivial to implement, and sounds reasonable, though it's not all that important either.
Flags: needinfo?(roc)
I wont have time. Looks like we don't really have a canvas 2D owner. To :milan for scheduling.
Flags: needinfo?(milan)
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.
Flags: needinfo?(milan) → needinfo?(fscholz)
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...
Flags: needinfo?(fscholz)
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.
Attachment #8571535 - Flags: review?(roc)
Attachment #8571535 - Attachment description: CanvasPath into a separate file, to avoid circular dependency. r=roc → Part 1. CanvasPath into a separate file, to avoid circular dependency. r=roc
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.
Attachment #8571535 - Flags: review?(roc) → review+
Same file, but CanvasPath.h is now an hg copy of CanvasRenderingContext2D.h
Attachment #8571535 - Attachment is obsolete: true
Attachment #8571998 - Flags: review+
Attachment #8571536 - Flags: review?(gwright) → review+
needs dom peer review for the changes in dom/webidl/CanvasRenderingContext2D.webidl
Flags: needinfo?(milan)
Keywords: checkin-needed
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.
Flags: needinfo?(milan)
Attachment #8571998 - Flags: review?(ehsan)
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!
Attachment #8571536 - Flags: review+
Attachment #8571998 - Flags: review?(ehsan)
(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.
Flags: needinfo?(ehsan)
(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! :-)
Indeed!
Flags: needinfo?(ehsan)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e27def3ae3a3
https://hg.mozilla.org/mozilla-central/rev/6f2b97191ec1
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: