289491 - Doc loaded events still broken for accessibility

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

[Aaron Leventhal]

There are now problems with firing the MSAA state change events for documents
that are finished loading.

This was working in previous tests. It could be that something has changed. One
thing that seems to be different is that you can no longer get the parent
document from the root content document.

Comment 1

[Aaron Leventhal]

Attachment: 1) Use docshell tree to determine if we're at the root, 2) Listen to state changes for STATE_WINDOW, 3) Use window.parent ==self to show when we're at the top of the content tree

This bug is actually crucial to get in. It blocks the screen reader beta tester
list from continuing work.

Comment 2

[Aaron Leventhal]

Comment on attachment 179995 [details] [diff] [review]
1) Use docshell tree to determine if we're at the root, 2) Listen to state changes for STATE_WINDOW, 3) Use window.parent ==self to show when we're at the top of the content tree

More testing shows me that this is still wrong.

Comment 3

[Aaron Leventhal]

Attachment: 1) Manage progress listening globally from accessibility service instead of attempting to do it from each document, 2) Use docshell busyFlags instead of caching our own info

Biesi, sorry for the previous patch being superfluous. It has been really tough
to get this right.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 180082 [details] [diff] [review]
1) Manage progress listening globally from accessibility service instead of attempting to do it from each document, 2) Use docshell busyFlags instead of caching our own info

nsPIAccessibleDocument.idl needs a new IID

accessible/src/base/nsAccessibilityService.cpp
+  NS_ASSERTION(aStateFlags & STATE_IS_DOCUMENT, "Other notifcations
excluded");

typo in notifications...

so out of curiousity, why did comparing parent not work?

+  docAccessible->FireDocLoadingEvent(!(aStateFlags & STATE_TRANSFERRING));

don't you return right at the beginning of this function if STATE_TRANSFERRING
is set?


Why does OnLocationChange do what you want? Is it because if the load is no
anchor jump, there won't be an accessible doc for it yet?


ok... looks right, I guess. but I don't know the a11y code, really...

Comment 5

[Aaron Leventhal]

(In reply to comment #4)
> (From update of attachment 180082 [details] [diff] [review] [edit])
> nsPIAccessibleDocument.idl needs a new IID
Done.

> so out of curiousity, why did comparing parent not work?
It does work, but I was concerned about whether it would always work, especially
depending on window.parent.noAccess capability pref. I needed the docshell
anyay, and was more comfortable with that working as I expected.

> +  docAccessible->FireDocLoadingEvent(!(aStateFlags & STATE_TRANSFERRING));
> 
> don't you return right at the beginning of this function if STATE_TRANSFERRING
> is set?
No I return if neither STATE_TRANSFERRING nor STATE_STOP are set via:
  if (0 == (aStateFlags & (STATE_TRANSFERRING | STATE_STOP))) { return NS_OK; }

> Why does OnLocationChange do what you want? Is it because if the load is no
> anchor jump, there won't be an accessible doc for it yet?
Right, I've now added a comment to that effect.

Comment 6

[Aaron Leventhal]

Tor, will you still be able to look at this? I need to know if you can't -- it's
definitely a blocker for me.

Comment 7

[tor]

Comment on attachment 180082 [details] [diff] [review]
1) Manage progress listening globally from accessibility service instead of attempting to do it from each document, 2) Use docshell busyFlags instead of caching our own info

> NS_IMETHODIMP nsAccessibilityService::OnStateChange(nsIWebProgress *aWebProgress,
>   nsIRequest *aRequest, PRUint32 aStateFlags, nsresult aStatus)

>   nsCOMPtr<nsIDOMWindow> domWindow;
>   aWebProgress->GetDOMWindow(getter_AddRefs(domWindow));
>-  // Bug 214049 OnStateChange can come from non UI threads.
>-  if (!domWindow)
>-    return NS_OK;
>+  NS_ASSERTION(domWindow, "DOM Window for state change is null");

If this ever happens, you're going to crash shortly afterwards.

>   nsCOMPtr<nsIDOMDocument> domDoc;
>   domWindow->GetDocument(getter_AddRefs(domDoc));


>+  nsCOMPtr<nsIDocShellTreeItem> docShellTreeItem =
>+    nsAccessNode::GetDocShellTreeItemFor(domDocRootNode);

Can this fail?	If so, a crash is immanent.

>+  PRInt32 contentType;
>+  docShellTreeItem->GetItemType(&contentType);


> /* void onLocationChange (in nsIWebProgress aWebProgress, in nsIRequest aRequest, in nsIURI location); */
> NS_IMETHODIMP nsAccessibilityService::OnLocationChange(nsIWebProgress *aWebProgress,
>   nsIRequest *aRequest, nsIURI *location)
> {
>-  NS_NOTREACHED("notification excluded in AddProgressListener(...)");
>-  return NS_OK;
>+  // If the document is already loaded, this will just check to see
>+  // if an anchor jump event needs to be fired.
>+  nsCOMPtr<nsIDOMWindow> domWindow;
>+  aWebProgress->GetDOMWindow(getter_AddRefs(domWindow));
>+  NS_ASSERTION(domWindow, "DOM Window for state change is null");

Crash immanent if it failed, again.

>+  nsCOMPtr<nsIDOMDocument> domDoc;
>+  domWindow->GetDocument(getter_AddRefs(domDoc));

Comment 8

[tor]

Comment on attachment 180082 [details] [diff] [review]
1) Manage progress listening globally from accessibility service instead of attempting to do it from each document, 2) Use docshell busyFlags instead of caching our own info

With biesi's comments addressed and the bulletproofing, sr=tor.

Comment 9

[Aaron Leventhal]

Comment on attachment 180082 [details] [diff] [review]
1) Manage progress listening globally from accessibility service instead of attempting to do it from each document, 2) Use docshell busyFlags instead of caching our own info

I have addressed biesi and tor's comments. This only affects screen reader
users, the code is not loaded otherwise -- but it is a crucial fix for the beta
tester list.

Comment 10

[Aaron Leventhal]

Attachment: Addresses reviewer comments

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 180082 [details] [diff] [review]
1) Manage progress listening globally from accessibility service instead of attempting to do it from each document, 2) Use docshell busyFlags instead of caching our own info

Approving for 1.1a-destined trunk: a=shaver.

Comment 12

[Aaron Leventhal]

Checking in accessible/public/nsPIAccessibleDocument.idl;
/cvsroot/mozilla/accessible/public/nsPIAccessibleDocument.idl,v  <-- 
nsPIAccessibleDocument.idl
new revision: 1.3; previous revision: 1.2
done
Checking in accessible/src/atk/nsDocAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsDocAccessibleWrap.cpp,v  <-- 
nsDocAccessibleWrap.cpp
new revision: 1.17; previous revision: 1.16
done
Checking in accessible/src/atk/nsDocAccessibleWrap.h;
/cvsroot/mozilla/accessible/src/atk/nsDocAccessibleWrap.h,v  <-- 
nsDocAccessibleWrap.h
new revision: 1.5; previous revision: 1.4
done
Checking in accessible/src/base/nsAccessNode.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessNode.cpp,v  <--  nsAccessNode.cpp
new revision: 1.23; previous revision: 1.22
done
Checking in accessible/src/base/nsAccessNode.h;
/cvsroot/mozilla/accessible/src/base/nsAccessNode.h,v  <--  nsAccessNode.h
new revision: 1.21; previous revision: 1.20
done
Checking in accessible/src/base/nsAccessibilityService.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v  <-- 
nsAccessibilityService.cpp
new revision: 1.136; previous revision: 1.135
done
Checking in accessible/src/base/nsDocAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.cpp,v  <--  nsDocAccessible.cpp
new revision: 1.58; previous revision: 1.57
done
Checking in accessible/src/base/nsDocAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.h,v  <--  nsDocAccessible.h
new revision: 1.24; previous revision: 1.23
done
Checking in accessible/src/msaa/nsDocAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsDocAccessibleWrap.cpp,v  <-- 
nsDocAccessibleWrap.cpp
new revision: 1.26; previous revision: 1.25
done
Checking in accessible/src/msaa/nsDocAccessibleWrap.h;
/cvsroot/mozilla/accessible/src/msaa/nsDocAccessibleWrap.h,v  <-- 
nsDocAccessibleWrap.h
new revision: 1.10; previous revision: 1.9
done

Comment 13

[Firefox Bug Husbandry Bot]

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