245931 - Problems with accessibility API focus events

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[Aaron Leventhal]

Problems:
1. When we fire focus events in the accessibility API, the accessibility object
that's focused doesn't always have STATE_FOCUSED marked.
2. We're occasionally firing a focus event on the object being tabbed *from*,
which is totally wrong.

This bug is stopping Window-Eyes from working correctly with Mozilla.

Comment 1

[Aaron Leventhal]

Attachment: 1) Make sure STATE_FOCUSED is set on objects focus event indicates are focused, 2) Don't fire events using ExplicitOriginalTarget unless the current target has a binding parent

Comment 2

[Philip K. Warren]

Comment on attachment 150306 [details] [diff] [review]
1) Make sure STATE_FOCUSED is set on objects focus event indicates are focused, 2) Don't fire events using ExplicitOriginalTarget unless the current target has a binding parent

>Index: src/base/nsRootAccessible.cpp
>===================================================================
>+NS_IMETHODIMP nsRootAccessible::GetState(PRUint32 *aState) 
>+{
>+  nsresult rv = NS_ERROR_FAILURE;
>+  if (mDOMNode) {
>+    rv = nsDocAccessibleWrap::GetState(aState);
>+  }
>+  if (NS_FAILED(rv)) {
>+    return rv;
>+  }

Do you mean to return an error here if mDOMNode is not set? Just wondering...

>+  if (gLastFocusedNode) {
>+    nsCOMPtr<nsIDOMDocument> rootAccessibleDoc(do_QueryInterface(mDocument));
>+    nsCOMPtr<nsIDOMDocument> focusedDoc;
>+    gLastFocusedNode->GetOwnerDocument(getter_AddRefs(focusedDoc));
>+    if (rootAccessibleDoc == focusedDoc) {

Is it possible for both rootAccessibleDoc and focusedDoc to be null and you
still set STATE_FOCUSED?

>       gLastFocusedNode = focusNode;
>       NS_ADDREF(gLastFocusedNode);
>+      if (role == ROLE_TEXT) {
>+        printf("Hello");
>+      }

This should be taken out in the final patch.

>-    if (!content || content->IsContentOfType(nsIContent::eHTML)) {
...
>+    if (content && content->IsContentOfType(nsIContent::eHTML) &&

You did mean to switch these around, right?

>+      // The difference between event target types is not 100% clear from 

Incomplete comment.

Comment 3

[Aaron Leventhal]

(In reply to comment #2)
> (From update of attachment 150306 [details] [diff] [review])
> >Index: src/base/nsRootAccessible.cpp
> >===================================================================
> >+NS_IMETHODIMP nsRootAccessible::GetState(PRUint32 *aState) 
> >+{
> >+  nsresult rv = NS_ERROR_FAILURE;
> >+  if (mDOMNode) {
> >+    rv = nsDocAccessibleWrap::GetState(aState);
> >+  }
> >+  if (NS_FAILED(rv)) {
> >+    return rv;
> >+  }
> 
> Do you mean to return an error here if mDOMNode is not set? Just wondering...
Yes I did. When mDOMNode for any kind of nsAccessible/nsAccessNode is nsnull,
then it is no longer a relevant object.

> 
> >+  if (gLastFocusedNode) {
> >+    nsCOMPtr<nsIDOMDocument> rootAccessibleDoc(do_QueryInterface(mDocument));
> >+    nsCOMPtr<nsIDOMDocument> focusedDoc;
> >+    gLastFocusedNode->GetOwnerDocument(getter_AddRefs(focusedDoc));
> >+    if (rootAccessibleDoc == focusedDoc) {
> 
> Is it possible for both rootAccessibleDoc and focusedDoc to be null and you
> still set STATE_FOCUSED?
Not at this point, because rootAccessibleDoc would only be nsnull if mDOMNode
was also nsnull. I'll add an assertion.

> 
> >       gLastFocusedNode = focusNode;
> >       NS_ADDREF(gLastFocusedNode);
> >+      if (role == ROLE_TEXT) {
> >+        printf("Hello");
> >+      }
> 
> This should be taken out in the final patch.
> 
> >-    if (!content || content->IsContentOfType(nsIContent::eHTML)) {
> ...
> >+    if (content && content->IsContentOfType(nsIContent::eHTML) &&
> 
> You did mean to switch these around, right?
Yes

> 
> >+      // The difference between event target types is not 100% clear from 
> 
> Incomplete comment.
> 

Comment 4

[Aaron Leventhal]

Attachment: Fixes pkw's comments and also fixes focused tree items to show STATE_FOCUSABLE

Comment 5

[Brian Ryner (not reading)]

Comment on attachment 150319 [details] [diff] [review]
Fixes pkw's comments and also fixes focused tree items to show STATE_FOCUSABLE

sr=bryner

Comment 6

[Aaron Leventhal]

Checking in src/base/nsAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v  <--  nsAccessible.cpp
new revision: 1.102; previous revision: 1.101
done
Checking in src/base/nsRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v  <-- 
nsRootAccessible.cpp
new revision: 1.92; previous revision: 1.91
done
Checking in src/base/nsRootAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.h,v  <--  nsRootAccessible.h
new revision: 1.40; previous revision: 1.39
done
Checking in src/xul/nsXULFormControlAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULFormControlAccessible.cpp,v  <-- 
nsXULFormControlAccessible.cpp
new revision: 1.36; previous revision: 1.35
done

Comment 7

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381