288503 - Screen reader problems in pages with framesets

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

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.

Comment 1

[Aaron Leventhal]

Attachment: 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.

Comment 2

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

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...)

Comment 3

[Aaron Leventhal]

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 :(

Comment 4

[Aaron Leventhal]

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?

Comment 5

[Aaron Leventhal]

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

Comment 6

[Doug Turner (:dougt)]

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();

Comment 7

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

(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 :-)

Comment 8

[Aaron Leventhal]

Attachment: 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 *'

Comment 9

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

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?)

Comment 10

[Aaron Leventhal]

> 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

[Doug Turner (:dougt)]

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)!

Comment 12

[Aaron Leventhal]

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

Comment 13

[Firefox Bug Husbandry Bot]

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