420845 - Fire event_reorder on any embedded frames/iframes whos document has just loaded.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[Michael Curran]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008030305 Minefield/3.0b4pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008030305 Minefield/3.0b4pre

So that ATs know that a subtree has changed, nodes fire an event_reorder to say that its children have some how changed. For nodes that only contain text and the text changes, text_inserted/text_removed is fired. This means that ATs can simply watch for events that tell it to re-render part of the tree in their own cached implementations. However, iframes and frames do not fire an event to indicate that its children has changed (e.g. when a document is removed or added). If an event_reorder was fired on any iframe/frame in a document, when its own document was loaded, this would make it much easier for ATs such as NVDA to track changes such as this.  It could be possible to just track show and hide events, and then go up the parents or use node_child_of relation, however at this time in NVDA its much easier to just re-render subtrees than individual nodes.


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Marco Zehe (:MarcoZ)]

This is an important efficiency fix which we should implement if possible.

Comment 2

[Damon Sicore (:damons)]

Ok, so it's an efficiency fix, but should it block the release?

Comment 3

[Damon Sicore (:damons)]

Poke.  Answer for question in comment #2?

Comment 4

[alexander :surkov (:asurkov)]

Michael, we don't fire EVENT_DOCUMENT_LOAD_COMPLETE and etc event for documents contained by frames/iframes or arent' they helpful? 

Comment 5

[Aaron Leventhal]

Damon, i think screen readers like NVDA can get around this. Marco requested it though, so maybe he can explain why.

Mick, can you use the new NODE_CHILD_OF relation we put on the frame/iframe objects?

Comment 6

[Michael Curran]

(In reply to comment #5)
> Mick, can you use the new NODE_CHILD_OF relation we put on the frame/iframe
> objects?
Yes, we can use it, and probably will implement that after CSUN. However, I still think that allowing iframes to fire event_reorder is just making the over all pattern of events more complete.  As far as I understand, Gecko fires textInserted / textRemoved for any Accessible object that contains text, as it changes. But, for any accessible object that does not have text, and its children change in some way, an event_reorder is fired.  However, this is not true for frames/iframes. If these were fired for frames/iframes, NVDA's dynamic content support would become pretty much complete, in that if a new document is loaded in a frame/iframe due to an action on the page, NVDA would re-render the frame and therefore display the new sub document. This does not currently happen. We could use node_child_of, but the code would be a specific exception to the event_reorder rule. I guess this is not a really really important issue as we can use node_child_of, but  I still believe that adding this for frames/iframes, completes the use of reorder on accessible objects in Gecko.   

Comment 7

[Marco Zehe (:MarcoZ)]

(In reply to comment #2)
> Ok, so it's an efficiency fix, but should it block the release?

See Mick's explanation of the advantage it would have in comment #6. I would say it is at least a very very trong WANTED, because we should try and help the NVDA developers to get the best support possible, and to me it sounds like using relation_child_of is a hoop they'd have to go through because of our incomplete implementation.

Aaron, Surkov, how difficult would it be to implement this?

Comment 8

[Mike Schroepfer]

Given this can be worked around taking off the blocker list. We'd take a fix now or in a 0.x release.

Comment 9

[Aaron Leventhal]

Mick & Jamie, we never got around to fixing this, and I haven't heard you complain. That makes me think we don't need to fix it. It might make things more consistent but we have so many other bugs that are actually affecting users. Let me know if you feel strongly one way or another.

Comment 10

[Michael Curran]

NVDA does not depend on this bug specifically. Although as I have previously stated, currently there must be an exception made for documents in frames and iframes.
 This bug certainly isn't overly important right now. But I wouldn't like to see it get closed because of that.
All objects fire event_reorder if a child is added or removed, except for iframes and frames. 
Either:
*Fire an event_reorder when a document in the frame loads, or
*Fire an event_reorder when the new document object is created, and then fire event_document_load_complete on the document when its fully loaded.

Comment 11

[Marco Zehe (:MarcoZ)]

Is this still an issue after bug 472662 landed?

Comment 12

[James Teh [:Jamie]]

(In reply to comment #11)
> Is this still an issue after bug 472662 landed?
Yes. We still get no reorder event on frames/iframes in: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090307 Minefield/3.2a1pre.

Comment 13

[James Teh [:Jamie]]

For the record, document load complete isn't fired on the document either. Thankfully, we do get a state change, so this is what we are using in our reimplementation of NVDA's virtual buffer code.

Comment 14

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

Mass un-assigning bugs assigned to Aaron.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 16

[Marco Zehe (:MarcoZ)]

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

One nit:
>+  <title>Accessible documentation events testing</title>


Should be "document events", not "documentation", correct?

Related question: Is there a way for us to test the loading/state-busy, loaded/busy flag cleared, refreshing/state-busy etc. states we normally fire for a document load so we have test coverage for these? Or should I file a new bug for those?

Comment 17

[Marco Zehe (:MarcoZ)]

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

r=me with the nit in comment #16 addressed. I also tested this patch and it appears to have no negative effects on ATs.

Comment 18

[alexander :surkov (:asurkov)]

try build https://build.mozilla.org/tryserver-builds/[e-mail]-try-b240652c31e9/

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #16)

> Related question: Is there a way for us to test the loading/state-busy,
> loaded/busy flag cleared, refreshing/state-busy etc. states we normally fire
> for a document load so we have test coverage for these? Or should I file a new
> bug for those?

we should listen events I think and test states. iirc we have bugs here. It's worth to deal in another bug.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> (From update of attachment 388673 [details] [diff] [review])
> One nit:
> >+  <title>Accessible documentation events testing</title>
> 
> 
> Should be "document events", not "documentation", correct?

right, I guess I tired a bit. I replaced on "Accessible events testing for document" locally.

Comment 21

[alexander :surkov (:asurkov)]

James, Mick, could you try the try-server build to ensure this suites your needs?

Comment 22

[James Teh [:Jamie]]

(In reply to comment #21)
> James, Mick, could you try the try-server build to ensure this suites your
> needs?
Works very well. Thanks!

Comment 23

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

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

r=me; thanks.

Feels like we should document this somewhere. Do we have an AT guide?

>+++ b/accessible/src/base/nsDocAccessible.cpp
>@@ -612,17 +612,27 @@ nsDocAccessible::Init()

>+  FireDelayedAccessibleEvent(reorderEvent);
>+  return NS_OK;
> }

So we'll fire a reorder even for the main document? I guess that's ok.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #23)
> (From update of attachment 388673 [details] [diff] [review])
> r=me; thanks.
> 
> Feels like we should document this somewhere. Do we have an AT guide?

mark it as dev-doc-needed.

> >+  FireDelayedAccessibleEvent(reorderEvent);
> >+  return NS_OK;
> > }
> 
> So we'll fire a reorder even for the main document? I guess that's ok.

probably, it would be more logical to put it in parent's queue but this requires one more step to get parent's document :) The current approach is also ok.

Comment 25

[alexander :surkov (:asurkov)]

checked in on 1.9.2, http://hg.mozilla.org/mozilla-central/rev/3503da0dd622

Comment 26

[Eric Shepherd [:sheppy]]

Now listed in the notes on Firefox 3.6 for developers:

https://developer.mozilla.org/en/Firefox_3.6_for_developers

Comment 27

[Nickolay_Ponomarev]

The docs now have this phrase: "The reorder event is now sent to embedded frames and iframes when their document is loaded. See bug 420845."

Alexander, David, could you clarify the documentation or at least comment here, please? To a naive reader of that page it reads as "an alias for the DOM "load" event was added", which is probably far from being true.

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #27)
> The docs now have this phrase: "The reorder event is now sent to embedded
> frames and iframes when their document is loaded. See bug 420845."
> 
> Alexander, David, could you clarify the documentation or at least comment here,
> please? To a naive reader of that page it reads as "an alias for the DOM "load"
> event was added", which is probably far from being true.

You're right reorder event is not alias of the load event.  Load event is fired on document accessible when document was loaded. Reorder event is fired on document container accessible when document accessible was attached to it.

So that reorder event is used to notify the AT the subtree was changed. Load event is used to notify the AT the content of the document is ready (we don't fire any mutation events like reorder, show, hide for the document content prior this point).

The current implementation makes fire reorder event before load event.

I think technically AT would be able to use load event instead of reorder event in this case but they find the reorder event more comfortable because in this case they don't need any special processing of document container accessible. Iirc that was an original point of the bug.

Comment 29

[Nickolay_Ponomarev]

OK, is it a DOM event? Content-accessible or chrome-only? Fired only when accessibility is enabled? In other words, what kind of developers may be interested in this?

I have a strong suspicion this should have been added to accessibility APIs documentation only and isn't interesting to the general web developer/extension developer population. Am I wrong?

Comment 30

[alexander :surkov (:asurkov)]

You're right. It's a11y event. Usually developer/extension don't need this until they create  accessibility tool. In gecko all a11y events can be watched by nsIObserverService service. As well internal gecko a11y events result system events are fired on various platforms, for example, Win API NotifyWinEvent function is used to fire a MSAA event.

Comment 31

[Nickolay_Ponomarev]

Thanks a lot for the clarification! I clarified the text on https://developer.mozilla.org/en/Firefox_3.6_for_developers (at least making it clear it's about accessibility).

EVENT_REORDER is also mentioned on a few other pages, which may need updating, I'm not sure: https://developer.mozilla.org/Special:Search?search=EVENT_REORDER&type=fulltext&go=Search

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #31)
> Thanks a lot for the clarification! I clarified the text on
> https://developer.mozilla.org/en/Firefox_3.6_for_developers (at least making it
> clear it's about accessibility).

I guess it would make sense to have own section for accessibility changes.

> EVENT_REORDER is also mentioned on a few other pages, which may need updating,
> I'm not sure:
> https://developer.mozilla.org/Special:Search?search=EVENT_REORDER&type=fulltext&go=Search

Our accessibility document needs to be updated and possibly rewritten from the start.