Add basic support for Hit regions in Canvas

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cabanier, Assigned: cabanier)

Tracking

(Blocks 3 bugs, {dev-doc-complete})

Trunk
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 41 obsolete attachments)

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

Updated

5 years ago
Assignee: nobody → cabanier
Assignee

Updated

5 years ago
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?
Assignee

Comment 5

5 years ago
(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
Assignee

Comment 7

5 years ago
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+

Updated

5 years ago
Blocks: html5test
Assignee

Updated

5 years ago
Attachment #8369700 - Flags: review?(roc)
Assignee

Comment 11

5 years ago
Attachment #8369743 - Attachment is obsolete: true
Attachment #8369743 - Flags: review?(roc)
Attachment #8369753 - Flags: review?(roc)
Assignee

Comment 12

5 years ago
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-
Assignee

Comment 17

5 years ago
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?
Assignee

Comment 18

5 years ago
Attachment #8369700 - Attachment is obsolete: true
Attachment #8369859 - Flags: review?(roc)
Assignee

Updated

5 years ago
Attachment #8369859 - Flags: review?(roc)
Assignee

Comment 19

5 years ago
Attachment #8369859 - Attachment is obsolete: true
Assignee

Comment 20

5 years ago
Attachment #8369860 - Attachment is obsolete: true
Assignee

Comment 21

5 years ago
Attachment #8369753 - Attachment is obsolete: true
Assignee

Comment 22

5 years ago
Attachment #8369757 - Attachment is obsolete: true
Attachment #8369869 - Flags: review?(roc)
Assignee

Updated

5 years ago
Attachment #8369862 - Flags: review?(roc)
Assignee

Comment 23

5 years ago
(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?
Assignee

Comment 26

5 years ago
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.
Assignee

Comment 27

5 years ago
Attachment #8369862 - Attachment is obsolete: true
Attachment #8369862 - Flags: review?(roc)
Attachment #8370424 - Flags: review?(roc)
Assignee

Comment 28

5 years ago
Attachment #8369866 - Attachment is obsolete: true
Assignee

Comment 29

5 years ago
Attachment #8369869 - Attachment is obsolete: true
Attachment #8369869 - Flags: review?(roc)
Attachment #8370427 - Flags: review?(roc)
Assignee

Updated

5 years ago
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-
Assignee

Comment 32

5 years ago
(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
Assignee

Updated

5 years ago
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
Assignee

Comment 34

5 years ago
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.
Assignee

Comment 35

5 years ago
Posted file 5. Pass hit bounds to a11y code (obsolete) —
Attachment #8370384 - Attachment is obsolete: true
Attachment #8370546 - Flags: review?(surkov.alexander)
Assignee

Comment 36

5 years ago
Attachment #8370546 - Attachment is obsolete: true
Attachment #8370546 - Flags: review?(surkov.alexander)
Attachment #8370547 - Flags: review?(surkov.alexander)
Assignee

Updated

5 years ago
Attachment #8370547 - Attachment is patch: true
Assignee

Updated

5 years ago
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.
Assignee

Comment 38

5 years ago
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)
Assignee

Comment 40

5 years ago
Attachment #8370547 - Attachment is obsolete: true
Attachment #8370547 - Flags: review?(surkov.alexander)
Attachment #8370933 - Flags: review?(surkov.alexander)
Assignee

Updated

5 years ago
Attachment #8370933 - Attachment is patch: true
Assignee

Comment 41

5 years ago
(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+
Assignee

Comment 43

5 years ago
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
Assignee

Comment 45

5 years ago
(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.
Assignee

Comment 46

5 years ago
Attachment #8370933 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8371048 - Attachment is patch: true
Assignee

Comment 47

5 years ago
Attachment #8371048 - Attachment is obsolete: true
Assignee

Comment 48

5 years ago
Attachment #8371052 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8371054 - Attachment is patch: true
Assignee

Comment 49

5 years ago
Attachment #8370427 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8371076 - Flags: review?(roc)
Assignee

Comment 50

5 years ago
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.
Assignee

Comment 52

5 years ago
(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.
Assignee

Updated

5 years ago
Blocks: 935566
Assignee

Comment 53

5 years ago
Assignee

Comment 54

5 years ago
Assignee

Updated

5 years ago
Attachment #8374415 - Flags: review?(roc)
Assignee

Updated

5 years ago
Attachment #8374416 - Flags: review?(surkov.alexander)
Assignee

Updated

5 years ago
Attachment #8374416 - Flags: review?(surkov.alexander)
Assignee

Comment 55

5 years ago
Attachment #8374416 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8374416 - Flags: review?(surkov.alexander)
Assignee

Comment 57

5 years ago
Attachment #8374429 - Attachment is obsolete: true
Assignee

Updated

5 years ago
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?
Assignee

Comment 60

5 years ago
Attachment #8374415 - Attachment is obsolete: true
Assignee

Comment 61

5 years ago
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
Assignee

Comment 63

5 years ago
(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
Assignee

Comment 64

5 years ago
(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
Assignee

Comment 65

5 years ago
Attachment #8374796 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8374795 - Flags: review?(roc)
Assignee

Comment 67

5 years ago
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-
Assignee

Comment 71

5 years ago
(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.
Assignee

Comment 73

5 years ago
Attachment #8370425 - Attachment is obsolete: true
Assignee

Comment 74

5 years ago
Attachment #8371078 - Attachment is obsolete: true
Assignee

Comment 75

5 years ago
Attachment #8374795 - Attachment is obsolete: true
Attachment #8375910 - Flags: review?(roc)
Assignee

Comment 76

5 years ago
Attachment #8375909 - Attachment is obsolete: true
Assignee

Comment 77

5 years ago
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-
Assignee

Comment 79

5 years ago
Attachment #8375917 - Attachment is obsolete: true
Assignee

Comment 80

5 years ago
Attachment #8375918 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8376055 - Flags: review?(roc)
Assignee

Updated

5 years ago
Keywords: checkin-needed
Please fix your commit messages and re-request checkin.
Flags: needinfo?(roc)
Keywords: checkin-needed
Assignee

Comment 82

5 years ago
(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?
Assignee

Updated

5 years ago
Flags: needinfo?(ryanvm)
They're all the same?
Flags: needinfo?(ryanvm)
Assignee

Comment 87

5 years ago
Attachment #8371076 - Attachment is obsolete: true
Assignee

Comment 88

5 years ago
Attachment #8376054 - Attachment is obsolete: true
Assignee

Comment 89

5 years ago
Attachment #8376055 - Attachment is obsolete: true
Assignee

Comment 90

5 years ago
Attachment #8375206 - Attachment is obsolete: true
Assignee

Updated

5 years ago
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)
Assignee

Comment 94

5 years ago
(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)
Assignee

Comment 95

5 years ago
Attachment #8379396 - Attachment is obsolete: true
Assignee

Comment 96

5 years ago
Attachment #8379397 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 97

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

I figured out the memory leak
Flags: needinfo?(emorley)
Assignee

Updated

5 years ago
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.)
Assignee

Comment 102

5 years ago
Flags: needinfo?(cabanier)
Assignee

Updated

5 years ago
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.
Assignee

Comment 105

5 years ago
Attachment #8381669 - Attachment is obsolete: true
Assignee

Updated

5 years ago
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'
Assignee

Comment 109

5 years ago
(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
Assignee

Comment 112

5 years ago
Does this test look ok?
Try run: https://tbpl.mozilla.org/?tree=Try&rev=14f03976450a
Attachment #8383494 - Flags: feedback?(surkov.alexander)
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Updated

5 years ago
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+
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Updated

5 years ago
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.