267190 - [MSAA] Fire events for jump to named anchor

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

We need to fire some kind of accessibilty event when a user jumps within the
current page to a location within. This allows a screen reader or other AT to
update it's internal user location pointers.

Comment 1

[Aaron Leventhal]

The scrolling event issue has been moved to bug 267387.

The EVENT_SELECTION_WITHIN event has been chosen for named anchor jumps.

Comment 2

[Aaron Leventhal]

Attachment: On nsIWebProgress listener location change, if staying in same doc, fire EVENT_SELECTION_WITHIN on new position in doc

Comment 3

[Philip K. Warren]

Comment on attachment 164326 [details] [diff] [review]
On nsIWebProgress listener location change, if staying in same doc, fire EVENT_SELECTION_WITHIN on new position in doc

>Index: nsDocAccessibleWrap.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/msaa/nsDocAccessibleWrap.cpp,v
>retrieving revision 1.18
>diff -u -r1.18 nsDocAccessibleWrap.cpp
>--- nsDocAccessibleWrap.cpp	2 Aug 2004 12:32:58 -0000	1.18
>+++ nsDocAccessibleWrap.cpp	2 Nov 2004 23:20:48 -0000
>@@ -267,6 +273,83 @@
>+        PRInt32 numChildren;
>+        accessible->GetChildCount(&numChildren);
>+        if (!numChildren) {
>+          return accessible;  // It's a leaf accessible, return it
>+        }

Looking at the GetChildCount code, it looks like it can return -1 in some
cases. I don't know the chances of this happening, but you may want to change
this check to be:

if (numChildren == 0)

Also are you potentially leaking the "accessible" variable in
GetFirstLeafAccessible? What happens if accessible is non-null, but either has
STATE_INVISIBLE or has children?

Comment 4

[Aaron Leventhal]

Attachment: Address pkwarren's comments

Comment 5

[Aaron Leventhal]

Attachment: New patch that also fires EVENT_SELECTION_WITHIN when user jumps to URL w/ anchor or clears anchor

Comment 6

[Aaron Leventhal]

Comment on attachment 164749 [details] [diff] [review]
New patch that also fires EVENT_SELECTION_WITHIN when user jumps to URL w/ anchor or clears anchor

New patch. Seeking another review for the same bug.

Comment 7

[Philip K. Warren]

Comment on attachment 164749 [details] [diff] [review]
New patch that also fires EVENT_SELECTION_WITHIN when user jumps to URL w/ anchor or clears anchor

>+      // Instantiate walker lazily since we won't need it in 90% of the cases
>+      // where the first DOM node we're given provides an accessible
>+      nsCOMPtr<nsIDOMDocumentTraversal> trav = do_QueryInterface(mDocument);
>+      NS_ASSERTION(trav, "No DOM document traversal for document");
>+      trav->CreateTreeWalker(mDOMNode, 
>+                            nsIDOMNodeFilter::SHOW_ELEMENT | nsIDOMNodeFilter::SHOW_TEXT,
>+                            nsnull, PR_FALSE, getter_AddRefs(walker));

Do you really want just an assertion here, if trav could really be null?

>+  if (!mWasAnchor && !hasAnchor) {
>+    return;
>+  }
>+  mWasAnchor = hasAnchor;

You described the purpose of mWasAnchor to me, but it might be good to add a
small comment here.

>+  if (mIsNewDocument) {
>+    mIsNewDocument = PR_FALSE;
>+
>+    if (mBusy != eBusyStateDone) {
>+      mBusy = eBusyStateDone; // before event callback so STATE_BUSY is not reported
>+      FireToolkitEvent(nsIAccessibleEvent::EVENT_STATE_CHANGE, this, nsnull);
>+      FireAnchorJumpEvent();
>+    }
>+  }
>+
>+  mBusy = eBusyStateDone;

Nit: setting mBusy twice.

Comment 8

[Aaron Leventhal]

I'll add a comment for mWasAnchor.

The mBusyState has to be set twice. I fixed that in earlier bug. If we don't set
it before we fire the state change event, then the AT gets the wrong STATE_BUSY
bit when it checks.

I think an assertion is fine on the QI since we know it's an HTML doc.

Comment 9

[Aaron Leventhal]

Checking in src/base/nsAccessibilityService.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v  <-- 
nsAccessibilityService.cpp
new revision: 1.120; previous revision: 1.119
done
Checking in src/base/nsAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v  <--  nsAccessible.cpp
new revision: 1.119; previous revision: 1.118
done
Checking in src/base/nsDocAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.cpp,v  <--  nsDocAccessible.cpp
new revision: 1.48; previous revision: 1.47
done
Checking in src/base/nsDocAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.h,v  <--  nsDocAccessible.h
new revision: 1.17; previous revision: 1.16
done
Checking in src/base/nsRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v  <-- 
nsRootAccessible.cpp
new revision: 1.103; previous revision: 1.102
done
Checking in src/msaa/nsDocAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsDocAccessibleWrap.cpp,v  <-- 
nsDocAccessibleWrap.cpp
new revision: 1.20; previous revision: 1.19
done
Checking in src/msaa/nsDocAccessibleWrap.h;
/cvsroot/mozilla/accessible/src/msaa/nsDocAccessibleWrap.h,v  <-- 
nsDocAccessibleWrap.h
new revision: 1.6; previous revision: 1.5
done