Doc loaded events still broken for accessibility

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: aaronlev, Assigned: aaronlev)

Tracking

({access, sec508})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

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

This bug is actually crucial to get in. It blocks the screen reader beta tester
list from continuing work.
Attachment #179995 - Flags: superreview?(tor)
Attachment #179995 - Flags: review?(cbiesinger)
Attachment #179995 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 2

14 years ago
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.
Attachment #179995 - Attachment is obsolete: true
Attachment #179995 - Flags: superreview?(tor)
(Assignee)

Comment 3

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

Biesi, sorry for the previous patch being superfluous. It has been really tough
to get this right.
Attachment #180082 - Flags: superreview?(dmose)
Attachment #180082 - Flags: review?(cbiesinger)
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...
Attachment #180082 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 5

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

(Assignee)

Updated

14 years ago
Attachment #180082 - Flags: superreview?(dmose) → superreview?(tor)
(Assignee)

Comment 6

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

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

14 years ago
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.
Attachment #180082 - Flags: superreview?(tor) → superreview+
(Assignee)

Comment 9

14 years ago
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.
Attachment #180082 - Flags: approval-aviary1.1a?
(Assignee)

Comment 10

14 years ago
Created attachment 180841 [details] [diff] [review]
Addresses reviewer comments
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.
Attachment #180082 - Flags: approval-aviary1.1a? → approval-aviary1.1a+
(Assignee)

Comment 12

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