Last Comment Bug 764686 - Accessible::ChildAtPoint should be consistent with GetBounds
: Accessible::ChildAtPoint should be consistent with GetBounds
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla16
Assigned To: Eitan Isaacson [:eeejay]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-13 19:14 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-06-26 01:58 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test cases (1.98 KB, patch)
2012-06-13 19:14 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Review
Always check if point are in match's bounds in Accessible::ChildAtPoint(). (3.17 KB, patch)
2012-06-13 19:28 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Review
Bug 764686 - Always check if point are in match's bounds in Accessible::ChildAtPoint(). (6.64 KB, patch)
2012-06-14 14:52 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
tbsaunde+mozbugs: feedback+
Details | Diff | Review
Always check if point are in match's bounds in Accessible::ChildAtPoint(). r=davidb (7.34 KB, patch)
2012-06-18 18:31 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Review
Always check if point are in match's bounds in Accessible::ChildAtPoint(). r=davidb (7.34 KB, patch)
2012-06-20 16:57 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Review
Always check if point are in match's bounds in Accessible::ChildAtPoint(). r=davidb (6.86 KB, patch)
2012-06-21 10:41 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Review

Description Eitan Isaacson [:eeejay] 2012-06-13 19:14:35 PDT
Created attachment 632993 [details] [diff] [review]
Test cases

These are really two issues with the same solution:

First, there is code in ChildAtPoint() that handles cases where a child is not represented by a layout frame[1]. But it only works if the given accessible (this) is the direct parent of the non-frame children. The failure for this is demonstrated in the image map test case.

Second, I think it is reasonable to expect ChildAtPoint to have results that are consistent with the union rectangle returned in GetBounds. But if we test with a point that is in between lines of text, it falls in between the two layout frames. This is demonstrated in the "Hello World" text test.

1. http://dxr.lanedo.com/mozilla-central/accessible/src/generic/Accessible.cpp.html#l809
Comment 1 Eitan Isaacson [:eeejay] 2012-06-13 19:28:45 PDT
Created attachment 632995 [details] [diff] [review]
Always check if point are in match's bounds in Accessible::ChildAtPoint().
Comment 2 Trevor Saunders (:tbsaunde) 2012-06-14 07:15:19 PDT
Comment on attachment 632995 [details] [diff] [review]
Always check if point are in match's bounds in Accessible::ChildAtPoint().

>-  if (accessible == this) {

removing this check seems reasonable, but it means as is this patch will allow returning things that aren't kids.

areas and
>+  // sub documents (XXX: subdocuments should be handled by methods of
>+  // OuterDocAccessibles).
>+  PRUint32 childCount = ChildCount();
>+  for (PRUint32 childIdx = 0; childIdx < childCount; childIdx++) {
>+    Accessible* child = GetChildAt(childIdx);

it seems like either you should use the accessible you obtained by looking at the frame tree instead of this, or you should move this code before looking at the frame tree.

also behavior changes should really come with tests not just test cases :)
Comment 3 David Bolter [:davidb] 2012-06-14 08:36:42 PDT
Comment on attachment 632995 [details] [diff] [review]
Always check if point are in match's bounds in Accessible::ChildAtPoint().

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

This is a bit scary so let's get the mochitest changes into this patch so that we well better.

It is tempting to think about putting the rare case into the specialized implementations (image map etc) - but I'm not asking you do that here.

Trevor's concern about ensuring the frame accessible is an actual accessible descendant of |this| seems valid to me although I don't know the cases intimately.

::: accessible/src/generic/Accessible.cpp
@@ +799,5 @@
>    Accessible* accessible = contentDocAcc->GetAccessibleOrContainer(content);
>    if (!accessible)
>      return fallbackAnswer;
>  
> +  // Manually walk through accessible children and see if the are within this

What do you guys think of a comment like:

[line length needs fixing of course]

// Hurray! We have an accessible for the frame that layout gave
// us but we still need to do a couple of things:
// 1. Although it should be rare, check to see if this
// accessible itself has an accessible child at the given point 
// (recursively). This can happen where layout won't walk
// deep enough into 
// things for us, such as image map areas sub documents (XXX: 
// subdocuments should be handled by methods 
// OuterDocAccessibles).
// 2. Ensure the accessible heirarchy, specifically that this 
// accessible is a child of (or is) |this|
Comment 4 David Bolter [:davidb] 2012-06-14 08:37:22 PDT
(In reply to David Bolter [:davidb] from comment #3)
> This is a bit scary so let's get the mochitest changes into this patch so
> that we well better.

"feel" better.
Comment 5 David Bolter [:davidb] 2012-06-14 08:39:02 PDT
Comment on attachment 632995 [details] [diff] [review]
Always check if point are in match's bounds in Accessible::ChildAtPoint().

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

::: accessible/src/generic/Accessible.cpp
@@ -799,5 @@
>    Accessible* accessible = contentDocAcc->GetAccessibleOrContainer(content);
>    if (!accessible)
>      return fallbackAnswer;
>  
> -  if (accessible == this) {

BTW - I like this removal.
Comment 6 Eitan Isaacson [:eeejay] 2012-06-14 09:42:35 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Comment on attachment 632995 [details] [diff] [review]
> Always check if point are in match's bounds in Accessible::ChildAtPoint().
> 
> >-  if (accessible == this) {
> 
> removing this check seems reasonable, but it means as is this patch will
> allow returning things that aren't kids.
> 

Sorry, I don't follow. How will this allow accessibles to be returned that are not descendants of |this|?

> areas and
> >+  // sub documents (XXX: subdocuments should be handled by methods of
> >+  // OuterDocAccessibles).
> >+  PRUint32 childCount = ChildCount();
> >+  for (PRUint32 childIdx = 0; childIdx < childCount; childIdx++) {
> >+    Accessible* child = GetChildAt(childIdx);
> 
> it seems like either you should use the accessible you obtained by looking
> at the frame tree instead of this, or you should move this code before
> looking at the frame tree.
> 

Oops, oversight.

> also behavior changes should really come with tests not just test cases :)

Right, the other attachment are tests, and as David requests below, they will be in the patch.
Comment 7 Eitan Isaacson [:eeejay] 2012-06-14 09:44:38 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > Comment on attachment 632995 [details] [diff] [review]
> > Always check if point are in match's bounds in Accessible::ChildAtPoint().
> > 
> > >-  if (accessible == this) {
> > 
> > removing this check seems reasonable, but it means as is this patch will
> > allow returning things that aren't kids.
> > 
> 
> Sorry, I don't follow. How will this allow accessibles to be returned that
> are not descendants of |this|?
> 

Oops again. disregard. I read the code this time :)
Comment 8 Eitan Isaacson [:eeejay] 2012-06-14 14:52:00 PDT
Created attachment 633291 [details] [diff] [review]
Bug 764686 - Always check if point are in match's bounds in Accessible::ChildAtPoint().
Comment 9 David Bolter [:davidb] 2012-06-15 07:49:46 PDT
Comment on attachment 633291 [details] [diff] [review]
Bug 764686 - Always check if point are in match's bounds in Accessible::ChildAtPoint().

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

r=me if you get the tests passing. You might want to f? cappella on image map test changes.

::: accessible/src/generic/Accessible.cpp
@@ +818,5 @@
>  
>      child = parent;
>    }
>  
> +  // Manually walk through accessible children and see if the are within this

nit: the/they

@@ +834,5 @@
> +        aY >= childY && aY < childY + childHeight &&
> +        (child->State() & states::INVISIBLE) == 0) {
> +
> +      if (aWhichChild == eDeepestChild)
> +        return child->ChildAtPoint(aX, aY, eDeepestChild);

A recursive call after a while loop makes me itchy but in practice we should never recurse too much here (unless frame trees go haywire). This seems right.

::: accessible/tests/mochitest/hittest/test_general.html
@@ +55,5 @@
>        outOfFlow.style.top = rectArea.top + "px";
>  
>        testChildAtPoint("area", 1, 1, "area", "area");
>  
> +

nit: I spacer line is enough

@@ +68,5 @@
> +      var thelettera = getAccessible("imgmap").firstChild;
> +      // Passes
> +      hitTest("imgmap", thelettera, thelettera);
> +      // Fails
> +      hitTest("container", "imgmap", thelettera);

See bug 570322. (image map frames have a bad personality)
Comment 10 Eitan Isaacson [:eeejay] 2012-06-15 09:44:09 PDT
(In reply to David Bolter [:davidb] from comment #3)
> It is tempting to think about putting the rare case into the specialized
> implementations (image map etc) - but I'm not asking you do that here.
> 

For the record, before this patch it wouldn't matter if it were in a specialized implementation since it would not be called when the outside caller called it on a container above the parent.

In general, overrides for this method seems wrong in the current implementation, since we don't recursively call ChildAtPoint() for each matching descendant. In other words, if a user calls the method from a higher container, and it returns a leaf via layout queries, we may have skipped an accessible in the lineage that has a specialized ChildAtPoint().
Comment 11 Eitan Isaacson [:eeejay] 2012-06-15 09:47:28 PDT
(In reply to David Bolter [:davidb] from comment #9)
> @@ +68,5 @@
> > +      var thelettera = getAccessible("imgmap").firstChild;
> > +      // Passes
> > +      hitTest("imgmap", thelettera, thelettera);
> > +      // Fails
> > +      hitTest("container", "imgmap", thelettera);
> 
> See bug 570322. (image map frames have a bad personality)

Sorry, that "Fails" comment was when this was a testcase. This patch makes it pass!
Comment 12 Trevor Saunders (:tbsaunde) 2012-06-15 17:04:07 PDT
Comment on attachment 633291 [details] [diff] [review]
Bug 764686 - Always check if point are in match's bounds in Accessible::ChildAtPoint().

this seems correct f=me with that

>+      if (aWhichChild == eDeepestChild)
>+        return child->ChildAtPoint(aX, aY, eDeepestChild);
>+      else
>+        return child;

no else after return, we usually would do return aWichChild == eDirectChild ? child : accessible->ChildAtPoint(aX, aY);

>+
>+      ensureImageMapTree("imgmap");
>+      var thelettera = getAccessible("imgmap").firstChild;

nit, names should be cammel case
Comment 14 Eitan Isaacson [:eeejay] 2012-06-18 17:59:02 PDT
Backed out:
http://hg.mozilla.org/integration/mozilla-inbound/rev/df8f2986f750

Apparently there was a mid-air API collusion with bug 570322.
Comment 15 Eitan Isaacson [:eeejay] 2012-06-18 18:31:58 PDT
Created attachment 634264 [details] [diff] [review]
Always check if point are in match's bounds in Accessible::ChildAtPoint(). r=davidb

Here is an updated patch that works after the patch in bug 570322.
Comment 17 Phil Ringnalda (:philor) 2012-06-18 20:53:51 PDT
Backed out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/8cc7aa5c0c95 - Windows says "test_general.html | Wrong deepest child accessible at the point (1, 40) of 'container' - got '['p@id="paragraph" node', address: [object HTMLParagraphElement], role: paragraph, address: [xpconnect wrapped nsIAccessible]]', expected '['null node', address: [object Text], role: text leaf, name: 'Hello World', address: [xpconnect wrapped nsIAccessible]]'"
Comment 18 Eitan Isaacson [:eeejay] 2012-06-20 16:57:21 PDT
Created attachment 635132 [details] [diff] [review]
Always check if point are in match's bounds in Accessible::ChildAtPoint(). r=davidb

I removed the text in-between-line hittest since I guess it was too shaky and fails on Windows. I am very far away from having a Windows setup to test why. The image map test stays in here, so there should be coverage for these changes.
Comment 19 Eitan Isaacson [:eeejay] 2012-06-21 10:41:20 PDT
Created attachment 635372 [details] [diff] [review]
Always check if point are in match's bounds in Accessible::ChildAtPoint(). r=davidb

Oops, I think I forgot to remove the actual test. Here we go.
Comment 21 alexander :surkov 2012-06-25 22:48:59 PDT
with this change deep hit test must slow as a hell, also the deep and direct hit test code is not equivalent (deep testing has boundaries checks).
Comment 22 Eitan Isaacson [:eeejay] 2012-06-25 23:09:15 PDT
(In reply to alexander :surkov from comment #21)
> with this change deep hit test must slow as a hell,

In cases where the deepest frame's accessible has children. I think the code that was there before the patch was wrong and unoptimized. Now it is only unoptimized :)

> also the deep and direct
> hit test code is not equivalent (deep testing has boundaries checks).

They were not equivalent before this patch either.
Comment 23 Ed Morley [:emorley] 2012-06-26 01:58:54 PDT
https://hg.mozilla.org/mozilla-central/rev/a65f7692ceab

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