966591 - Add basic support for Hit regions in Canvas

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Rik Cabanier]

Provide a simple version of hit regions per:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#hit-regions

Comment 1

[Rik Cabanier]

Attachment: 1. put very basic hit region interface in + add runtime flag

try run: https://tbpl.mozilla.org/?tree=Try&rev=04ba0981d636

Comment 2

[Rik Cabanier]

Attachment: 1. put very basic hit region interface in + add runtime flag

try server run: https://tbpl.mozilla.org/?tree=Try&rev=04ba0981d636

Comment 3

[alexander :surkov (:asurkov)]

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.

Comment 4

[alexander :surkov (:asurkov)]

(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?

Comment 5

[Rik Cabanier]

(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?

Comment 6

[alexander :surkov (:asurkov)]

(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

Comment 7

[Rik Cabanier]

ok. Let me know if you prefer 1 big patch or a bunch of little ones

Comment 8

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

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 )

Comment 9

[Rik Cabanier]

Attachment: 2. Validate input to AddHitRegion. Provide partial implementation

Comment 10

[Rik Cabanier]

Attachment: 3. Compute bounds of current path and set them as the accessible region

Comment 11

[Rik Cabanier]

Attachment: 3. Compute bounds of current path and set them as the accessible region

Comment 12

[Rik Cabanier]

Attachment: 4. complete implementation of removeHitRegion

Comment 13

[alexander :surkov (:asurkov)]

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 14

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

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

Comment 15

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

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 (

Comment 16

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

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 (

Comment 17

[Rik Cabanier]

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?

Comment 18

[Rik Cabanier]

Attachment: 2. Validate input to AddHitRegion. Provide partial implementation

Comment 19

[Rik Cabanier]

Attachment: 2. Validate input to AddHitRegion. Provide partial implementation

Comment 20

[Rik Cabanier]

Attachment: 2. Validate input to AddHitRegion. Provide partial implementation

Comment 21

[Rik Cabanier]

Attachment: 3. Compute bounds of current path and set them as the accessible region

Comment 22

[Rik Cabanier]

Attachment: 4. complete implementation of removeHitRegion

Comment 23

[Rik Cabanier]

Attachment: 5. Pass hit bounds to a11y code

Comment 24

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

(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 25

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

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 26

[Rik Cabanier]

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.

Comment 27

[Rik Cabanier]

Attachment: 2. Validate input to AddHitRegion. Provide partial implementation

Comment 28

[Rik Cabanier]

Attachment: 3. Compute bounds of current path and set them as the accessible region

Comment 29

[Rik Cabanier]

Attachment: 4. complete implementation of removeHitRegion

Comment 30

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

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.

Comment 31

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

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.

Comment 32

[Rik Cabanier]

(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

Comment 33

[alexander :surkov (:asurkov)]

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 34

[Rik Cabanier]

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.

Comment 35

[Rik Cabanier]

Attachment: 5. Pass hit bounds to a11y code

Comment 36

[Rik Cabanier]

Attachment: 5. Pass hit bounds to a11y code

Comment 37

[alexander :surkov (:asurkov)]

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 38

[Rik Cabanier]

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.

Comment 39

[alexander :surkov (:asurkov)]

(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)

Comment 40

[Rik Cabanier]

Attachment: 5. Pass hit bounds to a11y code

Comment 41

[Rik Cabanier]

(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 42

[alexander :surkov (:asurkov)]

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)

Comment 43

[Rik Cabanier]

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.

Comment 44

[alexander :surkov (:asurkov)]

(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

Comment 45

[Rik Cabanier]

(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.

Comment 46

[Rik Cabanier]

Attachment: 5. Pass hit bounds to a11y code

Comment 47

[Rik Cabanier]

Attachment: 5. Pass hit bounds to a11y code

Comment 48

[Rik Cabanier]

Attachment: 5. Pass hit bounds to a11y code

Comment 49

[Rik Cabanier]

Attachment: 4. complete implementation of removeHitRegion

Comment 50

[Rik Cabanier]

Attachment: 5. Pass hit bounds to a11y code

Sorry. Patch got messed up by moving between pc and mac

Comment 51

[Trevor Saunders (:tbsaunde)]

> ::: 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.

Comment 52

[Rik Cabanier]

(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.

Comment 53

[Rik Cabanier]

Attachment: 6. some bug fixes to match spec

Comment 54

[Rik Cabanier]

Attachment: 7. bounds + error handling tests

Comment 55

[Rik Cabanier]

Attachment: 7. bounds + error handling tests

Comment 56

[Rik Cabanier]

try server build: https://tbpl.mozilla.org/?tree=Try&rev=20eca4fda27e

Comment 57

[Rik Cabanier]

Attachment: 7. bounds + error handling tests

Comment 58

[alexander :surkov (:asurkov)]

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)

Comment 59

[alexander :surkov (:asurkov)]

Rik, could you please summarize (point to the comment) what kind of feedback you wait from Robert?

Comment 60

[Rik Cabanier]

Attachment: 6. some bug fixes to match spec

Comment 61

[Rik Cabanier]

Attachment: 7. bounds + error handling tests

Comment 62

[alexander :surkov (:asurkov)]

(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

Comment 63

[Rik Cabanier]

(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

Comment 64

[Rik Cabanier]

(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

Comment 65

[Rik Cabanier]

Attachment: 7. bounds + error handling tests

Comment 66

[Rik Cabanier]

try server run: https://tbpl.mozilla.org/?tree=Try&rev=a0775f0bf042

Comment 67

[Rik Cabanier]

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?

Comment 68

[alexander :surkov (:asurkov)]

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

Comment 69

[alexander :surkov (:asurkov)]

(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

Comment 70

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

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.

Comment 71

[Rik Cabanier]

(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?

Comment 72

[alexander :surkov (:asurkov)]

(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.

Comment 73

[Rik Cabanier]

Attachment: 3. Compute bounds of current path and set them as the accessible region

Comment 74

[Rik Cabanier]

Attachment: 5. Pass hit bounds to a11y code

Comment 75

[Rik Cabanier]

Attachment: 6. some bug fixes to match spec

Comment 76

[Rik Cabanier]

Attachment: 5. Pass hit bounds to a11y code

Comment 77

[Rik Cabanier]

Attachment: 6. some bug fixes to match spec

Comment 78

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

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.

Comment 79

[Rik Cabanier]

Attachment: 5. Pass hit bounds to a11y code

Comment 80

[Rik Cabanier]

Attachment: 6. some bug fixes to match spec

Comment 81

[Ryan VanderMeulen [:RyanVM]]

Please fix your commit messages and re-request checkin.

Comment 82

[Rik Cabanier]

(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?

Comment 83

[Ryan VanderMeulen [:RyanVM]]

They're all the same?

Comment 84

[Rik Cabanier]

Attachment: 1. put very basic hit region interface in + add runtime flag

Comment 85

[Rik Cabanier]

Attachment: 2. Validate input to AddHitRegion. Provide partial implementation

Comment 86

[Rik Cabanier]

Attachment: 3. Compute bounds of current path and set them as the accessible region

Comment 87

[Rik Cabanier]

Attachment: 4. complete implementation of removeHitRegion

Comment 88

[Rik Cabanier]

Attachment: 5. Pass hit bounds to a11y code

Comment 89

[Rik Cabanier]

Attachment: 6. some bug fixes to match spec

Comment 90

[Rik Cabanier]

Attachment: 7. bounds + error handling tests

Comment 91

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/895728963f16
https://hg.mozilla.org/integration/mozilla-inbound/rev/261a1ca9045c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c66968c2edd
https://hg.mozilla.org/integration/mozilla-inbound/rev/81ba9bddd84e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ac56da10646
https://hg.mozilla.org/integration/mozilla-inbound/rev/c428b424ef95
https://hg.mozilla.org/integration/mozilla-inbound/rev/b44289e756fc

Comment 92

[Ryan VanderMeulen [:RyanVM]]

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)

Comment 93

[Ed Morley [:emorley]]

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

Comment 94

[Rik Cabanier]

(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?

Comment 95

[Rik Cabanier]

Attachment: 6. some bug fixes to match spec

Comment 96

[Rik Cabanier]

Attachment: 7. bounds + error handling tests

Comment 97

[Rik Cabanier]

successful try run: https://tbpl.mozilla.org/?tree=Try&rev=99eeaa50e559

I figured out the memory leak

Comment 98

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24f3ddfd7c48
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e64b0a7271
https://hg.mozilla.org/integration/mozilla-inbound/rev/5692dd63d785
https://hg.mozilla.org/integration/mozilla-inbound/rev/9779b2fd4ec3
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec50b53150ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc089133d229
https://hg.mozilla.org/integration/mozilla-inbound/rev/675c6efa096f

Comment 99

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/24f3ddfd7c48
https://hg.mozilla.org/mozilla-central/rev/a1e64b0a7271
https://hg.mozilla.org/mozilla-central/rev/5692dd63d785
https://hg.mozilla.org/mozilla-central/rev/9779b2fd4ec3
https://hg.mozilla.org/mozilla-central/rev/ec50b53150ca
https://hg.mozilla.org/mozilla-central/rev/fc089133d229
https://hg.mozilla.org/mozilla-central/rev/675c6efa096f

\m/ :D

Comment 100

[Daniel Holbert [:dholbert]]

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?

Comment 101

[Daniel Holbert [:dholbert]]

(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.)

Comment 102

[Rik Cabanier]

Attachment: 8. Fix compile issue with non-a11y build

Comment 103

[Daniel Holbert [:dholbert]]

Comment on attachment 8381669 [details] [diff] [review]
8. Fix compile issue with non-a11y build

># HG changeset patch
># Parent 5e4b40a4da7c1a7c177cc54d90765635b8adc0ef
># User Rik Cabanier <[e-mail]>
>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

Comment 104

[Daniel Holbert [:dholbert]]

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.

Comment 105

[Rik Cabanier]

Attachment: 8. Fix compile issue with non-a11y build

Comment 106

[Daniel Holbert [:dholbert]]

I sanity-checked that the revised part 8 fixes my compile issues, and pushed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9a2c52728a

Comment 107

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5d9a2c52728a

Comment 108

[Oleg Romashin (:romaxa)]

seems canvas does not compile anymore with accessibiltiy disabled
CanvasRenderingContext2D.cpp:2420:38: error: 'hitregion' is not a member of 'nsGkAtoms'

Comment 109

[Rik Cabanier]

(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?

Comment 110

[Daniel Holbert [:dholbert]]

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.

Comment 111

[Oleg Romashin (:romaxa)]

Oh, right, I just missed the patch in m-c. sorry

Comment 112

[Rik Cabanier]

Attachment: 9. add test file for Accessible::ChildAtPoint calling into hit regions

Does this test look ok?
Try run: https://tbpl.mozilla.org/?tree=Try&rev=14f03976450a

Comment 113

[Ryan VanderMeulen [:RyanVM]]

Should this have some sort of review before landing?

Comment 114

[alexander :surkov (:asurkov)]

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

Comment 115

[Rik Cabanier]

Attachment: 9. add test file for Accessible::ChildAtPoint calling into hit regions

Try build: https://tbpl.mozilla.org/?tree=Try&rev=dd3c5f8da65e

Comment 116

[Ryan VanderMeulen [:RyanVM]]

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

Comment 117

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e40e9359bce

Comment 118

[Rich Schwerdtfeger]

Rik We found a bug where clearRect does not clear out hit regions it encompasses. Do I file a separate bug?

Comment 119

[Florian Scholz (Open Web Docs)]

(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

Comment 120

[Florian Scholz (Open Web Docs)]

(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.