Closed Bug 966591 Opened 7 years ago Closed 7 years ago

Add basic support for Hit regions in Canvas

Categories

(Core :: Canvas: 2D, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: cabanier, Assigned: cabanier)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(9 files, 41 obsolete files)

5.11 KB, patch
Details | Diff | Splinter Review
4.84 KB, patch
Details | Diff | Splinter Review
3.14 KB, patch
Details | Diff | Splinter Review
985 bytes, patch
Details | Diff | Splinter Review
2.72 KB, patch
Details | Diff | Splinter Review
4.39 KB, patch
Details | Diff | Splinter Review
5.18 KB, patch
Details | Diff | Splinter Review
4.53 KB, patch
Details | Diff | Splinter Review
3.88 KB, patch
Details | Diff | Splinter Review
Assignee: nobody → cabanier
Attachment #8369018 - Flags: review?(surkov.alexander)
Comment on attachment 8369019 [details] [diff] [review]
1. put very basic hit region interface in + add runtime flag

I'm not a layout peer so can't review it. So asking Robert for review.
Attachment #8369019 - Flags: review?(roc)
Attachment #8369018 - Flags: review?(surkov.alexander)
(In reply to Rik Cabanier from comment #1)
> Created attachment 8369018 [details] [diff] [review]
> 1. put very basic hit region interface in + add runtime flag

is it actually ready for review?
(In reply to alexander :surkov from comment #4)
> (In reply to Rik Cabanier from comment #1)
> > Created attachment 8369018 [details] [diff] [review]
> > 1. put very basic hit region interface in + add runtime flag
> 
> is it actually ready for review?

Well, it's a small patch that basically just provides the interface + a runtime flag.
It doesn't do anything :-) Am I not supposed to break it into smaller pieces?
(In reply to Rik Cabanier from comment #5)
> Well, it's a small patch that basically just provides the interface + a
> runtime flag.
> It doesn't do anything :-) Am I not supposed to break it into smaller pieces?

it's up to you and your reviewer I think
ok. Let me know if you prefer 1 big patch or a bunch of little ones
Comment on attachment 8369019 [details] [diff] [review]
1. put very basic hit region interface in + add runtime flag

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

This patch is OK but we shouldn't land it by itself. So please attach more patches that actually do something, preferably something we could ship.

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2355,5 @@
>  
>    return new TextMetrics(width);
>  }
>  
> +void CanvasRenderingContext2D::AddHitRegion(const HitRegionOptions& )

space before )
Attachment #8369019 - Flags: review?(roc) → review+
Blocks: html5test
Attachment #8369700 - Flags: review?(roc)
Attachment #8369743 - Attachment is obsolete: true
Attachment #8369743 - Flags: review?(roc)
Attachment #8369753 - Flags: review?(roc)
Attachment #8369757 - Flags: review?(roc)
Comment on attachment 8369700 [details] [diff] [review]
2. Validate input to AddHitRegion. Provide partial implementation

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2355,5 @@
>  
>    return new TextMetrics(width);
>  }
>  
> +void CanvasRenderingContext2D::AddHitRegion(const HitRegionOptions& options, ErrorResult& error)

nit: keep void on new line

@@ +2358,5 @@
>  
> +void CanvasRenderingContext2D::AddHitRegion(const HitRegionOptions& options, ErrorResult& error)
> +{
> +  // if an ID was passed in, remove old hit region first
> +  if(options.mId.Length() != 0) {

nit: space after if

see https://developer.mozilla.org/en/docs/Developer_Guide/Coding_Style
Comment on attachment 8369700 [details] [diff] [review]
2. Validate input to AddHitRegion. Provide partial implementation

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2358,5 @@
>  
> +void CanvasRenderingContext2D::AddHitRegion(const HitRegionOptions& options, ErrorResult& error)
> +{
> +  // if an ID was passed in, remove old hit region first
> +  if(options.mId.Length() != 0) {

Actually I don't think we need this condition at all. Just call RemoveHitRegion unconditionally.

@@ +2372,5 @@
> +  // check if the control is a descendant of our canvas
> +  HTMLCanvasElement* canvas = GetCanvas();
> +  if (!canvas || !nsContentUtils::ContentIsDescendantOf(options.mControl, canvas)) {
> +    error.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +    return;

Hmm. Does the spec say what should happen if the control element is a descendant of the canvas when addHitRegion is called, but it is moved somewhere else in the document later?

@@ +2383,5 @@
> +}
> +
> +void CanvasRenderingContext2D::RemoveHitRegion(const nsAString& id)
> +{
> +  if(id.Length() == 0) {

space before (

Actually I don't think we need this condition at all.

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +687,5 @@
>  
>    /**
> +    * State information for hit regions
> +    */
> +  std::map<nsString, nsRefPtr<mozilla::dom::Element> > mHitRegionsOptions;

Use nsTHashtable please
Attachment #8369700 - Flags: review?(roc) → review-
Comment on attachment 8369753 [details] [diff] [review]
3. Compute bounds of current path and set them as the accessible region

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2360,5 @@
> +static void
> +ReleaseBBoxPropertyValue(void*    aObject,       /* unused */
> +                            nsIAtom* aPropertyName, /* unused */
> +                            void*    aPropertyValue,
> +                            void*    aData          /* unused */)

Fix indent

@@ +2389,5 @@
>    }
>  
> +  // check if the path is valid
> +  EnsureUserSpacePath(CanvasWindingRule::Nonzero);
> +  if(!mPath) {

space before (
Attachment #8369753 - Flags: review?(roc) → review+
Comment on attachment 8369757 [details] [diff] [review]
4. complete implementation of removeHitRegion

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

This will need to be fixed for nsTHashtable

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2409,2 @@
>  {
> +  if(aId.Length() == 0) {

Space before (
Attachment #8369757 - Flags: review?(roc) → review-
Comment on attachment 8369700 [details] [diff] [review]
2. Validate input to AddHitRegion. Provide partial implementation

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2372,5 @@
> +  // check if the control is a descendant of our canvas
> +  HTMLCanvasElement* canvas = GetCanvas();
> +  if (!canvas || !nsContentUtils::ContentIsDescendantOf(options.mControl, canvas)) {
> +    error.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +    return;

It does not.
I will take this to the mailing list. Logically, the hit region should be removed if this happens.

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +687,5 @@
>  
>    /**
> +    * State information for hit regions
> +    */
> +  std::map<nsString, nsRefPtr<mozilla::dom::Element> > mHitRegionsOptions;

That class isn't as convenient since I now have to create another structure.
I see that stl classes in other places so why is a std::map not ok?
Attachment #8369700 - Attachment is obsolete: true
Attachment #8369859 - Flags: review?(roc)
Attachment #8369859 - Flags: review?(roc)
Attachment #8369859 - Attachment is obsolete: true
Attachment #8369860 - Attachment is obsolete: true
Attachment #8369753 - Attachment is obsolete: true
Attachment #8369757 - Attachment is obsolete: true
Attachment #8369869 - Flags: review?(roc)
Attachment #8369862 - Flags: review?(roc)
Attached patch 5. Pass hit bounds to a11y code (obsolete) — Splinter Review
(In reply to Rik Cabanier from comment #17)
> ::: content/canvas/src/CanvasRenderingContext2D.h
> @@ +687,5 @@
> >  
> >    /**
> > +    * State information for hit regions
> > +    */
> > +  std::map<nsString, nsRefPtr<mozilla::dom::Element> > mHitRegionsOptions;
> 
> That class isn't as convenient since I now have to create another structure.
> I see that stl classes in other places so why is a std::map not ok?

One reason is that it's a balanced binary tree not a hashtable, and a hashtable is more efficient here.

There are some other reasons that you can read about in dev.platform.
Comment on attachment 8369862 [details] [diff] [review]
2. Validate input to AddHitRegion. Provide partial implementation

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

Looks good. I guess we'll need to add something to remove the element when it moves outside the canvas, and I'm not sure how to do that yet. But that'll be a separate patch.

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2383,5 @@
> +
> +void
> +CanvasRenderingContext2D::RemoveHitRegion(const nsAString& id)
> +{
> +  mHitRegionsOptions.RemoveEntry(nsString(id));

you don't need the nsString temporary here

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +185,5 @@
>                    mozilla::ErrorResult& error);
>    TextMetrics*
>      MeasureText(const nsAString& rawText, mozilla::ErrorResult& error);
>  
> +  void AddHitRegion(const HitRegionOptions& options, mozilla::ErrorResult& error);

Why the prefix?

@@ +687,5 @@
>    /**
> +    * State information for hit regions
> +    */
> +
> +  struct RegionInfo: public nsStringHashKey

space before :

@@ +689,5 @@
> +    */
> +
> +  struct RegionInfo: public nsStringHashKey
> +  {
> +    nsRefPtr<mozilla::dom::Element> element;

Why the prefix?

Also, call this mElement and put it at the end of the class.

@@ +698,5 @@
> +    }
> +    RegionInfo(const nsAString *aKey) :
> +      nsStringHashKey(aKey)
> +    {
> +    }

I think you can MOZ_DELETE this

@@ +703,5 @@
> +    RegionInfo(const RegionInfo& aOther) :
> +      nsStringHashKey(&aOther.GetKey())
> +    {
> +      NS_ERROR("Should never be called");
> +    }

Can't you MOZ_DELETE this?
Comment on attachment 8369862 [details] [diff] [review]
2. Validate input to AddHitRegion. Provide partial implementation

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2383,5 @@
> +
> +void
> +CanvasRenderingContext2D::RemoveHitRegion(const nsAString& id)
> +{
> +  mHitRegionsOptions.RemoveEntry(nsString(id));

was failing earlier but not anymore. (Maybe with the STL map?)
Fixed.

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +185,5 @@
>                    mozilla::ErrorResult& error);
>    TextMetrics*
>      MeasureText(const nsAString& rawText, mozilla::ErrorResult& error);
>  
> +  void AddHitRegion(const HitRegionOptions& options, mozilla::ErrorResult& error);

the mozilla prefix is used all over the .h file

@@ +698,5 @@
> +    }
> +    RegionInfo(const nsAString *aKey) :
> +      nsStringHashKey(aKey)
> +    {
> +    }

These are needed and meed to be public. Otherwise, template instantiation fails.

@@ +703,5 @@
> +    RegionInfo(const RegionInfo& aOther) :
> +      nsStringHashKey(&aOther.GetKey())
> +    {
> +      NS_ERROR("Should never be called");
> +    }

copy constructor was needed too.
Deleted this since the default copy constructor is fine.
Attachment #8369862 - Attachment is obsolete: true
Attachment #8369862 - Flags: review?(roc)
Attachment #8370424 - Flags: review?(roc)
Attachment #8369866 - Attachment is obsolete: true
Attachment #8369869 - Attachment is obsolete: true
Attachment #8369869 - Flags: review?(roc)
Attachment #8370427 - Flags: review?(roc)
Attachment #8370384 - Flags: review?(surkov.alexander)
Comment on attachment 8370424 [details] [diff] [review]
2. Validate input to AddHitRegion. Provide partial implementation

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

r+ with that

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +698,5 @@
> +      nsStringHashKey(aKey)
> +    {
> +    }
> +
> +    nsRefPtr<mozilla::dom::Element> mElement;

Really, this is not needed. Take it out.
Attachment #8370424 - Flags: review?(roc) → review+
Comment on attachment 8370427 [details] [diff] [review]
4. complete implementation of removeHitRegion

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2406,5 @@
>  
>  void
>  CanvasRenderingContext2D::RemoveHitRegion(const nsAString& id)
>  {
> +  RegionInfo* info = mHitRegionsOptions.GetEntry(nsString(id));

The nsString constructor should not be needed here.
Attachment #8370427 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> Comment on attachment 8370424 [details] [diff] [review]
> 2. Validate input to AddHitRegion. Provide partial implementation
> 
> Review of attachment 8370424 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with that
> 
> ::: content/canvas/src/CanvasRenderingContext2D.h
> @@ +698,5 @@
> > +      nsStringHashKey(aKey)
> > +    {
> > +    }
> > +
> > +    nsRefPtr<mozilla::dom::Element> mElement;
> 
> Really, this is not needed. Take it out.

Are you talking about the mozilla and dom prefixes or it being an nsRefPtr?
mElement is definitely needed since we need to remember the element that's associated with the ID
Attachment #8370384 - Flags: review?(surkov.alexander)
Comment on attachment 8370384 [details] [diff] [review]
5. Pass hit bounds to a11y code

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

::: accessible/src/generic/Accessible.cpp
@@ +922,5 @@
>    if (frame) {
> +    nsINode* node = GetNode();
> +    gfx::Rect* fallbackRect = NULL;
> +    if (node) {
> +      fallbackRect = static_cast<gfx::Rect*>(node->GetProperty(nsGkAtoms::hitregion));

if hitregion rect is always relative canvas? is it same for nested hitregions? in what units?

@@ +929,1 @@
>      *aBoundingFrame = nsLayoutUtils::GetContainingBlockForClientRect(frame);

do you need this logic for in-canvas rect?

@@ +929,3 @@
>      *aBoundingFrame = nsLayoutUtils::GetContainingBlockForClientRect(frame);
> +
> +    if(fallbackRect) {

I wouldn't name it as fallbackRect, it's rather rectInCanvas or hitRegionRect.

nit: space after if and while

@@ +939,5 @@
> +      if(canvasFrame)
> +        *aBoundingFrame = canvasFrame;
> +
> +      aTotalBounds = nsRect(fallbackRect->x, fallbackRect->y, fallbackRect->width, fallbackRect->height);
> +	    }

nit: wrong indentation
Comment on attachment 8370384 [details] [diff] [review]
5. Pass hit bounds to a11y code

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

::: accessible/src/generic/Accessible.cpp
@@ +922,5 @@
>    if (frame) {
> +    nsINode* node = GetNode();
> +    gfx::Rect* fallbackRect = NULL;
> +    if (node) {
> +      fallbackRect = static_cast<gfx::Rect*>(node->GetProperty(nsGkAtoms::hitregion));

The bounds we pass in in addHitRegion are relative to the canvas. This will be true even for nested ones.

The units need another translation (which is why I cancelled the review but you already did it :-))

@@ +929,1 @@
>      *aBoundingFrame = nsLayoutUtils::GetContainingBlockForClientRect(frame);

Likely not.
I was being cautious in case there is no canvasFrame found below (which should never happen)

@@ +929,3 @@
>      *aBoundingFrame = nsLayoutUtils::GetContainingBlockForClientRect(frame);
> +
> +    if(fallbackRect) {

Will fix.
Attached file 5. Pass hit bounds to a11y code (obsolete) —
Attachment #8370384 - Attachment is obsolete: true
Attachment #8370546 - Flags: review?(surkov.alexander)
Attached patch 5. Pass hit bounds to a11y code (obsolete) — Splinter Review
Attachment #8370546 - Attachment is obsolete: true
Attachment #8370546 - Flags: review?(surkov.alexander)
Attachment #8370547 - Flags: review?(surkov.alexander)
Attachment #8370547 - Attachment is patch: true
Flags: needinfo?(roc)
Comment on attachment 8370547 [details] [diff] [review]
5. Pass hit bounds to a11y code

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

::: accessible/src/generic/Accessible.cpp
@@ +920,5 @@
>  void
>  Accessible::GetBoundsRect(nsRect& aTotalBounds, nsIFrame** aBoundingFrame)
>  {
>    nsIFrame* frame = GetFrame();
>    if (frame) {

for the record: I think it makes sense if no frame means no hit region (until somebody decides to use aria-hidden="false" in canvas content ;) )

@@ +921,5 @@
>  Accessible::GetBoundsRect(nsRect& aTotalBounds, nsIFrame** aBoundingFrame)
>  {
>    nsIFrame* frame = GetFrame();
>    if (frame) {
> +    nsINode* node = GetNode();

the spec says that an element is a control what sounds like a DOM element, so if the document cannot be associated with hit region then it makes sense to use mContent

@@ +927,5 @@
> +    if (node) {
> +      hitRegionRect = static_cast<gfx::Rect*>(node->GetProperty(nsGkAtoms::hitregion));
> +    }
> +
> +    if (hitRegionRect) {

move the statement under previous if

@@ +929,5 @@
> +    }
> +
> +    if (hitRegionRect) {
> +      // This is for canvas fallback content
> +      // Search for the ancestor canvas

nit: perhaps "Find a canvas frame the found hit region is relative to."?

@@ +936,5 @@
> +        canvasFrame = canvasFrame->GetParent();
> +
> +      // make the canvas the bounding frame
> +      if (canvasFrame)
> +        *aBoundingFrame = canvasFrame;

either no check or no rect computation

@@ +942,5 @@
> +      nsPresContext* presContext = mDoc->PresContext();
> +      aTotalBounds = nsRect(presContext->DevPixelsToAppUnits(hitRegionRect->x),
> +        presContext->DevPixelsToAppUnits(hitRegionRect->y),
> +        presContext->DevPixelsToAppUnits(hitRegionRect->width),
> +        presContext->DevPixelsToAppUnits(hitRegionRect->height));

nit: wrong indentation

does it make sense to store hit region rect in app units on the element? Is it really in dev pixels (not css pixles)?

@@ +945,5 @@
> +        presContext->DevPixelsToAppUnits(hitRegionRect->width),
> +        presContext->DevPixelsToAppUnits(hitRegionRect->height));
> +    } else {
> +      *aBoundingFrame = nsLayoutUtils::GetContainingBlockForClientRect(frame);
> +      aTotalBounds = nsLayoutUtils::GetAllInFlowRectsUnion(frame, *aBoundingFrame, nsLayoutUtils::RECTS_ACCOUNT_FOR_TRANSFORMS);

nit: I think would prefer to have 'return' in 'if' block rather than having 'else' block here.
Comment on attachment 8370547 [details] [diff] [review]
5. Pass hit bounds to a11y code

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

::: accessible/src/generic/Accessible.cpp
@@ +927,5 @@
> +    if (node) {
> +      hitRegionRect = static_cast<gfx::Rect*>(node->GetProperty(nsGkAtoms::hitregion));
> +    }
> +
> +    if (hitRegionRect) {

done

@@ +929,5 @@
> +    }
> +
> +    if (hitRegionRect) {
> +      // This is for canvas fallback content
> +      // Search for the ancestor canvas

done

@@ +942,5 @@
> +      nsPresContext* presContext = mDoc->PresContext();
> +      aTotalBounds = nsRect(presContext->DevPixelsToAppUnits(hitRegionRect->x),
> +        presContext->DevPixelsToAppUnits(hitRegionRect->y),
> +        presContext->DevPixelsToAppUnits(hitRegionRect->width),
> +        presContext->DevPixelsToAppUnits(hitRegionRect->height));

I think so.
Otherwise, would you get the correct a11y region if you increase the device pixel ratio?

@@ +945,5 @@
> +        presContext->DevPixelsToAppUnits(hitRegionRect->width),
> +        presContext->DevPixelsToAppUnits(hitRegionRect->height));
> +    } else {
> +      *aBoundingFrame = nsLayoutUtils::GetContainingBlockForClientRect(frame);
> +      aTotalBounds = nsLayoutUtils::GetAllInFlowRectsUnion(frame, *aBoundingFrame, nsLayoutUtils::RECTS_ACCOUNT_FOR_TRANSFORMS);

yes, that's much better.
I restructured the code.
(In reply to Rik Cabanier from comment #38)

> @@ +942,5 @@
> > +      nsPresContext* presContext = mDoc->PresContext();
> > +      aTotalBounds = nsRect(presContext->DevPixelsToAppUnits(hitRegionRect->x),
> > +        presContext->DevPixelsToAppUnits(hitRegionRect->y),
> > +        presContext->DevPixelsToAppUnits(hitRegionRect->width),
> > +        presContext->DevPixelsToAppUnits(hitRegionRect->height));
> 
> I think so.
> Otherwise, would you get the correct a11y region if you increase the device
> pixel ratio?

It's better to ask Robert. But if those are in dev pixels then wouldn't it be nicer to hook into GetBounds() instead (to avoid conversation from dev to app and from app to dev pixels)
Attached patch 5. Pass hit bounds to a11y code (obsolete) — Splinter Review
Attachment #8370547 - Attachment is obsolete: true
Attachment #8370547 - Flags: review?(surkov.alexander)
Attachment #8370933 - Flags: review?(surkov.alexander)
Attachment #8370933 - Attachment is patch: true
(In reply to alexander :surkov from comment #39)
> (In reply to Rik Cabanier from comment #38)
> 
> > @@ +942,5 @@
> > > +      nsPresContext* presContext = mDoc->PresContext();
> > > +      aTotalBounds = nsRect(presContext->DevPixelsToAppUnits(hitRegionRect->x),
> > > +        presContext->DevPixelsToAppUnits(hitRegionRect->y),
> > > +        presContext->DevPixelsToAppUnits(hitRegionRect->width),
> > > +        presContext->DevPixelsToAppUnits(hitRegionRect->height));
> > 
> > I think so.
> > Otherwise, would you get the correct a11y region if you increase the device
> > pixel ratio?
> 
> It's better to ask Robert. But if those are in dev pixels then wouldn't it
> be nicer to hook into GetBounds() instead (to avoid conversation from dev to
> app and from app to dev pixels)

I did some more debugging and it turns out that I had to do the conversion on the canvas end (like I did in https://bugzilla.mozilla.org/show_bug.cgi?id=958241)
Updated the patch with this + addressed your previous comments
Comment on attachment 8370933 [details] [diff] [review]
5. Pass hit bounds to a11y code

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

I'd love to see mochitest

::: accessible/src/generic/Accessible.cpp
@@ +923,5 @@
>    nsIFrame* frame = GetFrame();
>    if (frame) {
> +    nsINode* node = GetNode();
> +    gfx::Rect* hitRegionRect = NULL;
> +    if (node) {

what about mContent thing, is it reasonable to use it here?

@@ +927,5 @@
> +    if (node) {
> +      hitRegionRect = static_cast<gfx::Rect*>(node->GetProperty(nsGkAtoms::hitregion));
> +
> +      if (hitRegionRect) {
> +        // This is for canvas fallback content

nit: maybe it makes sense to move this commetn before hitRegionRect obtaining

@@ +933,5 @@
> +        nsIFrame* canvasFrame = frame->GetParent();
> +        while (canvasFrame && (canvasFrame->GetType() != nsGkAtoms::HTMLCanvasFrame))
> +          canvasFrame = canvasFrame->GetParent();
> +
> +        // make the canvas the bounding frame

nit: seems like dupes the comment above

@@ -925,4 @@
>      *aBoundingFrame = nsLayoutUtils::GetContainingBlockForClientRect(frame);
> -    aTotalBounds = nsLayoutUtils::
> -      GetAllInFlowRectsUnion(frame, *aBoundingFrame,
> -                             nsLayoutUtils::RECTS_ACCOUNT_FOR_TRANSFORMS);

pls revert this change (esp if you break 80 chars per line restriction)
Attachment #8370933 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8370933 [details] [diff] [review]
5. Pass hit bounds to a11y code

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

::: accessible/src/generic/Accessible.cpp
@@ +923,5 @@
>    nsIFrame* frame = GetFrame();
>    if (frame) {
> +    nsINode* node = GetNode();
> +    gfx::Rect* hitRegionRect = NULL;
> +    if (node) {

When I step into GetNode(), I get to:
{
  return mContent;
}
on line 2584. So it seems mContent is used already.

@@ +927,5 @@
> +    if (node) {
> +      hitRegionRect = static_cast<gfx::Rect*>(node->GetProperty(nsGkAtoms::hitregion));
> +
> +      if (hitRegionRect) {
> +        // This is for canvas fallback content

will do.

@@ +933,5 @@
> +        nsIFrame* canvasFrame = frame->GetParent();
> +        while (canvasFrame && (canvasFrame->GetType() != nsGkAtoms::HTMLCanvasFrame))
> +          canvasFrame = canvasFrame->GetParent();
> +
> +        // make the canvas the bounding frame

not really. Here we say that the canvas we found is the bounding frame for the fallback content.

@@ -925,4 @@
>      *aBoundingFrame = nsLayoutUtils::GetContainingBlockForClientRect(frame);
> -    aTotalBounds = nsLayoutUtils::
> -      GetAllInFlowRectsUnion(frame, *aBoundingFrame,
> -                             nsLayoutUtils::RECTS_ACCOUNT_FOR_TRANSFORMS);

will do.
(In reply to Rik Cabanier from comment #43)

> When I step into GetNode(), I get to:
> {
>   return mContent;
> }
> on line 2584. So it seems mContent is used already.

sure but GetNode() is virtual method, in case of DocAccessible it returns mDocumentNode, so if don't need a document for hit regions then you can use mContent directly what will be a bit faster

> > +        // make the canvas the bounding frame
> 
> not really. Here we say that the canvas we found is the bounding frame for
> the fallback content.

up to you
(In reply to alexander :surkov from comment #44)
> (In reply to Rik Cabanier from comment #43)
> 
> > When I step into GetNode(), I get to:
> > {
> >   return mContent;
> > }
> > on line 2584. So it seems mContent is used already.
> 
> sure but GetNode() is virtual method, in case of DocAccessible it returns
> mDocumentNode, so if don't need a document for hit regions then you can use
> mContent directly what will be a bit faster

ah. Ok, I will change the code.
Attached patch 5. Pass hit bounds to a11y code (obsolete) — Splinter Review
Attachment #8370933 - Attachment is obsolete: true
Attachment #8371048 - Attachment is patch: true
Attached patch 5. Pass hit bounds to a11y code (obsolete) — Splinter Review
Attachment #8371048 - Attachment is obsolete: true
Attached patch 5. Pass hit bounds to a11y code (obsolete) — Splinter Review
Attachment #8371052 - Attachment is obsolete: true
Attachment #8371054 - Attachment is patch: true
Attachment #8370427 - Attachment is obsolete: true
Attachment #8371076 - Flags: review?(roc)
Attached patch 5. Pass hit bounds to a11y code (obsolete) — Splinter Review
Sorry. Patch got messed up by moving between pc and mac
Attachment #8371054 - Attachment is obsolete: true
> ::: accessible/src/generic/Accessible.cpp
> @@ +920,5 @@
> >  void
> >  Accessible::GetBoundsRect(nsRect& aTotalBounds, nsIFrame** aBoundingFrame)
> >  {
> >    nsIFrame* frame = GetFrame();
> >    if (frame) {
> 
> for the record: I think it makes sense if no frame means no hit region
> (until somebody decides to use aria-hidden="false" in canvas content ;) )

in accessible hit region maybe, but it seems to me if your using hit regions as well as canvas you've choosen to do your own layout, so I don't see why the browsers layout should matter here.

> does it make sense to store hit region rect in app units on the element? Is
> it really in dev pixels (not css pixles)?

I don't think it makes sense to store anything on nodes here, since we now have an API designed for holding state we can just teach GetBounds() and {,Depest}ChildAt() how to deal with hit regions.  That'll also have the benefit of making hit testing on non rectangular shapes work better.

> > > +        // make the canvas the bounding frame
> > 
> > not really. Here we say that the canvas we found is the bounding frame for
> > the fallback content.
> 
> up to you

Well, if we support hit regions without elements then we'll probably have to fix that api somehow so it doesn't return the outer frame.  Saying the canvas frame is the outer frame for child regions seems wrong, but we can deal with both of those problems later since we don't support those yet.
(In reply to Trevor Saunders (:tbsaunde) from comment #51)
> 
> Well, if we support hit regions without elements then we'll probably have to
> fix that api somehow so it doesn't return the outer frame.  Saying the
> canvas frame is the outer frame for child regions seems wrong, but we can
> deal with both of those problems later since we don't support those yet.

Yes, if we're going to support the full hit region API, we will need to build a bridge between the accessibility APIs and the canvas that has the regions.
I will do that later if I have the time.
Blocks: 935566
Attached patch 6. some bug fixes to match spec (obsolete) — Splinter Review
Attached patch 7. bounds + error handling tests (obsolete) — Splinter Review
Attachment #8374415 - Flags: review?(roc)
Attachment #8374416 - Flags: review?(surkov.alexander)
Attachment #8374416 - Flags: review?(surkov.alexander)
Attached patch 7. bounds + error handling tests (obsolete) — Splinter Review
Attachment #8374416 - Attachment is obsolete: true
Attachment #8374416 - Flags: review?(surkov.alexander)
Attached patch 7. bounds + error handling tests (obsolete) — Splinter Review
Attachment #8374429 - Attachment is obsolete: true
Attachment #8374415 - Flags: review?(roc)
Comment on attachment 8374460 [details] [diff] [review]
7. bounds + error handling tests

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

you'd need to have r+ for content mochitest

::: accessible/tests/mochitest/bounds/a11y.ini
@@ +3,5 @@
>  [test_list.html]
>  [test_select.html]
>  [test_zoom.html]
>  [test_zoom_text.html]
> +[test_canvas.html]

it's good to consider to move the test under test/elm so you can add move tests there like hit testing when implemented

::: accessible/tests/mochitest/bounds/test_canvas.html
@@ +17,5 @@
> +  <script type="application/javascript"
> +          src="../layout.js"></script>
> +
> +  <script type="application/javascript">
> +  SpecialPowers.setBoolPref("canvas.hitregions.enabled", true);

don't you need to off the pref after the test? do we care?

@@ +27,5 @@
> +      context.beginPath();
> +      context.rect(10, 10, 150, 100);
> +      context.addHitRegion({control: element});
> +      var input = getAccessible("showA");
> +      var [xLI, yLI, widthLI, heightLI] = getBounds(input);

nit: what LI means? It sounds like HTML:li. You could use simple accX, accY etc

@@ +31,5 @@
> +      var [xLI, yLI, widthLI, heightLI] = getBounds(input);
> +      ok(xLI == 35, "xLI should be 35 and not " + xLI);
> +      ok(yLI == 404, "yLI should be 404 and not " + yLI);
> +      ok(widthLI == 150, "widthLI should be 150 and not " + widthLI);
> +      ok(heightLI == 100, "heightLI should be 100 and not " + heightLI);

it seems like is() function should be used here

it might be nicer to test
(canvasX + x, xLI)
Attachment #8374460 - Flags: review+
Attachment #8374416 - Flags: review?(surkov.alexander)
Rik, could you please summarize (point to the comment) what kind of feedback you wait from Robert?
Attached patch 6. some bug fixes to match spec (obsolete) — Splinter Review
Attachment #8374415 - Attachment is obsolete: true
Attached patch 7. bounds + error handling tests (obsolete) — Splinter Review
Attachment #8374460 - Attachment is obsolete: true
(In reply to alexander :surkov from comment #58)

> ::: accessible/tests/mochitest/bounds/test_canvas.html
> @@ +17,5 @@
> > +  <script type="application/javascript"
> > +          src="../layout.js"></script>
> > +
> > +  <script type="application/javascript">
> > +  SpecialPowers.setBoolPref("canvas.hitregions.enabled", true);
> 
> don't you need to off the pref after the test? do we care?

ignore it
(In reply to alexander :surkov from comment #58)
> Comment on attachment 8374460 [details] [diff] [review]
> 7. bounds + error handling tests
> 
> Review of attachment 8374460 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> you'd need to have r+ for content mochitest

Good to know!

> 
> ::: accessible/tests/mochitest/bounds/a11y.ini
> @@ +3,5 @@
> >  [test_list.html]
> >  [test_select.html]
> >  [test_zoom.html]
> >  [test_zoom_text.html]
> > +[test_canvas.html]
> 
> it's good to consider to move the test under test/elm so you can add move
> tests there like hit testing when implemented

Will do

> 
> ::: accessible/tests/mochitest/bounds/test_canvas.html
> @@ +17,5 @@
> > +  <script type="application/javascript"
> > +          src="../layout.js"></script>
> > +
> > +  <script type="application/javascript">
> > +  SpecialPowers.setBoolPref("canvas.hitregions.enabled", true);
> 
> don't you need to off the pref after the test? do we care?
> 
> @@ +27,5 @@
> > +      context.beginPath();
> > +      context.rect(10, 10, 150, 100);
> > +      context.addHitRegion({control: element});
> > +      var input = getAccessible("showA");
> > +      var [xLI, yLI, widthLI, heightLI] = getBounds(input);
> 
> nit: what LI means? It sounds like HTML:li. You could use simple accX, accY
> etc

yes, I copied it from another test that used <li>. Will fix.

> 
> @@ +31,5 @@
> > +      var [xLI, yLI, widthLI, heightLI] = getBounds(input);
> > +      ok(xLI == 35, "xLI should be 35 and not " + xLI);
> > +      ok(yLI == 404, "yLI should be 404 and not " + yLI);
> > +      ok(widthLI == 150, "widthLI should be 150 and not " + widthLI);
> > +      ok(heightLI == 100, "heightLI should be 100 and not " + heightLI);
> 
> it seems like is() function should be used here
> 
> it might be nicer to test
> (canvasX + x, xLI)

That is better! I will make that change
(In reply to alexander :surkov from comment #59)
> Rik, could you please summarize (point to the comment) what kind of feedback
> you wait from Robert?

https://bugzilla.mozilla.org/show_bug.cgi?id=966591#c30
Attached patch 7. bounds + error handling tests (obsolete) — Splinter Review
Attachment #8374796 - Attachment is obsolete: true
Attachment #8374795 - Flags: review?(roc)
Comment on attachment 8375206 [details] [diff] [review]
7. bounds + error handling tests

Does this look OK now?

I think this is enough for a minimal implementation of addHitRegion. What do you think?
Flags: needinfo?(surkov.alexander)
Comment on attachment 8375206 [details] [diff] [review]
7. bounds + error handling tests

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

::: accessible/tests/mochitest/elm/test_canvas.html
@@ +34,5 @@
> +      var [accX, accY, accWidth, accHeight] = getBounds(input);
> +      is(accX, cnvX + 10, "accX should be 10 and not " + accX);
> +      is(accY, cnvY + 10, "accY should be 10 and not " + accY);
> +      is(accWidth, 150, "accWidth should be 150 and not " + accWidth);
> +      is(accHeight, 100, "accHeight should be 100 and not " + accHeight);

I would use constants instead, up to you
(In reply to Rik Cabanier from comment #67)
> Comment on attachment 8375206 [details] [diff] [review]
> 7. bounds + error handling tests
> 
> Does this look OK now?

yep (pls check with roc content part)

> I think this is enough for a minimal implementation of addHitRegion. What do
> you think?

sure, that was the plan if I understand right
Flags: needinfo?(surkov.alexander)
Comment on attachment 8374795 [details] [diff] [review]
6. some bug fixes to match spec

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2386,2 @@
>    if (!canvas || !nsContentUtils::ContentIsDescendantOf(options.mControl, canvas)) {
> +    IsDescendant = false;

isDescendant

@@ +2399,5 @@
>    Matrix ctm = mTarget->GetTransform();
>    ctm.Scale(AppUnitsPerCSSPixel(), AppUnitsPerCSSPixel());
>    mgfx::Rect* bounds = new mgfx::Rect(mPath->GetBounds(ctm));
> +  if ((bounds->width == 0) || (bounds->height == 0) || !bounds->IsFinite()) {
> +    // The specified pixels has no pixels.

"The specified region"

@@ +2407,5 @@
> +    return;
> +  }
> +
> +  if (IsDescendant) {
> +    options.mControl->SetProperty(nsGkAtoms::hitregion, bounds, ReleaseBBoxPropertyValue, true);

Actually, since bounds is in appunits, it should be stored as an nsRect instead of gfx::Rect.
Attachment #8374795 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #70)
> Comment on attachment 8374795 [details] [diff] [review]
> 6. some bug fixes to match spec
> 
> Review of attachment 8374795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> @@ +2386,2 @@
> >    if (!canvas || !nsContentUtils::ContentIsDescendantOf(options.mControl, canvas)) {
> > +    IsDescendant = false;
> 
> isDescendant
> 
> @@ +2399,5 @@
> >    Matrix ctm = mTarget->GetTransform();
> >    ctm.Scale(AppUnitsPerCSSPixel(), AppUnitsPerCSSPixel());
> >    mgfx::Rect* bounds = new mgfx::Rect(mPath->GetBounds(ctm));
> > +  if ((bounds->width == 0) || (bounds->height == 0) || !bounds->IsFinite()) {
> > +    // The specified pixels has no pixels.
> 
> "The specified region"
> 
> @@ +2407,5 @@
> > +    return;
> > +  }
> > +
> > +  if (IsDescendant) {
> > +    options.mControl->SetProperty(nsGkAtoms::hitregion, bounds, ReleaseBBoxPropertyValue, true);
> 
> Actually, since bounds is in appunits, it should be stored as an nsRect
> instead of gfx::Rect.

Does that mean I have to redo the other patches, or can I just do this in this patch?
(In reply to Rik Cabanier from comment #71)

> > Actually, since bounds is in appunits, it should be stored as an nsRect
> > instead of gfx::Rect.
> 
> Does that mean I have to redo the other patches, or can I just do this in
> this patch?

I think you need to update all other patches to meet the change (like irrc a11y patch won't need a gfx:Rect local variable anymore) but you don't have to rerequest review for all of them since the change is rather trivial.
Attachment #8370425 - Attachment is obsolete: true
Attached patch 5. Pass hit bounds to a11y code (obsolete) — Splinter Review
Attachment #8371078 - Attachment is obsolete: true
Attached patch 6. some bug fixes to match spec (obsolete) — Splinter Review
Attachment #8374795 - Attachment is obsolete: true
Attachment #8375910 - Flags: review?(roc)
Attached patch 5. Pass hit bounds to a11y code (obsolete) — Splinter Review
Attachment #8375909 - Attachment is obsolete: true
Attached patch 6. some bug fixes to match spec (obsolete) — Splinter Review
Attachment #8375910 - Attachment is obsolete: true
Attachment #8375910 - Flags: review?(roc)
Attachment #8375918 - Flags: review?(roc)
Comment on attachment 8375918 [details] [diff] [review]
6. some bug fixes to match spec

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2405,5 @@
> +  if (isDescendant) {
> +    nsRect* nsBounds = new nsRect(bounds.x * AppUnitsPerCSSPixel(),
> +                                  bounds.y * AppUnitsPerCSSPixel(),
> +                                  bounds.width * AppUnitsPerCSSPixel(),
> +                                  bounds.height * AppUnitsPerCSSPixel());

These could overflow what can be represented in an nsRect. Call nsLayoutUtils::RoundGfxRectToAppRect.
Attachment #8375918 - Flags: review?(roc) → review-
Attached patch 5. Pass hit bounds to a11y code (obsolete) — Splinter Review
Attachment #8375917 - Attachment is obsolete: true
Attached patch 6. some bug fixes to match spec (obsolete) — Splinter Review
Attachment #8375918 - Attachment is obsolete: true
Attachment #8376055 - Flags: review?(roc)
Keywords: checkin-needed
Please fix your commit messages and re-request checkin.
Flags: needinfo?(roc)
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #81)
> Please fix your commit messages and re-request checkin.

What is wrong with the commit messages?
Flags: needinfo?(ryanvm)
They're all the same?
Flags: needinfo?(ryanvm)
Attachment #8371076 - Attachment is obsolete: true
Attachment #8376054 - Attachment is obsolete: true
Attached patch 6. some bug fixes to match spec (obsolete) — Splinter Review
Attachment #8376055 - Attachment is obsolete: true
Attached patch 7. bounds + error handling tests (obsolete) — Splinter Review
Attachment #8375206 - Attachment is obsolete: true
Keywords: checkin-needed
Backed out for a mochitest leak :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb8597ff68ea

https://tbpl.mozilla.org/php/getParsedLog.php?id=35045222&tree=Mozilla-Inbound

06:54:38     INFO -  TEST-INFO | leakcheck | leaked 2 nsRect (32 bytes)
06:54:38  WARNING -  TEST-UNEXPECTED-FAIL | leakcheck | 32 bytes leaked (nsRect)
(In reply to Ed Morley [:edmorley UTC+0] from comment #93)
> Also:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=35051054&tree=Mozilla-
> Inbound

This seems only to fail for B2G. Is there a way to disable that platform or to turn the runtime flag on?
Flags: needinfo?(emorley)
Attachment #8379396 - Attachment is obsolete: true
Attachment #8379397 - Attachment is obsolete: true
Keywords: checkin-needed
successful try run: https://tbpl.mozilla.org/?tree=Try&rev=99eeaa50e559

I figured out the memory leak
Flags: needinfo?(emorley)
Summary: Add support for Hit regions in Canvas → Add basic support for Hit regions in Canvas
This broke --disable-accessibility builds, with:
{
 0:18.21 /scratch/work/builds/mozilla-inbound/mozilla/content/canvas/src/CanvasRenderingContext2D.cpp:621:47: error: no member named 'hitregion' in 'nsGkAtoms'
 0:18.21   aEntry->mElement->DeleteProperty(nsGkAtoms::hitregion);
 0:18.21                                    ~~~~~~~~~~~^
 0:18.27 /scratch/work/builds/mozilla-inbound/mozilla/content/canvas/src/CanvasRenderingContext2D.cpp:2420:49: error: no member named 'hitregion' in 'nsGkAtoms'
 0:18.27     options.mControl->DeleteProperty(nsGkAtoms::hitregion);
 0:18.28                                      ~~~~~~~~~~~^
 0:18.28 /scratch/work/builds/mozilla-inbound/mozilla/content/canvas/src/CanvasRenderingContext2D.cpp:2421:46: error: no member named 'hitregion' in 'nsGkAtoms'
 0:18.28     options.mControl->SetProperty(nsGkAtoms::hitregion, nsBounds,
 0:18.28                                   ~~~~~~~~~~~^
 0:18.28 /scratch/work/builds/mozilla-inbound/mozilla/content/canvas/src/CanvasRenderingContext2D.cpp:2439:45: error: no member named 'hitregion' in 'nsGkAtoms'
 0:18.28   info->mElement->DeleteProperty(nsGkAtoms::hitregion);
 0:18.28                                  ~~~~~~~~~~~^
}

Part 3 ( http://hg.mozilla.org/integration/mozilla-inbound/rev/5692dd63d785 ) added nsGkAtoms::hitregion to an #ifdeffed chunk of nsGkAtomList.h, but then added non-#ifdeffed usages of this atom in CanvasRenderingContext2D.cpp.

Either the atom needs to be outside of the #ifdef, or its usages all need to be #ifdef ACCESSIBILITY.

cabanier, could you figure out which of those is more appropriate and post a followup patch to fix this?
Flags: needinfo?(cabanier)
(In reply to Daniel Holbert [:dholbert] from comment #100)
> Either the atom needs to be outside of the #ifdef, or its usages all need to
> be #ifdef ACCESSIBILITY.

(IMHO the latter would be more hygenic, if it's possible/sensible to add those #ifdefs.)
Flags: needinfo?(cabanier)
Attachment #8381669 - Flags: review?(dholbert)
Comment on attachment 8381669 [details] [diff] [review]
8. Fix compile issue with non-a11y build

># HG changeset patch
># Parent 5e4b40a4da7c1a7c177cc54d90765635b8adc0ef
># User Rik Cabanier <cabanier@adobe.com>
>Bug 966591 - fix compile issue with non-a11y builds

That message is a bit too vague -- maybe prepend "Add #ifdef guards to" to your text there.

>diff --git a/content/canvas/src/CanvasRenderingContext2D.cpp b/content/canvas/src/CanvasRenderingContext2D.cpp
> PLDHashOperator
> CanvasRenderingContext2D::RemoveHitRegionProperty(RegionInfo* aEntry, void*)
> {
>+#ifdef ACCESSIBILITY
>   aEntry->mElement->DeleteProperty(nsGkAtoms::hitregion);
>+#endif
>   return PL_DHASH_NEXT;
> }
[...]
>+#ifdef ACCESSIBILITY
>   mHitRegionsOptions.EnumerateEntries(RemoveHitRegionProperty, nullptr);
>+#endif

Is this the only usage of RemoveHitRegionProperty? If so, probably better to wrap the whole function definition (and declaration) in an #ifdef, rather than that one ::hitregion line.

>@@ -2412,36 +2416,40 @@ CanvasRenderingContext2D::AddHitRegion(c
>+#ifdef ACCESSIBILITY
>     options.mControl->DeleteProperty(nsGkAtoms::hitregion);
>     options.mControl->SetProperty(nsGkAtoms::hitregion, nsBounds,
>                                   ReleaseBBoxPropertyValue);
>+#endif

Similarly, this is the only usage of ReleaseBBoxPropertyValue, so that function (ReleaseBBoxPropertyValue) should be entirely #ifdeffed as well. (My --enable-warnings-as-errors build has a compile error from "unused function" with this patch as it currently stands, as a result of that.)

r=me with the above addressed
Attachment #8381669 - Flags: review?(dholbert) → review+
Comment on attachment 8381669 [details] [diff] [review]
8. Fix compile issue with non-a11y build

>@@ -2412,36 +2416,40 @@ CanvasRenderingContext2D::AddHitRegion(c
>   if (isDescendant) {
>     nsRect* nsBounds = new nsRect();
>     gfxRect rect(bounds.x, bounds.y, bounds.width, bounds.height);
>     *nsBounds = nsLayoutUtils::RoundGfxRectToAppRect(rect, AppUnitsPerCSSPixel());
>+#ifdef ACCESSIBILITY
>     options.mControl->DeleteProperty(nsGkAtoms::hitregion);
>     options.mControl->SetProperty(nsGkAtoms::hitregion, nsBounds,
>                                   ReleaseBBoxPropertyValue);
>+#endif
>   }

As you noted in IRC, this whole chunk should be #ifdeffed, too.
Attachment #8381669 - Attachment is obsolete: true
Keywords: checkin-needed
I sanity-checked that the revised part 8 fixes my compile issues, and pushed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9a2c52728a
Keywords: checkin-needed
seems canvas does not compile anymore with accessibiltiy disabled
CanvasRenderingContext2D.cpp:2420:38: error: 'hitregion' is not a member of 'nsGkAtoms'
(In reply to Oleg Romashin (:romaxa) from comment #108)
> seems canvas does not compile anymore with accessibiltiy disabled
> CanvasRenderingContext2D.cpp:2420:38: error: 'hitregion' is not a member of
> 'nsGkAtoms'

That should have been fixed with the last patch. Is it not building if that one is added?
I'm guessing romaxa simply didn't have that last patch, since it was only merged to mozilla-central this evening

romaxa, please comment if you still hit this issue with latest mozilla-central.
Oh, right, I just missed the patch in m-c. sorry
Does this test look ok?
Try run: https://tbpl.mozilla.org/?tree=Try&rev=14f03976450a
Attachment #8383494 - Flags: feedback?(surkov.alexander)
Keywords: checkin-needed
Attachment #8383494 - Flags: feedback?(surkov.alexander)
Should this have some sort of review before landing?
Keywords: checkin-needed
Comment on attachment 8383494 [details] [diff] [review]
9. add test file for Accessible::ChildAtPoint calling into hit regions

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

some style nits:
1) no spaces in end of line
2) spaces around operators like x + y

r=me with nits addressed

::: accessible/tests/mochitest/hittest/test_canvas_hitregion.html
@@ +14,5 @@
> +
> +  <script type="application/javascript">
> +    SpecialPowers.setBoolPref("canvas.hitregions.enabled", true);
> +
> +    function drawCheckbox(context, element, x, y) 

probably redrawCheckbox to make it clearer we do that whenever checkbox state is changed

@@ +43,5 @@
> +
> +    function doTest()
> +    {
> +      // Hit testing. See bug #726097
> +      getNode("hittest").scrollIntoView(true);

do you need to wrap canvas by div container. if you need to scroll canvas into view then anyway you don't really to refer to that bug number

@@ +50,5 @@
> +      drawCheckbox(context, document.getElementById('hitcheck'), 20, 40);
> +
> +      var hitcanvas = getAccessible("hitcanvas");
> +      var hitcheck = getAccessible("hitcheck");
> +      //var hittest = getAccessible("hittest");

unneeded?

@@ +56,5 @@
> +      var [hitX, hitY, hitWidth, hitHeight] = getBounds(hitcanvas);
> +
> +      var docAcc = getAccessible(document);
> +      var tgtX = hitX+25;
> +      var tgtY = hitY+45;

where the delta goes from?

@@ +64,5 @@
> +
> +      tgtY = hitY+75;
> +      hitAcc = docAcc.getDeepestChildAtPoint(tgtX, tgtY);
> +      is(hitAcc, hitcanvas, "Hit match at " + tgtX + "," + tgtY + 
> +                          ". Found: " + prettyName(hitAcc));

it'd be nice to comment what cases you test here (for exmaple, hit test for the point shared between shadow DOM checkbox and drawn checkbox boundaries or hit test for the point belonging to canvas drawn checkbox and not belonging to DOM checkbox boundaries.

@@ +78,5 @@
> +<body>
> +
> +  <a target="_blank"
> +     href="https://bugzilla.mozilla.org/show_bug.cgi?id=726097"
> +     title="nsIAccessible::childAtPoint() from browser tests">Mozilla Bug 726097</a>

fix bug number please
Attachment #8383494 - Flags: review+
Keywords: checkin-needed
Blocks: 979692
Blocks: 979995
Depends on: 989708
Rik We found a bug where clearRect does not clear out hit regions it encompasses. Do I file a separate bug?
(In reply to Rich Schwerdtfeger from comment #118)
> Rik We found a bug where clearRect does not clear out hit regions it
> encompasses. Do I file a separate bug?
Yes, please.

---
Docs (with a note that this is still behind a pref)
https://developer.mozilla.org/en-US/Firefox/Releases/30#Interfaces.2FAPIs.2FDOM
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D.addHitRegion
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D.removeHitRegion
(In reply to Rich Schwerdtfeger from comment #118)
> Rik We found a bug where clearRect does not clear out hit regions it
> encompasses. Do I file a separate bug?

See bug 1121581.
You need to log in before you can comment on or make changes to this bug.