Last Comment Bug 754165 - fire document load events on iframes too
: fire document load events on iframes too
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: alexander :surkov
:
: alexander :surkov
Mentors:
Depends on: 760038
Blocks: eventa11y
  Show dependency treegraph
 
Reported: 2012-05-10 21:15 PDT by alexander :surkov
Modified: 2012-05-31 09:03 PDT (History)
6 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (8.86 KB, patch)
2012-05-28 08:33 PDT, alexander :surkov
tbsaunde+mozbugs: review+
mzehe: feedback+
Details | Diff | Splinter Review

Description alexander :surkov 2012-05-10 21:15:44 PDT
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 2012-05-11 04:19:08 PDT
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 2012-05-11 04:19:34 PDT
(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/surkov.alexander@gmail.com-d823fa033eae/
Comment 3 Marco Zehe (:MarcoZ) 2012-05-11 07:08:34 PDT
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 2012-05-11 07:19:39 PDT
I think we can ask Jamie to fix that if NVDA is unique case. What about other screen readers?
Comment 5 Marco Zehe (:MarcoZ) 2012-05-11 08:08:57 PDT
The others wait until everything is loaded and start speaking their page summaries. I tested JAWS and Window-Eyes.
Comment 6 Marco Zehe (:MarcoZ) 2012-05-11 08:21:12 PDT
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] 2012-05-13 16:27:21 PDT
(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 2012-05-13 18:45:37 PDT
(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] 2012-05-13 20:03:18 PDT
(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] 2012-05-13 20:25:39 PDT
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) 2012-05-14 01:23:33 PDT
(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] 2012-05-14 01:31:03 PDT
(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 2012-05-14 01:38:50 PDT
(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 15 Marco Zehe (:MarcoZ) 2012-05-14 06:30:35 PDT
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 2012-05-23 07:18:38 PDT
Here's a new build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-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) 2012-05-23 08:23:57 PDT
No better, NVDA still waits until the very last of those iFrames has finished loading on my blog.
Comment 18 alexander :surkov 2012-05-23 08:30:25 PDT
(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) 2012-05-23 09:43:26 PDT
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 2012-05-23 09:48:52 PDT
(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 2012-05-26 08:52:41 PDT
So, Marco, can you any negative effect on any screen reader running the latest build?
Comment 22 alexander :surkov 2012-05-28 08:33:20 PDT
Created attachment 627706 [details] [diff] [review]
patch
Comment 23 Trevor Saunders (:tbsaunde) 2012-05-28 12:15:57 PDT
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) 2012-05-29 01:19:15 PDT
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 2012-05-29 01:37:42 PDT
(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 2012-05-29 01:39:34 PDT
(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) 2012-05-29 04:05:12 PDT
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 2012-05-29 10:35:05 PDT
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 2012-05-29 10:54:11 PDT
(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 31 Ed Morley [:emorley] 2012-05-30 07:36:09 PDT
https://hg.mozilla.org/mozilla-central/rev/b4c563086f1f

Note You need to log in before you can comment on or make changes to this bug.