149654 - Send accessibility events for DOM mutation events, and invalidate appropriate part of accessibility cache

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

DOM mutation events are events that are fired when part of the DOM changes.

For example, IE fires an EVENT_OBJECT_REORDER event on a parent accessible when
the children underneath it are changed. There is also EVENT_OBJECT_CREATE,
EVENT_OBJECT_DESTROY.

Assistive technologies do not yet listen to these events. Until they do, this
bug will probably be marked FUTURE.

Comment 1

[Aaron Leventhal]

Kyle, is this related to any of the event work you're doing for ATK? Can we
utilize some of those events for our MSAA support?

Comment 2

[John Sun]

Requirement:
When implementing EVENT_OBJECT_REORDER, pls remember ATK need additional
informationm, whose data struct defined as AtkChildrenChange in file
nsRootAccessible.h.

Comment 3

[Aaron Leventhal]

Does anyone from Sun want this bug?

Comment 4

[Kyle Yuan]

Aaron, If this is not a critical thing, I can take it. Now I need to work with 
gnopernicus developers till our alpha version released (scheduled in early Oct).

Comment 5

[Aaron Leventhal]

Attachment: Some progress made; some problems remain

This works for inserted nodes, but not removed nodes. For some reason,
DOMNodeRemoved is always reporting the document as the related node.

Also, DOMSubtreeModified is not implemented yet, so we can't use that for
EVENT_REORDER.

If someone from the Sun team still has time to work on this, that is great.

Comment 6

[Aaron Leventhal]

Attachment: Testcase

Comment 7

[Aaron Leventhal]

Hmm... I think we actually want to use
DOMNodeInsertedIntoDocument/DOMNodeRemovedFromDocument instead of
DOMNodeInserted/DOMNodeRemoved. The latter give don't give the relatedNode we
want. Looks like we need bug 74219 and bug 74220 as well.

I find that it's easier to debug mutation events in mfcembed than in the full
XUL browser, because there are a lot fewer of them then.

Comment 8

[Kyle Yuan]

aaron, thanks for the good start. 
taking...

Comment 9

[Aaron Leventhal]

Kyle, it turns out that MSAA clients will crash if we use EVENT_OBJECT_CREATE or
EVENT_OBJECT_DESTROY. Therefore, at least in Windows, we need to limit ourselves to:

EVENT_OJBECT_REORDER
EVENT_OBJECT_SHOW
EVENT_OBJECT_HIDE

Comment 10

[Aaron Leventhal]

More specifically, Windows assistive technology can't hook EVENT_OBJECT_CREATE
and EVENT_OBJECT_DESTROY.   They are sometimes fired on partially destroyed or
partially constructed windows and then further processing causes a GPF in many
programs.

Comment 11

[Kyle Yuan]

Here is my comments:
1) 
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericElement.cpp#39
24
it will look for the mutation event listeners before the event get fired, and 
this checking took place within target node's document. So we either get rid of 
this checking code (could be a performance issue), or add the event listener in 
a lower class, such as nsHTMLIFrameAccessible.

2) So far, SubtreeModified/NodeRemovedFromDocument/NodeInsertedIntoDocument are 
not implemeted yet, NodeInserted/NodeRemoved will crash ATs, what should we do 
with AttrModified/CharacterDataModified? Watch the change of |visible| 
attribute?

Comment 12

[Aaron Leventhal]

Kyle,

We don't need to listen to DOMAttrModified yet -- I think we can add that  if we
find particular attributes that are worth listening to.

NodeInserted/NodeRemoved will not crash ATs -- it is
EVENT_OBJECT_CREATE/EVENT_OBJECT_DESTROY, which can only be a problem if the AT
asks to listen for them.

In any case we can always fire EVENT_OBJECT_REORDER for NodeInserted/NodeRemoved.

Comment 13

[Kyle Yuan]

Attachment: first version

only listen to DOMNodeInserted and DOMNodeRemoved, fire EVENT_REORDER to ATs.

aaron, any advice?

Comment 14

[Kyle Yuan]

Attachment: oops, the previous patch is wrong

Comment 15

[Aaron Leventhal]

Kyle,

> it will look for the mutation event listeners before the event get fired, and 
> this checking took place within target node's document. So we either get rid 
> of  this checking code (could be a performance issue), or add the event 
> listener in a lower class, such as nsHTMLIFrameAccessible.

We could add the listers in a similar way to the AddContentDocListeners(), where
I'm listening to progress/scrollbar stuff on individual nsHTMLIFrameAccessible's
but using the nsRootAccessible it inherits from to do most of the work.
However, perhaps that's a bug. Hixie might know -- Hixie, if a script attaches a
mutation listener to a document with iframes, should mutations in the iframes
bubble up to the top level document?

Also, I think the problem with using DOMNodeInserted/DOMNodeRemoved is that they
don't have the exact related node we want -- they give always give the document
for that. Ian, is that correct? I think that is why we need
DOMNodeInsertedIntoDocument/DOMNodeRemovedFromDocument to be implemented.
Otherwise the Accessibility API's will always report the document for REORDER
events instead of the specific accessible node. I could be wrong -- what do you
think?

I think FireDOMMutationEvent(I) should be called FireAccessibleMutationEvent()
or HandleDOMMutationEvent. It's not firing a dom event.

Comment 16

[Hixie (not reading bugmail)]

Mutation events are not special, so they should bubble just like other events.
Do click events bubble out of iframes?

Anyway, I don't really know much about DOM Events. I recomment asking jst or
someone else close to the DOM WG.

Comment 17

[Kyle Yuan]

> Also, I think the problem with using DOMNodeInserted/DOMNodeRemoved is that
> they don't have the exact related node we want -- they give always give the
> document for that. 
You could be wrong. With your test case, I always get <div> for insertion and 
<body> for removal as the related node. 
I've added the w3c DOM 2 event document link into the URL section. The 
DOMNodeRemovedFromDocument/DOMNodeInsertedIntoDocument is not bubble-able and 
even not contains related node. So they are not suitable for our case, I think.

Comment 18

[Johnny Stenback (:jst)]

Mutation events, just like all other event's, do *not* bubble from a iframe
document to the document that contained the iframe. You might be able to capture
them in the top-level chrome window, but I'm not sure if that's ever been tested...

If DOMNodeInserted/DOMNodeRemoved always give the document as the related node I
think that's a bug in our implementation, and I know we have other event
targeting bugs as well, so this one could be related, who knows...

Comment 19

[Kyle Yuan]

Aaron, just as jst said, iframes' mutation events can bubble up to the top 
level document (chrome xul document). That's not the point. The point is we 
have to add mutation event listeners to *every* document (our 
nsHTMLIFrameAccessible), otherwise mutation events won't get fired. 

Maybe we should make nsHTMLIFrameAccessible inherit from nsRootAccessible, or 
move the mutation listeners into their common parent - nsDocAccessibleMixin.

Comment 20

[Aaron Leventhal]

I don't think we need to put it in nsDocAccessibleMixin. Look how I changed web
progress and scroll listening so that it occurs on each document, including
frames/iframes.

Comment 21

[Kyle Yuan]

there is a little difference - AddContentDocListeners/AddScrollListener only  
listen to the root document's web progress and scrolling (they are called by 
nsRootAccessible/nsHTMLIFrameRootAccessible), but I need to listen every 
document's mutation events.

BTW, I found the ScrollPositionListener can't capture the scroll events from 
frame/iframe.

Comment 22

[Aaron Leventhal]

No, they listen to every iframe's scroll/progress events:

See nsHTMLIFrameRootAccessible.cpp:

/* void addAccessibleEventListener (in nsIAccessibleEventListener aListener); */
NS_IMETHODIMP
nsHTMLIFrameRootAccessible::AddAccessibleEventListener(nsIAccessibleEventListener
*aListener)
{
  NS_ASSERTION(aListener, "Trying to add a null listener!");
  if (!mListener) {
    mListener = aListener;
    AddContentDocListeners();
  }
  return NS_OK;
}

/* void removeAccessibleEventListener (); */
NS_IMETHODIMP nsHTMLIFrameRootAccessible::RemoveAccessibleEventListener()
{
  if (mListener) {
    RemoveContentDocListeners();
    mListener = nsnull;
  }
  return NS_OK;
}

This works because nsHTMLIFrameRootAccessible inherits from nsRootAccessible.

Comment 23

[Aaron Leventhal]

This bug is now very important. We need the mutation events in order to
invalidate parts of the cache when the dom changes.

We may need to listen for attributes in some cases - for example, on the <a>
element, the attributes can affect whether it is a link or not.

Comment 24

[Kyle Yuan]

Comment on attachment 105544 [details] [diff] [review]
oops, the previous patch is wrong

this patch is obsolete. Aaron, you will work on this, right?

Comment 25

[Aaron Leventhal]

Taking

Comment 26

[Aaron Leventhal]

Another testcase here:
http://www.mozilla.org/docs/dom/samples/dynatable-light/index.html

Comment 27

[Aaron Leventhal]

Attachment: Testcase - includes test for replaceChild which now fires SubtreeModified()

Comment 28

[Aaron Leventhal]

Attachment: Also fixes bug 74218 -- impls SubtreeModified() mutation event

I'm not 100% sure whether replaceChild() is the only place where I need to impl
the mutation event SubtreeModified()

Comment 29

[Kyle Yuan]

Comment on attachment 121253 [details] [diff] [review]
Also fixes bug 74218 -- impls SubtreeModified() mutation event

r=kyle

Comment 30

[Johnny Stenback (:jst)]

Comment on attachment 121253 [details] [diff] [review]
Also fixes bug 74218 -- impls SubtreeModified() mutation event

- In nsAccessibilityService.cpp

@@ -767,7 +767,7 @@
       const nsTextFragment *textFrag;
       textContent->GetText(&textFrag);
       PRUnichar theChar = textFrag->CharAt(0);
-      if (theChar == NBSP)
+      if (theChar == NBSP || theChar=='\n')

There's a comment above this code talking about this, you need to update the
comment too. And what's the need for NBSP here (a local const PRUnichar)? That
local costs code and gives you little, if anything. Remove NBSP, replace it
with 160, and add an appropriate comment just above the if check.

- In nsDocAccessible.cpp:

@@ -363,6 +371,29 @@
					   nsITimer::TYPE_ONE_SHOT);
     }
   }
+
+  // add ourself as a mutation event listener 
+  // (this slows down mozilla about 3%, but only used when accessibility APIs
active)
+  nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(mDocument));
+  NS_ASSERTION(target, "No dom event target for document");
+  nsresult rv = target->AddEventListener(NS_LITERAL_STRING("DOMAttrModified"), 
+    NS_STATIC_CAST(nsIDOMMutationListener*, this), PR_TRUE);
+  NS_ASSERTION(NS_SUCCEEDED(rv), "failed to register listener");
+  rv = target->AddEventListener(NS_LITERAL_STRING("DOMSubtreeModified"), 
+    NS_STATIC_CAST(nsIDOMMutationListener*, this), PR_TRUE);
...

No need for those casts, or?

@@ -378,6 +409,15 @@
   // Remove scroll position listener
   nsCOMPtr<nsIPresShell> presShell(do_QueryReferent(mWeakShell));
   RemoveScrollListener(presShell);
+
+  nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(mDocument));
+  NS_ASSERTION(target, "No dom event target for document");
+  target->RemoveEventListener(NS_LITERAL_STRING("DOMAttrModified"),
NS_STATIC_CAST(nsIDOMMutationListener*, this), PR_TRUE);
...

Same thing here, no need for the casts.

- In nsGenericHTMLContainerElement::ReplaceChildAt():

     if (oldKid) {
+      if (nsGenericElement::HasMutationListeners(this,
NS_EVENT_BITS_MUTATION_SUBTREEMODIFIED)) {
+	 nsCOMPtr<nsIDOMEventTarget> node(do_QueryInterface(oldKid));
+	 nsMutationEvent mutation;
+	 mutation.eventStructType = NS_MUTATION_EVENT;
+	 mutation.message = NS_MUTATION_SUBTREEMODIFIED;
+	 mutation.mTarget = node;
+	 mutation.mRelatedNode = do_QueryInterface(NS_STATIC_CAST(nsIContent *,
this));
+	 
+	 nsEventStatus status = nsEventStatus_eIgnore;
+	 oldKid->HandleDOMEvent(nsnull, &mutation, nsnull,
+				NS_EVENT_FLAG_INIT, &status);
+      }
+

Fireing DOMSubtreeModified on the old child is wrong, if anything, you should
be fireing a DOMNodeRemoved on the old child. If you want to fire
DOMSubtreeModified then you should fire that on the parent of the node being
removed.

http://www.w3.org/TR/2000/REC-DOM-Level-2-Events-20001113/events.html#Events-ev
entgroupings-mutationevents

Fix the above, and I'll have one more look.

       oldKid->SetDocument(nsnull, PR_TRUE, PR_TRUE);
>       oldKid->SetParent(nsnull);
>       NS_RELEASE(oldKid);

Comment 31

[Aaron Leventhal]

Attachment: With Jst's improvements

Comment 32

[Aaron Leventhal]

Comment on attachment 121862 [details] [diff] [review]
With Jst's improvements

With Jst's improvements

Comment 33

[Aaron Leventhal]

Comment on attachment 121862 [details] [diff] [review]
With Jst's improvements

Carrying Kyle's r=

Comment 34

[Aaron Leventhal]

checked in

Comment 35

[Peter Van der Beken [:peterv]]

Did anyone approve this checkin for 1.4b?

Comment 36

[Alec Flett]

and again, we ask: did anyone approve this for 1.4beta? We should back this out
if it is causing bug 203774

Comment 37

[Aaron Leventhal]

No one approved this, during my move to Germany I didn't realize the tree had
changed status, and checked it in.

This bug fixes some crashes, because it ensures we shutdown all accessibles,
even those that get added in dynamic pages. It doesn't cause the crashes in bug
203774. Those were caused in the other large scale accessibility module checkins
before the tree was frozen.

Comment 38

[(not reading, please use [e-mail] instead)]

from bug 203774:

aaronl writes, "Bug 149654 did not cause these crashes, those were caused by the
accessibility rewrite in general. Bug 149654 actually fixes some crashes,
because it ensures we shutdown all accessibles, even those that get added in
dynamic pages."