If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Screen reader problems in pages with framesets

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
13 years ago
13 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

13 years ago
Pages with framesets are being reported by a screen reader as missing frames.
That is because Firefox is telling the screen reader to grab the web page as
soon as the frameset document itself load, as opposed to when all of the
documents in the docshell tree are finished loading.

Firefox should fire the events for a finished page when the entire frameset has
been loaded.
(Assignee)

Comment 1

13 years ago
Created attachment 179305 [details] [diff] [review]
1) Wait until all frames loaded to fire state change event that indicates we're loaded, 2) Move generally useful getters into nsAccessNode as static already_AddRefed() methods

My wife is due with child any day now, please help review this quickly if you
can so I can get it into the tree for screen reader testing.

The important changes are in nsDocAccessible and nsDocAccessibleWrap

The other stuff pretty much is just code cleanup and moving getters into
nsAccessNode.
Attachment #179305 - Flags: superreview?(dougt)
Attachment #179305 - Flags: review?(cbiesinger)
Comment on attachment 179305 [details] [diff] [review]
1) Wait until all frames loaded to fire state change event that indicates we're loaded, 2) Move generally useful getters into nsAccessNode as static already_AddRefed() methods

+PRBool nsDocAccessibleWrap::IsCompletelyLoaded(nsIDocShellTreeItem
*aDocShellTreeItem)

So... why can't this use STATE_IS_DOCUMENT
(http://lxr.mozilla.org/seamonkey/source/uriloader/base/nsIWebProgressListener.
idl#122)? (or STATE_IS_WINDOW as needed...)
(Assignee)

Comment 3

13 years ago
Wow, I'll definitely try that. When I last had looked at that idl there were no
docs. When I asked on IRC this week you weren't there, and it sounded like I had
to do this manually from the responses I got :(
(Assignee)

Comment 4

13 years ago
Biesi, as it turns out I think my patch is necessary.

In the current trunk code (without this patch), I do use STATE_IS_DOCUMENT (see
the assertion in the code). Even so, I get a lot more than just one STATE_STOP
-- perhaps there is 1 per frame and 1 for the frameset, I'm not sure. Anyway, if
I use any of those STATE_STOP's to fire the event that lets the accessible tree
get built, it only sees some of the frames. STATE_IS_WINDOW doesn't help. How am
I supposed to know if it's the STATE_STOP which finally means we're done?
(Assignee)

Comment 5

13 years ago
Actually I think I figured out the problem. Will post new patch when I can.

Comment 6

13 years ago
Comment on attachment 179305 [details] [diff] [review]
1) Wait until all frames loaded to fire state change event that indicates we're loaded, 2) Move generally useful getters into nsAccessNode as static already_AddRefed() methods

although it is a matter of style, most people use a for loop not a while in
this case:

while (-- childCount >= 0) {


Move:  "+    nsCOMPtr<nsIDocShellTreeItem> childTreeItem;" outside of the loop.


can a child of a docShellTreeNode ever be null?  If so, IsCompletelyLoaded
needs to protect against a null aDocShellTreeItem.


Style nit again:

+  if (loadEventDocAccessible) {
+    nsCOMPtr<nsPIAccessible> privateAccessible =
+      do_QueryInterface(loadEventDocAccessible);
+    nsCOMPtr<nsIAccessible> loadEventAccesible  =
+      do_QueryInterface(loadEventDocAccessible);  

above this change you have long lines, i would not do a return after the equal.
 There are a couple of these cases.  


What there a reason you had 4ms before:
-      mDocLoadTimer->InitWithFuncCallback(DocLoadCallback, this, 4,
+      mDocLoadTimer->InitWithFuncCallback(DocLoadCallback, this, 0,

Why the .get()?

-  GetDocAccessibleFor(mWeakShell, aDocAccessible);
+  *aDocAccessible = GetDocAccessibleFor(mWeakShell).get();
(In reply to comment #3)
> Wow, I'll definitely try that. When I last had looked at that idl there were no
> docs.

Yes, thank darin for the docs :-)
(Assignee)

Updated

13 years ago
Attachment #179305 - Flags: superreview?(dougt)
Attachment #179305 - Flags: review?(cbiesinger)
(Assignee)

Comment 8

13 years ago
Created attachment 179495 [details] [diff] [review]
Much simpler now that I check dom window in OnStateChange() to make sure it is the top content window

Doug, the following .get() is necessary otherwise I get a compile error:
*aDocAccessible = GetDocAccessibleFor(mWeakShell).get();
c:/moz\mozilla\accessible\src\base\nsAccessNode.cpp(297) : error C2440: '=' :
cannot convert from 'already_AddRefed<T>' to 'nsIAccessibleDocument *'
Attachment #179305 - Attachment is obsolete: true
Attachment #179495 - Flags: superreview?(dougt)
Attachment #179495 - Flags: review?(cbiesinger)
Comment on attachment 179495 [details] [diff] [review]
Much simpler now that I check dom window in OnStateChange() to make sure it is the top content window

accessible/src/base/nsAccessNode.cpp
+nsAccessNode::GetDocAccessibleFor(nsISupports *aContainer)
+{
+  nsCOMPtr<nsIDocShell> docShell(do_QueryInterface(aContainer));
+  nsCOMPtr<nsIPresShell> presShell;
+  docShell->GetPresShell(getter_AddRefs(presShell));

Can aContainer be a non-docshell? If not, I think an assertion that docShell is
not null would be nice here...

accessible/src/base/nsDocAccessible.cpp
-      mDocLoadTimer->InitWithFuncCallback(DocLoadCallback, this, 4,
-					   nsITimer::TYPE_ONE_SHOT);
-    }
+    FireDocLoadFinished();

Why was the timer needed? (Ie., did something change that it is no longer
needed?)
Attachment #179495 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 10

13 years ago
> Can aContainer be a non-docshell? If not, I think an assertion that docShell
> is not null would be nice here...
Will add that.

> accessible/src/base/nsDocAccessible.cpp
> -      mDocLoadTimer->InitWithFuncCallback(DocLoadCallback, this, 4,
> -					   nsITimer::TYPE_ONE_SHOT);
> -    }
> +    FireDocLoadFinished();
> 
> Why was the timer needed? (Ie., did something change that it is no longer
> needed?)
I think the timer was a fix for several problems -- this frameset problem and
some order of destruction problems that we no longer have. Ijust didn't
understand nsIWebProgress very well in the past, and the timer was a hack that I
could get away with to solve the problem. After a bunch of testing with this new
code, it doesn't appear that we need the timer anymore. Next time I employ a
hack like that I'll add better comments.

Comment 11

13 years ago
Comment on attachment 179495 [details] [diff] [review]
Much simpler now that I check dom window in OnStateChange() to make sure it is the top content window

looks good.

congrats on being a dad (again)!
Attachment #179495 - Flags: superreview?(dougt) → superreview+
(Assignee)

Comment 12

13 years ago
Checking in accessible/src/base/nsAccessNode.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessNode.cpp,v  <--  nsAccessNode.cpp
new revision: 1.22; previous revision: 1.21
done
Checking in accessible/src/base/nsAccessNode.h;
/cvsroot/mozilla/accessible/src/base/nsAccessNode.h,v  <--  nsAccessNode.h
new revision: 1.20; previous revision: 1.19
done
Checking in accessible/src/base/nsAccessibilityService.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v  <-- 
nsAccessibilityService.cpp
new revision: 1.135; previous revision: 1.134
done
Checking in accessible/src/base/nsCaretAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsCaretAccessible.cpp,v  <-- 
nsCaretAccessible.cpp
new revision: 1.29; previous revision: 1.28
done
Checking in accessible/src/base/nsCaretAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsCaretAccessible.h,v  <--  nsCaretAccessible.h
new revision: 1.9; previous revision: 1.8
done
Checking in accessible/src/base/nsDocAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.cpp,v  <--  nsDocAccessible.cpp
new revision: 1.56; previous revision: 1.55
done
Checking in accessible/src/base/nsDocAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.h,v  <--  nsDocAccessible.h
new revision: 1.23; previous revision: 1.22
done
Checking in accessible/src/base/nsRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v  <-- 
nsRootAccessible.cpp
new revision: 1.114; previous revision: 1.113
done
Checking in accessible/src/base/nsRootAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.h,v  <--  nsRootAccessible.h
new revision: 1.47; previous revision: 1.46
done
Checking in accessible/src/html/Makefile.in;
/cvsroot/mozilla/accessible/src/html/Makefile.in,v  <--  Makefile.in
new revision: 1.20; previous revision: 1.19
done
Checking in accessible/src/mac/Makefile.in;
/cvsroot/mozilla/accessible/src/mac/Makefile.in,v  <--  Makefile.in
new revision: 1.13; previous revision: 1.12
done
Checking in accessible/src/msaa/nsDocAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsDocAccessibleWrap.cpp,v  <-- 
nsDocAccessibleWrap.cpp
new revision: 1.25; previous revision: 1.24
done
Checking in accessible/src/msaa/nsDocAccessibleWrap.h;
/cvsroot/mozilla/accessible/src/msaa/nsDocAccessibleWrap.h,v  <-- 
nsDocAccessibleWrap.h
new revision: 1.9; previous revision: 1.8
done
Checking in accessible/src/xul/Makefile.in;
/cvsroot/mozilla/accessible/src/xul/Makefile.in,v  <--  Makefile.in
new revision: 1.17; previous revision: 1.16
done
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.