Add basic support for Hit regions in Canvas

RESOLVED FIXED in mozilla30

Status

()

Core
Canvas: 2D
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Rik Cabanier, Assigned: Rik Cabanier)

Tracking

(Blocks: 4 bugs, {dev-doc-complete})

Trunk
mozilla30
dev-doc-complete
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

4 years ago
Assignee: nobody → cabanier
(Assignee)

Comment 1

4 years ago
Created attachment 8369018 [details] [diff] [review]
1. put very basic hit region interface in + add runtime flag

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

Comment 2

4 years ago
Created attachment 8369019 [details] [diff] [review]
1. put very basic hit region interface in + add runtime flag

try server run: https://tbpl.mozilla.org/?tree=Try&rev=04ba0981d636
Attachment #8369018 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8369018 - Flags: review?(surkov.alexander)

Comment 3

4 years ago
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)

Updated

4 years ago
Attachment #8369018 - Flags: review?(surkov.alexander)

Comment 4

4 years ago
(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

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

Comment 6

4 years ago
(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

4 years ago
ok. Let me know if you prefer 1 big patch or a bunch of little ones
Keywords: dev-doc-needed
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

4 years ago
Blocks: 802882
(Assignee)

Comment 9

4 years ago
Created attachment 8369700 [details] [diff] [review]
2. Validate input to AddHitRegion. Provide partial implementation
(Assignee)

Comment 10

4 years ago
Created attachment 8369743 [details] [diff] [review]
3. Compute bounds of current path and set them as the accessible region
Attachment #8369743 - Flags: review?(roc)
(Assignee)

Updated

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

Comment 11

4 years ago
Created attachment 8369753 [details] [diff] [review]
3. Compute bounds of current path and set them as the accessible region
Attachment #8369743 - Attachment is obsolete: true
Attachment #8369743 - Flags: review?(roc)
Attachment #8369753 - Flags: review?(roc)
(Assignee)

Comment 12

4 years ago
Created attachment 8369757 [details] [diff] [review]
4. complete implementation of removeHitRegion
Attachment #8369757 - Flags: review?(roc)

Comment 13

4 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
@@ +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

4 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

4 years ago
Created attachment 8369859 [details] [diff] [review]
2. Validate input to AddHitRegion. Provide partial implementation
Attachment #8369700 - Attachment is obsolete: true
Attachment #8369859 - Flags: review?(roc)
(Assignee)

Updated

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

Comment 19

4 years ago
Created attachment 8369860 [details] [diff] [review]
2. Validate input to AddHitRegion. Provide partial implementation
Attachment #8369859 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
Created attachment 8369862 [details] [diff] [review]
2. Validate input to AddHitRegion. Provide partial implementation
Attachment #8369860 - Attachment is obsolete: true
(Assignee)

Comment 21

4 years ago
Created attachment 8369866 [details] [diff] [review]
3. Compute bounds of current path and set them as the accessible region
Attachment #8369753 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
Created attachment 8369869 [details] [diff] [review]
4. complete implementation of removeHitRegion
Attachment #8369757 - Attachment is obsolete: true
Attachment #8369869 - Flags: review?(roc)
(Assignee)

Updated

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

Comment 23

4 years ago
Created attachment 8370384 [details] [diff] [review]
5. Pass hit bounds to a11y code
(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

4 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

4 years ago
Created attachment 8370424 [details] [diff] [review]
2. Validate input to AddHitRegion. Provide partial implementation
Attachment #8369862 - Attachment is obsolete: true
Attachment #8369862 - Flags: review?(roc)
Attachment #8370424 - Flags: review?(roc)
(Assignee)

Comment 28

4 years ago
Created attachment 8370425 [details] [diff] [review]
3. Compute bounds of current path and set them as the accessible region
Attachment #8369866 - Attachment is obsolete: true
(Assignee)

Comment 29

4 years ago
Created attachment 8370427 [details] [diff] [review]
4. complete implementation of removeHitRegion
Attachment #8369869 - Attachment is obsolete: true
Attachment #8369869 - Flags: review?(roc)
Attachment #8370427 - Flags: review?(roc)
(Assignee)

Updated

4 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

4 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

4 years ago
Attachment #8370384 - Flags: review?(surkov.alexander)

Comment 33

4 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));

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

4 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

4 years ago
Created attachment 8370546 [details]
5. Pass hit bounds to a11y code
Attachment #8370384 - Attachment is obsolete: true
Attachment #8370546 - Flags: review?(surkov.alexander)
(Assignee)

Comment 36

4 years ago
Created attachment 8370547 [details] [diff] [review]
5. Pass hit bounds to a11y code
Attachment #8370546 - Attachment is obsolete: true
Attachment #8370546 - Flags: review?(surkov.alexander)
Attachment #8370547 - Flags: review?(surkov.alexander)
(Assignee)

Updated

4 years ago
Attachment #8370547 - Attachment is patch: true
(Assignee)

Updated

4 years ago
Flags: needinfo?(roc)

Comment 37

4 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
@@ +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

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

Comment 39

4 years ago
(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

4 years ago
Created attachment 8370933 [details] [diff] [review]
5. Pass hit bounds to a11y code
Attachment #8370547 - Attachment is obsolete: true
Attachment #8370547 - Flags: review?(surkov.alexander)
Attachment #8370933 - Flags: review?(surkov.alexander)
(Assignee)

Updated

4 years ago
Attachment #8370933 - Attachment is patch: true
(Assignee)

Comment 41

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

4 years ago
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

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

Comment 44

4 years ago
(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

4 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

4 years ago
Created attachment 8371048 [details] [diff] [review]
5. Pass hit bounds to a11y code
Attachment #8370933 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8371048 - Attachment is patch: true
(Assignee)

Comment 47

4 years ago
Created attachment 8371052 [details] [diff] [review]
5. Pass hit bounds to a11y code
Attachment #8371048 - Attachment is obsolete: true
(Assignee)

Comment 48

4 years ago
Created attachment 8371054 [details] [diff] [review]
5. Pass hit bounds to a11y code
Attachment #8371052 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8371054 - Attachment is patch: true
(Assignee)

Comment 49

4 years ago
Created attachment 8371076 [details] [diff] [review]
4. complete implementation of removeHitRegion
Attachment #8370427 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 50

4 years ago
Created attachment 8371078 [details] [diff] [review]
5. Pass hit bounds to a11y code

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

4 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

4 years ago
Blocks: 935566
(Assignee)

Comment 53

4 years ago
Created attachment 8374415 [details] [diff] [review]
6. some bug fixes to match spec
(Assignee)

Comment 54

4 years ago
Created attachment 8374416 [details] [diff] [review]
7. bounds + error handling tests
(Assignee)

Updated

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

Updated

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

Updated

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

Comment 55

4 years ago
Created attachment 8374429 [details] [diff] [review]
7. bounds + error handling tests
Attachment #8374416 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 57

4 years ago
Created attachment 8374460 [details] [diff] [review]
7. bounds + error handling tests
Attachment #8374429 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 58

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

Updated

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

Comment 59

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

Comment 60

4 years ago
Created attachment 8374795 [details] [diff] [review]
6. some bug fixes to match spec
Attachment #8374415 - Attachment is obsolete: true
(Assignee)

Comment 61

4 years ago
Created attachment 8374796 [details] [diff] [review]
7. bounds + error handling tests
Attachment #8374460 - Attachment is obsolete: true

Comment 62

4 years ago
(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

4 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

4 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

4 years ago
Created attachment 8375206 [details] [diff] [review]
7. bounds + error handling tests
Attachment #8374796 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 67

4 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 68

4 years ago
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

4 years ago
(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

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

Comment 72

4 years ago
(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

4 years ago
Created attachment 8375907 [details] [diff] [review]
3. Compute bounds of current path and set them as the accessible region
Attachment #8370425 - Attachment is obsolete: true
(Assignee)

Comment 74

4 years ago
Created attachment 8375909 [details] [diff] [review]
5. Pass hit bounds to a11y code
Attachment #8371078 - Attachment is obsolete: true
(Assignee)

Comment 75

4 years ago
Created attachment 8375910 [details] [diff] [review]
6. some bug fixes to match spec
Attachment #8374795 - Attachment is obsolete: true
Attachment #8375910 - Flags: review?(roc)
(Assignee)

Comment 76

4 years ago
Created attachment 8375917 [details] [diff] [review]
5. Pass hit bounds to a11y code
Attachment #8375909 - Attachment is obsolete: true
(Assignee)

Comment 77

4 years ago
Created attachment 8375918 [details] [diff] [review]
6. some bug fixes to match spec
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

4 years ago
Created attachment 8376054 [details] [diff] [review]
5. Pass hit bounds to a11y code
Attachment #8375917 - Attachment is obsolete: true
(Assignee)

Comment 80

4 years ago
Created attachment 8376055 [details] [diff] [review]
6. some bug fixes to match spec
Attachment #8375918 - Attachment is obsolete: true
(Assignee)

Updated

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

Updated

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

Comment 82

4 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

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

Comment 84

4 years ago
Created attachment 8379391 [details] [diff] [review]
1. put very basic hit region interface in + add runtime flag
Attachment #8369019 - Attachment is obsolete: true
(Assignee)

Comment 85

4 years ago
Created attachment 8379392 [details] [diff] [review]
2. Validate input to AddHitRegion. Provide partial implementation
Attachment #8370424 - Attachment is obsolete: true
(Assignee)

Comment 86

4 years ago
Created attachment 8379393 [details] [diff] [review]
3. Compute bounds of current path and set them as the accessible region
Attachment #8375907 - Attachment is obsolete: true
(Assignee)

Comment 87

4 years ago
Created attachment 8379394 [details] [diff] [review]
4. complete implementation of removeHitRegion
Attachment #8371076 - Attachment is obsolete: true
(Assignee)

Comment 88

4 years ago
Created attachment 8379395 [details] [diff] [review]
5. Pass hit bounds to a11y code
Attachment #8376054 - Attachment is obsolete: true
(Assignee)

Comment 89

4 years ago
Created attachment 8379396 [details] [diff] [review]
6. some bug fixes to match spec
Attachment #8376055 - Attachment is obsolete: true
(Assignee)

Comment 90

4 years ago
Created attachment 8379397 [details] [diff] [review]
7. bounds + error handling tests
Attachment #8375206 - Attachment is obsolete: true
(Assignee)

Updated

4 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

4 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

4 years ago
Created attachment 8381197 [details] [diff] [review]
6. some bug fixes to match spec
Attachment #8379396 - Attachment is obsolete: true
(Assignee)

Comment 96

4 years ago
Created attachment 8381198 [details] [diff] [review]
7. bounds + error handling tests
Attachment #8379397 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 97

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

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

Updated

4 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

4 years ago
Created attachment 8381669 [details] [diff] [review]
8. Fix compile issue with non-a11y build
Flags: needinfo?(cabanier)
(Assignee)

Updated

4 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

4 years ago
Created attachment 8381682 [details] [diff] [review]
8. Fix compile issue with non-a11y build
Attachment #8381669 - Attachment is obsolete: true
(Assignee)

Updated

4 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

4 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

4 years ago
Created attachment 8383494 [details] [diff] [review]
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
Attachment #8383494 - Flags: feedback?(surkov.alexander)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Attachment #8383494 - Flags: feedback?(surkov.alexander)
Should this have some sort of review before landing?
Keywords: checkin-needed

Comment 114

4 years ago
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)

Comment 115

4 years ago
Created attachment 8384972 [details] [diff] [review]
9. add test file for Accessible::ChildAtPoint calling into hit regions

Try build: https://tbpl.mozilla.org/?tree=Try&rev=dd3c5f8da65e
Attachment #8383494 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Blocks: 979692
Blocks: 979925

Updated

4 years ago
Blocks: 979995
Depends on: 989708

Comment 118

4 years ago
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
Keywords: dev-doc-needed → dev-doc-complete
(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.