Closed
Bug 289491
Opened 19 years ago
Closed 19 years ago
Doc loaded events still broken for accessibility
Categories
(Firefox :: Disability Access, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: aaronlev, Assigned: aaronlev)
Details
(Keywords: access)
Attachments
(2 files, 1 obsolete file)
34.18 KB,
patch
|
Biesinger
:
review+
tor
:
superreview+
shaver
:
approval-aviary1.1a1+
|
Details | Diff | Splinter Review |
35.49 KB,
patch
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
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)
Updated•19 years ago
|
Attachment #179995 -
Flags: review?(cbiesinger) → review+
Assignee | ||
Comment 2•19 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•19 years ago
|
||
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 4•19 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 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•19 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•19 years ago
|
Attachment #180082 -
Flags: superreview?(dmose) → superreview?(tor)
Assignee | ||
Comment 6•19 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 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 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•19 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•19 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 Approving for 1.1a-destined trunk: a=shaver.
Attachment #180082 -
Flags: approval-aviary1.1a? → approval-aviary1.1a+
Assignee | ||
Comment 12•19 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
Closed: 19 years ago
Resolution: --- → FIXED
Comment 13•5 years ago
|
||
Keywords: sec508
You need to log in
before you can comment on or make changes to this bug.
Description
•