658185 - Y!Mail: iframe containing actual mail message has a non-clearing busy state

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

STR:
1. Open the URL above.
2. Login using username neo_access_us2, password: testing
3. Press the "m" key to go to the Inbox. (Note, if the virtual cursor is on, you'll need to bypass it before pressing "m".)
4. When the Inbox is loaded, the first message in the Inbox folder will have focus and the virtual cursor will be off.
5. Navigate the Inbox using the up/down arrow keys to move through the rows in the table. As you press the up/down arrow keys, the next/previous row will be focused. The screen reader should read the contents of each column from left to right.
6. Open a message by pressing the Enter key. The message will open and the message header will have focus. The virtual cursor should automatically toggle on.

Actual: Since Firefox 4, the virtual cursor does not turn back on as expected, since the document inside the iframe where the message is being loaded into does not clear its busy state after the message finishes loading.

MarcoZ also reproduced this using the 6.0a1pre nightly builds.

In Firefox 3.6, this works as expected.

Comment 1

[James Teh [:Jamie]]

I'm not sure whether this is related, but we also get a non-clearing busy state on the "Untrusted Connection" document. To reproduce:
1. Open https://63.245.217.60/
Expected: The resultant "Untrusted Connection" document should not have the busy state.
Actual: The document has the busy state and it never disappears.
This works as expected in Firefox 3.6.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> I'm not sure whether this is related, but we also get a non-clearing busy
> state on the "Untrusted Connection" document. To reproduce:
> 1. Open https://63.245.217.60/
> Expected: The resultant "Untrusted Connection" document should not have the
> busy state.
> Actual: The document has the busy state and it never disappears.

It must different issue. 

Boris, is "untrusted connection" document isn't a subject of webprogress notfications nor pageshow events, i.e. similar to error pages?

Comment 3

[alexander :surkov (:asurkov)]

Marco, I follow your steps and I don't see any iframes for messages, they are located in the same document. The hierarchy is:
application (the tab document, no busy state)
  propertypage
    table
      cell (left panel, folders and etc)
      cell (right panel)
        section (here's a message)

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to comment #3)
> Marco, I follow your steps and I don't see any iframes for messages, they
> are located in the same document. The hierarchy is:
> application (the tab document, no busy state)
>   propertypage
>     table
>       cell (left panel, folders and etc)
>       cell (right panel)
>         section (here's a message)

When walking up from the place the focus lands using NVDA object navigation, I see a couple of sections, then come to the busy document, then to the iframe parent, then to the cell you mention.
So after pressing ENTER on a message, i land on the first item in that message pane that has text, walk up to the parents and find it.

Comment 5

[alexander :surkov (:asurkov)]

How do you do that? Obviously it's not accessible tree hierarchy. Do you do that by NVDA? If so then I'd need feedback from Jamie how they do that.

Comment 6

[Marco Zehe (:MarcoZ)]

The message i was viewing was the one titled "Here's your new car". The frame, and the document that has the busy state both have an accessible name of "Here's your next car".
Once the message opens and focus is transferred over:
1. Press NUMPAD0+NUMPAD5, and NVDA will say "Message header section".
2. Now press NUMPAD0+NUMPAD8. NVDA will read everything from the message and say "section" at the end.
3. Press NUMPAD0+NUMPAD8 again. You'll now land on either a section or the busy document with the accessible name of "Here's your new car".

Comment 7

[Boris Zbarsky [:bzbarsky]]

> i.e. similar to error pages?

It's an error page, period.  ;)

Comment 8

[alexander :surkov (:asurkov)]

filed bug 658737 for untrusted connection page, unccing Boris from this one.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> Marco, I follow your steps and I don't see any iframes for messages, they
> are located in the same document. The hierarchy is:
> application (the tab document, no busy state)
>   propertypage
>     table
>       cell (left panel, folders and etc)
>       cell (right panel)
>         section (here's a message)

I don't know why but now I see iframe and document (busy) in hierarchy. So I can reproduce.

Comment 10

[alexander :surkov (:asurkov)]

Guys, I don't know what happens here but sometimes the message isn't contained inside iframe. It appears Yahoo can generate different DOM for the same thing.

Comment 11

[James Teh [:Jamie]]

Attachment: Test case.

Alright. I've managed to narrow this down into a simple test case.

Steps to repro:
1. Open the attached test case.
2. Press the "Show hidden iframe" button.
3. Press tab.
Expected: The focused document should not have the busy state.
Actual: The focused document has the busy state.

It seems that if you create an iframe inside a hidden node, the document inside the iframe will be busy once the node is shown.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)

> It seems that if you create an iframe inside a hidden node, the document
> inside the iframe will be busy once the node is shown.

True. Thanks, Jamie!

Comment 13

[alexander :surkov (:asurkov)]

Boris, we use nsIWebProgressListener::OnStateChange(STATE_STOP) or "DOMContentLoaded" for error page to mark the document accessible as loaded. If these notifications were missed then how can I detect whether this document was loaded or not (in means of OnStateChange or DOMContentLoaded)?

(From screen reader point of view the loaded document means its content was loaded, parsed, rendered within its subframes. So OnStateChange() may be change on something else if needed.)

Comment 14

[Marco Zehe (:MarcoZ)]

This testcase is definitely confirmed to repro the problem here, too, thanks jamie!

Comment 15

[Boris Zbarsky [:bzbarsky]]

> how can I detect whether this document was loaded or not

document.readyState ?

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 17

[Marco Zehe (:MarcoZ)]

Comment on attachment 535363 [details] [diff] [review]
patch

This patch breaks the initial application mode in NVDA after logging in. In other words, after you hit Login and the inbox and other stuff loads, NVDA's virtual cursor comes on with this patch when it definitely shouldn't! So for some reason we force NVDA into virtual mode even though the body says role="application". r- for that.

Other than that, I found a few nits in the test comments:

>+      // The document shoudn't have busy state (the DOM document was loaded

Typo: shouldn't...

>+      // before its accessible was created). Do this test lately to make sure
>+      // the content of document accessible was created initially, prior this

Nit: "... prior to this ..."

>+      // the document accessible keeps busy state. The initial creation happens
>+      // asynchroniosly after document creation, there are no events we could

Typo: asynchronously

>+      // use to catch it.

Comment 18

[Marco Zehe (:MarcoZ)]

Comment on attachment 535363 [details] [diff] [review]
patch

Sorry for the noise and the false alarm regarding the application role. I didn't see this happening in the test case, so I went back to check with a regular nightly, and it turns out we're making NVDA turn off application mode there as well, even though the body element has role="application" set.

Changing this to an r+ then since it definitely fixes the bug. But please address my comment nits anyway!

Comment 19

[Marco Zehe (:MarcoZ)]

As a follow-up: We don't seem to be broken at all. It's just that the newest development throws one into a What's New area which actually is supposed to be a document with virtual cursor on. So nothing wrong here except that I'm tired and need a break (or another coffee). Sorry for the spam!

Comment 20

[Trevor Saunders (:tbsaunde)]

Comment on attachment 535363 [details] [diff] [review]
patch

>diff --git a/accessible/src/base/nsDocAccessible.cpp b/accessible/src/base/nsDocAccessible.cpp
>--- a/accessible/src/base/nsDocAccessible.cpp
>+++ b/accessible/src/base/nsDocAccessible.cpp
>@@ -602,16 +602,22 @@ nsDocAccessible::Init()
>   NS_LOG_ACCDOCCREATE_FOR("document initialize", mDocument, this)
> 
>   // Initialize notification controller.
>   nsCOMPtr<nsIPresShell> shell(GetPresShell());
>   mNotificationController = new NotificationController(this, shell);
>   if (!mNotificationController)
>     return PR_FALSE;

new is infalible now right? so lets get rid of that check if you don't mind.

>+  // Mark the document accessible as loaded if its DOM document was loaded at
>+  // this point (this can happen because a11y is started late or DOM document
>+  // having no container was loaded.
>+  if (mDocument->GetReadyStateEnum() == nsIDocument::READYSTATE_COMPLETE)
>+    mIsLoaded = PR_TRUE;

what about nsIDocument::READYSTATE_INTERACTIVE ?  should such a document be considered loaded by an AT?

otherwise looks good on the non-tests side.

Comment 21

[Trevor Saunders (:tbsaunde)]

Comment on attachment 535363 [details] [diff] [review]
patch

I looked into the READYSTATE_INTERACTIVE issue, and it looks like we want to consider documents in that state loading not loaded so r=me

Comment 22

[James Teh [:Jamie]]

(In reply to comment #19)
> We don't seem to be broken at all. It's just that the newest
> development throws one into a What's New area which actually is supposed to
> be a document with virtual cursor on.
Actually, nothing changed on the Yahoo! side. This is due to the fix for bug 653607 and is how it was supposed to work always. :)

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #20)

> >   // Initialize notification controller.
> >   nsCOMPtr<nsIPresShell> shell(GetPresShell());
> >   mNotificationController = new NotificationController(this, shell);
> >   if (!mNotificationController)
> >     return PR_FALSE;
> 
> new is infalible now right? so lets get rid of that check if you don't mind.

I didn't hear that. How does it happen? Anyway we have number of these checks so and we should deal with that separately.

Comment 24

[alexander :surkov (:asurkov)]

landed - http://hg.mozilla.org/mozilla-central/rev/5f991615ac6a

Is there a chance to get this into fx6? This one is quite important for Yahoo mail accessibility.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> I didn't hear that. 

Um... do you read mozilla.dev.platform?  If not, you should.  ;)  This has been the case for months now.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> > I didn't hear that. 
> 
> Um... do you read mozilla.dev.platform?  If not, you should.  ;)  This has
> been the case for months now.

I missed that. This is this, right? http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/8b97ca1a8858e78f

Comment 27

[Boris Zbarsky [:bzbarsky]]

Yep.

Comment 28

[Dão Gottwald [:dao]]

This bug has been blamed for two perf regressions:

Regression :( Tp4 increase 2.23% on XP Firefox
----------------------------------------------
    Previous: avg 333.071 stddev 2.494 of 30 runs up to revision 7a6804f6034e
    New     : avg 340.487 stddev 1.101 of 5 runs since revision 5f991615ac6a
    Change  : +7.415 (2.23% / z=2.974)
    Graph   : http://mzl.la/iBp70j

Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7a6804f6034e&tochange=5f991615ac6a

Regression :( Ts, Paint increase 3.54% on XP Firefox
----------------------------------------------------
    Previous: avg 384.195 stddev 3.560 of 30 runs up to revision 7a6804f6034e
    New     : avg 397.779 stddev 2.445 of 5 runs since revision 5f991615ac6a
    Change  : +13.584 (3.54% / z=3.816)
    Graph   : http://mzl.la/kN9Z28

Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7a6804f6034e&tochange=5f991615ac6a

Comment 29

[alexander :surkov (:asurkov)]

All this changeset changes is it adds a call of nsIDocument::GetReadyStateEnum() (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#7697). This call can't make the Init() heavier. Also I bet there's no many document accessibles objects (no more than DOM documents) and thus Init() can't be called too much (once per document accessible instance). Any hint/idea how can I proceed with regression?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Since this changeset is also being blamed for Dromaeo(v8), my suspicion is that the regression range finder just got confused and misblamed some things that were due to PGO turnoff...

The way to test that if desired is to back out this change and verify that there is no perf difference, then land it again.

Comment 31

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110527 Firefox/7.0a1

Comment 32

[Marco Zehe (:MarcoZ)]

Did anyone follow-up on the perf regression pointed out in comment #28 ? Did this get cleared up in another way? Because as Surkov says in comment #29, I agree that the change we made could not have affected that particular Talos perf test.

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #30)
> Since this changeset is also being blamed for Dromaeo(v8), my suspicion is
> that the regression range finder just got confused and misblamed some things
> that were due to PGO turnoff...

Agreed. Accessibility isn't even instantiated for any of these talos tests. Please treat as bogus.

Comment 34

[Marco Zehe (:MarcoZ)]

Comment on attachment 535363 [details] [diff] [review]
patch

Requesting landing this on Aurora even though this was committed to Central after the merge because:
1. It is dependent on an important Yahoo! Mail accessibility bug and the only real issue that is holding them back, and quite severely so. In other words if this bug is not fixed, users will not be able to read mail with NVDA or other screen readers that rely on our busy state being correct for these kinds of iframes. All screen readers on Windows do this, so they're all affected.
2. In addition, Yahoo! Mail is going to be the most accessible webmail client (much more accessible than Gmail) and thus is a very good real-life usecase for all modern kinds of accessibility techniques.
3. The patch has tests and is low risk.
4. The Talos blame is bogus, see comment #30 and comment #33.

Comment 35

[Marco Zehe (:MarcoZ)]

Pushed to Mozilla-Aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/eb32b791af00

Comment 36

[Marco Zehe (:MarcoZ)]

Verified fixed in Aurora build: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a2) Gecko/20110616 Firefox/6.0a2