491657 - getDeepestChildAtPoint must return null when point is not inside of accessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 375984 [details] [diff] [review]
patch

>-  nsCOMPtr<nsIContent> content;
>+
>+  nsIContent* content = nsnull;

Why this change?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> (From update of attachment 375984 [details] [diff] [review])
> >-  nsCOMPtr<nsIContent> content;
> >+
> >+  nsIContent* content = nsnull;
> 
> Why this change?

If you would like to keep history cleaner then I can remove it. It just get rid useless AddRef/Release.

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to comment #3)
> If you would like to keep history cleaner then I can remove it. It just get rid
> useless AddRef/Release.

No, that's fine, I just wanted to fully understand the implication here and make sure we don't accidentally introduce a leak with this.

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 375984 [details] [diff] [review]
patch

Patch looks correct. We should try to find out whether this also fixes the failures in the XUL tree testcase from bug 481417.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 375984 [details] [diff] [review]
patch

You've been busy :) I'll review this one in stages.

>-  if (!mDOMNode) {
>-    return NS_ERROR_FAILURE;  // Already shut down
>-  }

(context: nsAccessible::GetChildAtPoint)
Should you add a check for IsDefunct() here?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> (From update of attachment 375984 [details] [diff] [review])
> You've been busy :) I'll review this one in stages.

trying, really :)

> >-  if (!mDOMNode) {
> >-    return NS_ERROR_FAILURE;  // Already shut down
> >-  }
> 
> (context: nsAccessible::GetChildAtPoint)
> Should you add a check for IsDefunct() here?

nsAccessible::GetChildAtPoint is assumed to be called from nsIAccessible::getChildAtPoint of nsIAccessible::getDeepestChildAtPoint which make IsDefunct() check. I think our internal methods shouldn't check defunct state. Tough again it's always fork :)

Comment 8

[[e-mail]]

Comment on attachment 375984 [details] [diff] [review]
patch

>+    if (aChild)
>+      NS_IF_ADDREF(*aChild = fallbackAnswer);
>+    if (aDeepestChild)
>+      NS_IF_ADDREF(*aDeepestChild = fallbackAnswer);
Instead of using two out parameters, you may find it easier to pass in a flag to distinguish between whether you want the child or the deepest child.

>+        if (aDeepestChild) // XXX: Is it really deepest child?
If you really want the deepest child then you would have to call GetDeepestChildAtPoint recursively.

>+    // The point is in this accessible but not in a child. We are allowed to
>+    // return |this| as the answer.
>+    if (aChild)
>+      NS_IF_ADDREF(*aChild = accessible);
>+    if (aDeepestChild)
>+      NS_IF_ADDREF(*aDeepestChild = accessible);
>   }
Missing a return NS_OK;

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 375984 [details] [diff] [review]
patch

r=me with Neil's concerns addressed.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Adding Sam in case this helps his bug 480347.

Comment 11

[alexander :surkov (:asurkov)]

checked in with Neil's comments, http://hg.mozilla.org/mozilla-central/rev/ff250122fa99

Comment 12

[Samuel B. Quiring]

Bug 480347 is that Firefox does not seem know that the drop-down menu is on top of the main Firefox window.  If (x,y) is a point on the screen in the drop-down menu and I pass (x,y) to AccessibleComponent_getAccessibleAtPoint(), Firefox does not deliver the menu item at (x,y), but instead delivers the item from the main Firefox Window at (x,y) (assuming that the drop-down menu window is on top of the main Firefox Window).

I don't think this bug (491657) helps 480347, unless it makes Firefox aware of the window stacking order.  I cannot tell if it does.

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Right. I was thinking it might return no object instead of giving you a bogus one. Not ideal I know...