Closed Bug 613146 Opened 14 years ago Closed 14 years ago

amazon.de isn't read by NVDA

Categories

(Core :: Disability Access APIs, defect)

x86
Windows 7
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- final+

People

(Reporter: surkov, Assigned: surkov)

References

()

Details

(Keywords: access)

Attachments

(1 file)

from bug 599814 comment #29:

> I think amazon.de is a different issue. I suspect the document isn't firing a
> documentLoadComplete event
Not quite. It *is* firing the event, but the document still has the busy state
when it gets fired, so NVDA ignores it. Here's the entry from AccProbe's event
monitor:
IA2_EVENT_DOCUMENT_LOAD_COMPLETE Accessible Name: Amazon.de: Günstige Preise
bei Elektronik & Foto, DVD, Musik, Bücher, Games, Sp, Accessible Role:
document, Accessible State: [focusable, readOnly, focused, busy], Event Data:
hwnd=1700236; objectId=-4; childId=-154822576; windowClass=MozillaWindowClass;
I guess the document can have busy state (nsIDocShell::GetBusyFlags() !=
BUSY_FLAGS_NONE) when we fire document load event
(nsIWebProgressListener::OnStateChange with aStateFlags & (STATE_STOP ||
STATE_IS_DOCUMENT)). The busy state points that document is loading something
and this something shouldn't be new document in replace of this one).

I'm cc'ing Boris hoping to get some information what web page can initiate to
get busy flags on docshell after the page loaded. I assume web page starts to
do something on onload.

If my guess is correct then I think NVDA shouldn't look at busy state when it
gets document loaded event, but it should rely on state change events (the
meantime when we fire state busy false then state busy is still presented and
that's our bug).
Severity: normal → major
blocking2.0: --- → ?
OS: Mac OS X → Windows 7
(In reply to comment #1)
> If my guess is correct then I think NVDA shouldn't look at busy state when it
> gets document loaded event
The problem is that we don't render a document at all until the busy state goes away. This is because normally, when a document is busy, it doesn't fire events, so our buffer would be pointless and wouldn't update correctly. If what you're saying is correct, then we should ignore documentLoadComplete altogether and only rely on stateChange, but this makes the documentLoadComplete event somewhat useless to ATs.
Yes let's sort this out for FF4.
blocking2.0: ? → final+
> I'm cc'ing Boris hoping to get some information what web page can initiate to
> get busy flags on docshell after the page loaded. I assume web page starts to
> do something on onload.

Yep, this web page kicks off a whole bunch of new loads in onload (some location.replace stuff, inserting some iframes into the DOM, some flash gunk, etc).
Boris, does iframes insertion on onload set busy state on docshell associated with primary document? Should location.replace result in nsIWebProgressListener::OnStateChange with aStateFlags & STATE_STOP call?
> does iframes insertion on onload set busy state on docshell associated
> with primary document?

I'm not sure; I could check if it really matters....  An image insertion definitely would.

Note that the state _does_ go back to 0 eventually over here when I load the page in question, by the way.
(In reply to comment #6)
> > does iframes insertion on onload set busy state on docshell associated
> > with primary document?
> 
> I'm not sure; I could check if it really matters....  An image insertion
> definitely would.

> Note that the state _does_ go back to 0 eventually over here when I load the
> page in question, by the way.

Yes it is. But the problem is it's not 0 when AT handles document loaded event. Some time after it's 0.

Is that correct in question that the state busy can be set after the page was loaded? Iirc onload triggers before nsIWebProgressListener::OnStateChange(STATE_STOP) and I assume onload processing may turn busy set on and off few times after we handled nsIWebProgressListener::OnStateChange(STATE_STOP). If that's correct then perhaps we can't do any magic to delay a11y document loaded event to make sure there's no busy flag and it won't appear shortly after that.
> Yes it is. But the problem is it's not 0 when AT handles document loaded event.
> Some time after it's 0.

Right.

> Iirc onload triggers before nsIWebProgressListener::OnStateChange(STATE_STOP)

Depends on the progress listener, but generally yes.

> I assume onload processing may turn busy set on and off few times

Correct.

In general, I think the "busy" flag is pretty much useless.  It can change state at any time for random reasons (e.g. the user scrolled the page, so we paint a background, so we resolve background style and start a background image load).
(In reply to comment #8)

> In general, I think the "busy" flag is pretty much useless.  It can change
> state at any time for random reasons (e.g. the user scrolled the page, so we
> paint a background, so we resolve background style and start a background image
> load).

Isn't it a problem if AT gets info from browser while busy flag is set?
Problem in what sense?  Again, the busy flag can be set for any reason whatsoever, as long as any network stuff is live.  Heck, it can be set for the entire lifetime of the page if it includes a video stream or multipart jpeg stream or long-poll XHR or whatever.
(In reply to comment #10)
> Problem in what sense?

Well, I think AT wants to get response on WM_GETOBJECT Windows message, they could make us crawl through DOM tree, work with frames, perhaps make us to flush layout. All this doesn't sound like busy flag may bring any troubles on this way. Sounds right?

If so then I think we could get rid busy state on document accessible (perhaps keep it hardcoded for cases when we fire busy state change events for ATs that don't use document load events). Jamie, do you rely anyhow on busy state except an ignorance of document loaded events?
> Sounds right?

Yes.
(In reply to comment #11)
> If so then I think we could get rid busy state on document accessible (perhaps
> keep it hardcoded for cases when we fire busy state change events for ATs that
> don't use document load events). Jamie, do you rely anyhow on busy state except
> an ignorance of document loaded events?
As I said, we won't load a document at all if it has the busy state and afaik nor do other ATs. We don't specifically ignore documentLoadComplete events because of the busy state, but we refuse to load a busy document, so this is the indirect result. There are several reasons for this:
1. While the document is loading, Gecko a11y doesn't fire events to indicate changes to the document. Therefore, if we render a buffer, it won't get updated as nodes get changed on the page, which is bad if we render it when the page is only half loaded.
2. Once we render a buffer, we start reading the document. Without a useful busy state, we have no way of knowing when the document finishes loading. documentLoadComplete isn't enough; see below.
3. If a user switches to Firefox after a document has finished loading but it is still busy, we need a way of knowing whether the document has finished loading. Otherwise, we don't know whether to read the document or not.
As you say, most ATs use the stateChange event and check for the busy state, but these ATs will also have similar problems because the busy state can't be relied upon to determine whether the document has finished its initial load.
It's worth noting that amazon.de seems to work fine in Firefox 3.6.x.
I feel like I'm missing something...  Whether the document is done loading (in terms of firing onload) is readily checkable by asking the document (document.readyState) or by listening for the load event or the state change notification.  Is the problem that our a11y layer just doesn't expose or pass on that information?
(In reply to comment #15)
> I feel like I'm missing something...  Whether the document is done loading (in
> terms of firing onload) is readily checkable by asking the document
> (document.readyState) or by listening for the load event or the state change
> notification.  Is the problem that our a11y layer just doesn't expose or pass
> on that information?
It passes on the events, but as I noted, they're not reliable because the AT might get to the document after it's finished loading.

My suggestion would be to map the a11y busy state to !document.readyState.
(In reply to comment #13)

> 1. While the document is loading, Gecko a11y doesn't fire events to indicate
> changes to the document. Therefore, if we render a buffer, it won't get updated
> as nodes get changed on the page, which is bad if we render it when the page is
> only half loaded.

btw, Firefox 4 does.

> 2. Once we render a buffer, we start reading the document. Without a useful
> busy state, we have no way of knowing when the document finishes loading.
> documentLoadComplete isn't enough; see below.
> 3. If a user switches to Firefox after a document has finished loading but it
> is still busy, we need a way of knowing whether the document has finished
> loading. Otherwise, we don't know whether to read the document or not.
> As you say, most ATs use the stateChange event and check for the busy state,
> but these ATs will also have similar problems because the busy state can't be
> relied upon to determine whether the document has finished its initial load.

I would suggest to expose busy state until we fired document loaded event, in other words make a11y busy state related with neither docshell busy flags nor document.stateReady. This way we make sure we don't expose it when we fired an event and don't say the document accessible is not busy while we didn't fire document loaded event what makes us consistent.
Attached patch patchSplinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #491817 - Flags: review?(marco.zehe)
Attachment #491817 - Flags: review?(bolterbugz)
Comment on attachment 491817 [details] [diff] [review]
patch

r=me for the tests. I'll give this patch a whirl too after Ivem returned back at home on Saturday.
Attachment #491817 - Flags: review?(marco.zehe) → review+
Comment on attachment 491817 [details] [diff] [review]
patch

r=me if this tests out okay. I think this logic is right. I had some nits with the tests. BTW nice to see this patch when I came in (crossing one thing of my todo list) :)

>diff --git a/accessible/src/base/nsAccDocManager.cpp b/accessible/src/base/nsAccDocManager.cpp
>@@ -209,16 +209,20 @@ nsAccDocManager::OnStateChange(nsIWebPro

>+  // Mark the document accessible as loading, if it stays alive then we'll mark
>+  // it as loaded when we receive proper notification.
>+  docAcc->MarkAsLoading();

Maybe say "proper notification (DOMContentLoaded)".

\
> nsAccDocManager::HandleDOMDocumentLoad(nsIDocument *aDocument,

>+  // Mark the document as loaded to drop off the busy state flag on it.
>+  docAcc->MarkAsLoaded();

>+++ b/accessible/src/base/nsDocAccessible.cpp

>-  if (nsCoreUtils::IsDocumentBusy(mDocument)) {
>+  if (!mIsLoaded) {
>     *aState |= nsIAccessibleStates::STATE_BUSY;

Looks right.

>+++ b/accessible/tests/mochitest/events/docload_wnd.xul

>+    function testStates(aAccOrElmOrID, aState, aExtraState, aAbsentState,
>+                        aAbsentExtraState)
>+    {
>+      gOpenerWnd.testStates(aAccOrElmOrID, aState, aExtraState, aAbsentState,
>+                            aAbsentExtraState);
>+    }

I'd be tempted to name this test testStatesViaWindowOpener or something.

>@@ -86,16 +93,19 @@

>+
>+        testStates(event.accessible, (aIsEnabled ? STATE_BUSY : 0), 0,
>+                   (aIsEnabled ? 0 : STATE_BUSY), 0);

This is clever but at least for tests would this be more readable?

if (aIsEnabled) {
  testStates(event.accessible, STATE_BUSY, 0, 0, 0);
} else {
  testStates(event.accessible, 0, 0, STATE_BUSY, 0);
}

Normally I like clever, but I think this makes casual reading of our tests slightly nicer. (Thoughts?)

Aside: We seem to only call stateBusyChecker with a false arg.
Attachment #491817 - Flags: review?(bolterbugz) → review+
(In reply to comment #19)
> Comment on attachment 491817 [details] [diff] [review]
> patch
> 
> r=me for the tests. I'll give this patch a whirl too after Ivem returned back
> at home on Saturday.

thanks, just now I filed try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/surkov.alexander@gmail.com-f7bb2543d081
(In reply to comment #20)

> Maybe say "proper notification (DOMContentLoaded)".

mostly this is not about DOMContentLoaded

> I'd be tempted to name this test testStatesViaWindowOpener or something.

Why? Would you tempted to name STATE_BUSY as STATE_BUSYViaWindowOpener? :)

> >+        testStates(event.accessible, (aIsEnabled ? STATE_BUSY : 0), 0,
> >+                   (aIsEnabled ? 0 : STATE_BUSY), 0);
> 
> This is clever but at least for tests would this be more readable?
> 
> if (aIsEnabled) {
>   testStates(event.accessible, STATE_BUSY, 0, 0, 0);
> } else {
>   testStates(event.accessible, 0, 0, STATE_BUSY, 0);
> }
> 
> Normally I like clever, but I think this makes casual reading of our tests
> slightly nicer. (Thoughts?)

Well, I think I like short version but I won't insist on my version really hard.

> Aside: We seem to only call stateBusyChecker with a false arg.

right, it seems to me because state_busy true change event can't be delivered in time because document dies, it's timing and hard to write a test.
(In reply to comment #22)
> (In reply to comment #20)
> 
> > Maybe say "proper notification (DOMContentLoaded)".
> 
> mostly this is not about DOMContentLoaded
> 
> > I'd be tempted to name this test testStatesViaWindowOpener or something.
> 
> Why? Would you tempted to name STATE_BUSY as STATE_BUSYViaWindowOpener? :)

I don't see this as the same actually :)  (But I really don't mind)

> 
> > >+        testStates(event.accessible, (aIsEnabled ? STATE_BUSY : 0), 0,
> > >+                   (aIsEnabled ? 0 : STATE_BUSY), 0);
> > 
> > This is clever but at least for tests would this be more readable?
> > 
> > if (aIsEnabled) {
> >   testStates(event.accessible, STATE_BUSY, 0, 0, 0);
> > } else {
> >   testStates(event.accessible, 0, 0, STATE_BUSY, 0);
> > }
> > 
> > Normally I like clever, but I think this makes casual reading of our tests
> > slightly nicer. (Thoughts?)
> 
> Well, I think I like short version but I won't insist on my version really
> hard.

Me neither :)

> 
> > Aside: We seem to only call stateBusyChecker with a false arg.
> 
> right, it seems to me because state_busy true change event can't be delivered
> in time because document dies, it's timing and hard to write a test.

Yeah. Maybe one day we'll get it under test.
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/2867e48dec94
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: