Problems with accessibility API focus events

RESOLVED FIXED

Status

()

Core
Disability Access APIs
P1
major
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

({access, sec508})

Trunk
x86
Windows XP
access, sec508
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

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

Updated

14 years ago
Attachment #150306 - Flags: superreview?(bryner)
Attachment #150306 - Flags: review?(pkw)

Comment 2

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

Comment 3

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

(Assignee)

Updated

14 years ago
Attachment #150306 - Attachment is obsolete: true
Attachment #150306 - Flags: superreview?(bryner)
Attachment #150306 - Flags: review?(pkw)
(Assignee)

Comment 4

14 years ago
Created attachment 150319 [details] [diff] [review]
Fixes pkw's comments and also fixes focused tree items to show STATE_FOCUSABLE
(Assignee)

Updated

14 years ago
Attachment #150319 - Flags: superreview?(bryner)
Attachment #150319 - Flags: review?(pkw)

Updated

14 years ago
Attachment #150319 - Flags: review?(pkw) → review+
(Assignee)

Updated

14 years ago
Priority: -- → P1
Comment on attachment 150319 [details] [diff] [review]
Fixes pkw's comments and also fixes focused tree items to show STATE_FOCUSABLE

sr=bryner
Attachment #150319 - Flags: superreview?(bryner) → superreview+
(Assignee)

Comment 6

14 years ago
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
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.