754165 - fire document load events on iframes too

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

we were asked by AT vendor to do that. So let's patch and see if we get any negative effect from this.

Comment 1

[alexander :surkov (:asurkov)]

Marco can you test different screen readers and see if this affects them https://tbpl.mozilla.org/?tree=Try&rev=d823fa033eae

Comment 2

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #1)
> Marco can you test different screen readers and see if this affects them
> https://tbpl.mozilla.org/?tree=Try&rev=d823fa033eae

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d823fa033eae/

Comment 3

[Marco Zehe (:MarcoZ)]

This has the effect that NVDA starts allowing access to a page much later than previously. A simple test I did:

1. Start this try-server build. about:home is set as my default homepage.
Result: NVDA is in focus mode, because about:home automatically sets focus to the search textbox.
2. Press CTRL+L and enter http://www.marcozehe.de. My blog has a few iFrames to load on the front page because you can then easily share the posts from there.
Result: In a regular build, even before all the iframes were loaded, the main document became accessible, and NVDA switched into browse mode.
In this try build, NVDA waits until the very very last of the iframes has finished loading and only then allows access to its virtual cursor by switching to browse mode.

The user experience is that it feels much slower. Users should not have to wait for every single iframe to finish loading before the main document content becomes available to browse.

Comment 4

[alexander :surkov (:asurkov)]

I think we can ask Jamie to fix that if NVDA is unique case. What about other screen readers?

Comment 5

[Marco Zehe (:MarcoZ)]

The others wait until everything is loaded and start speaking their page summaries. I tested JAWS and Window-Eyes.

Comment 6

[Marco Zehe (:MarcoZ)]

What I meant to say was that they are waiting for the last iframe to be done loading anyway. So there is no difference.

Comment 7

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #4)
> I think we can ask Jamie to fix that if NVDA is unique case.
I will investigate this in the coming days, but thinking about it, I don't see how NVDA could be at fault here. This suggests that Firefox is no longer firing an event that it used to fire (either focus or document load on the main document). Additional events shouldn't hurt us, only missing ones. One possibility is that document load on the main document is being delayed to wait for document load on the others, but this change doesn't make sense to me. It's a huge backwards compatibility break.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #7)
> (In reply to alexander :surkov from comment #4)
> > I think we can ask Jamie to fix that if NVDA is unique case.
> I will investigate this in the coming days, but thinking about it,

thank you, Jamie. Honestly I'm surprised of this effect of the patch.

>  I don't
> see how NVDA could be at fault here. This suggests that Firefox is no longer
> firing an event that it used to fire (either focus or document load on the
> main document).

it shouldn't be a case.

> Additional events shouldn't hurt us, only missing ones. One
> possibility is that document load on the main document is being delayed to
> wait for document load on the others, but this change doesn't make sense to
> me. 

that's right, we do this now and the patch didn't change this.

>It's a huge backwards compatibility break.

Yes. We can introduce some compatibility code to handle that.

Comment 9

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #7)
> One possibility is that document load on the main document is being delayed to
> wait for document load on the others, but this change doesn't make sense to
> me. It's a huge backwards compatibility break.
Please disregard this; I had a brain fail. Document load just shouldn't affect NVDA at all after Gecko 2 because we always render documents as soon as they get focus.

Comment 10

[James Teh [:Jamie]]

I can't reproduce Marco's observation with this test case:
http://nvda.sourceforge.net/webTests/busy.html
(I prefer this because you can predict when the iframe will finish loading.)
For me, the main document renders straight away and the iframe content renders when it finishes loading 10 seconds later.

Marco, with your test case, what does NVDA say when you press NVDA+tab while it is loading before it switches to browse mode?

Comment 11

[Marco Zehe (:MarcoZ)]

(In reply to James Teh [:Jamie] from comment #10)
> I can't reproduce Marco's observation with this test case:
> http://nvda.sourceforge.net/webTests/busy.html
> (I prefer this because you can predict when the iframe will finish loading.)
> For me, the main document renders straight away and the iframe content
> renders when it finishes loading 10 seconds later.

Yes, I'm observing that with your testcase, too.

> Marco, with your test case, what does NVDA say when you press NVDA+tab while
> it is loading before it switches to browse mode?

Nothing, it basically becomes unresponsive until all iframes have finished loading. Then, it executes the NVDA+Tab and tells me the document accessible has focus.

I have the WebVisum extension installed, maybe that makes a difference.

Comment 12

[James Teh [:Jamie]]

(In reply to Marco Zehe (:MarcoZ) from comment #11)
> > Marco, with your test case, what does NVDA say when you press NVDA+tab while
> > it is loading before it switches to browse mode?
> Nothing, it basically becomes unresponsive until all iframes have finished
> loading.
I did observe nasty responsiveness problems while loading marcozehe.de with that try build, but wasn't sure if it was just me. Alex, could your patch cause severely degraded performance/freezing (as opposed to missing events or the like)?

Comment 13

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #12)
> (In reply to Marco Zehe (:MarcoZ) from comment #11)
> > > Marco, with your test case, what does NVDA say when you press NVDA+tab while
> > > it is loading before it switches to browse mode?
> > Nothing, it basically becomes unresponsive until all iframes have finished
> > loading.
> I did observe nasty responsiveness problems while loading marcozehe.de with
> that try build, but wasn't sure if it was just me. Alex, could your patch
> cause severely degraded performance/freezing (as opposed to missing events
> or the like)?

It shouldn't be. It makes us fire more events and it shouldn't affect on performance. Let me file new try server build just in case.

Comment 14

[alexander :surkov (:asurkov)]

new try server build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d4f858233538/

Comment 15

[Marco Zehe (:MarcoZ)]

Same result with this new try-server build. On my blog, NVDA is blocked until *all* iframes have also been loaded. And I'm seeing the performance lag that Jamie describes.

Comment 16

[alexander :surkov (:asurkov)]

Here's a new build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b60861c2bfcd/

it fires document loading events for top document of the document load chain. Marco, can you try it please?

Comment 17

[Marco Zehe (:MarcoZ)]

No better, NVDA still waits until the very last of those iFrames has finished loading on my blog.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #17)
> No better, NVDA still waits until the very last of those iFrames has
> finished loading on my blog.

I wonder if you can see a different picture on trunk (because there's no any change in case of your blog: events are preserved)

Comment 19

[Marco Zehe (:MarcoZ)]

OK, I can actually get NVDA to speak while the iFrames are still loading. So this latest build *is* better than the previous ones.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #19)
> OK, I can actually get NVDA to speak while the iFrames are still loading. So
> this latest build *is* better than the previous ones.

actually it should be the same to nightly in case of the web page loading (for example, your blog). If it's not then we need to ask Jamie to take a look.

Comment 21

[alexander :surkov (:asurkov)]

So, Marco, can you any negative effect on any screen reader running the latest build?

Comment 22

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 23

[Trevor Saunders (:tbsaunde)]

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

>diff --git a/accessible/src/generic/DocAccessible-inl.h b/accessible/src/generic/DocAccessible-inl.h
>new file mode 100644
>--- /dev/null
>+++ b/accessible/src/generic/DocAccessible-inl.h
>@@ -0,0 +1,19 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=2 et sw=2 tw=80: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef mozilla_a11y_DocAccessible_inl_h_
>+#define mozilla_a11y_DocAccessible_inl_h_
>+
>+#include "DocAccessible.h"
>+#include "nsAccessibilityService.h"
>+
>+inline DocAccessible*
>+DocAccessible::ParentDocument() const

shouldnt you include nsIDocument.h too?

loading state.
>+  if (parentTreeItem)
>+    return ParentDocument()->HasLoadState(eCompletelyLoaded);

kind of funny we only fire events when outer doc is loading, ubt I guess we have to keep from breaking people :/

btw, update the comment for this method?

Comment 24

[Marco Zehe (:MarcoZ)]

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

>+     * Load the document having sub document. No document loading events for
>+     * nested document.

Why? isn't this bug about exactly that, firing those events on iframes, too?

Comment 25

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #24)

> >+     * Load the document having sub document. No document loading events for
> >+     * nested document.
> 
> Why? isn't this bug about exactly that, firing those events on iframes, too?

not exactly, we fire events on root of tree of loading documents

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #23)

> >+#include "DocAccessible.h"
> >+#include "nsAccessibilityService.h"
> >+
> >+inline DocAccessible*
> >+DocAccessible::ParentDocument() const
> 
> shouldnt you include nsIDocument.h too?

I guess it's included by something else.

> loading state.
> >+  if (parentTreeItem)
> >+    return ParentDocument()->HasLoadState(eCompletelyLoaded);
> 
> kind of funny we only fire events when outer doc is loading, ubt I guess we
> have to keep from breaking people :/

it's ok if you keep them as about coalesced events

> btw, update the comment for this method?

sure

Comment 27

[Marco Zehe (:MarcoZ)]

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

OK, since the last try-server build doesn't show any negative impact on common Windows screen readers, f=me.

Comment 28

[Frank DiPalermo]

From Frank DiPalermo on 5/29/2012:
We are wondering if this fix might be also put into the 10.0.x ESR stream?  That is the version that IBM is standardized on and this fix is very important to some IBM applications.

Comment 29

[alexander :surkov (:asurkov)]

(In reply to Frank DiPalermo from comment #28)
> From Frank DiPalermo on 5/29/2012:
> We are wondering if this fix might be also put into the 10.0.x ESR stream? 
> That is the version that IBM is standardized on and this fix is very
> important to some IBM applications.

I doubt because only security fixes are allowed on ESR afaik.

Anyway, can we cc somebody from ESR team to answer the question?

Comment 30

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4c563086f1f

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b4c563086f1f