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

RESOLVED FIXED in Future

Status

()

defect
RESOLVED FIXED
17 years ago
16 years ago

People

(Reporter: aaronlev, Assigned: aaronlev)

Tracking

Trunk
Future
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Updated

17 years ago
Target Milestone: --- → Future
(Assignee)

Comment 1

17 years ago
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

17 years ago
Requirement:
When implementing EVENT_OBJECT_REORDER, pls remember ATK need additional
informationm, whose data struct defined as AtkChildrenChange in file
nsRootAccessible.h.
(Assignee)

Comment 3

17 years ago
Does anyone from Sun want this bug?
Blocks: atfmeta

Comment 4

17 years ago
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).
(Assignee)

Updated

17 years ago
Depends on: 74218
(Assignee)

Comment 5

17 years ago
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.
(Assignee)

Comment 6

17 years ago
Posted file Testcase (obsolete) —
(Assignee)

Comment 7

17 years ago
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.
Depends on: 74219, 74220

Comment 8

17 years ago
aaron, thanks for the good start. 
taking...
Assignee: aaronl → kyle.yuan
(Assignee)

Comment 9

17 years ago
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
(Assignee)

Updated

17 years ago
Depends on: 174910
(Assignee)

Comment 10

17 years ago
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

17 years ago
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?
(Assignee)

Comment 12

17 years ago
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

17 years ago
Posted patch first version (obsolete) — Splinter Review
only listen to DOMNodeInserted and DOMNodeRemoved, fire EVENT_REORDER to ATs.

aaron, any advice?
Attachment #103147 - Attachment is obsolete: true

Comment 14

17 years ago
Attachment #105541 - Attachment is obsolete: true
(Assignee)

Comment 15

17 years ago
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.
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

17 years ago
> 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.
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

17 years ago
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.
OS: Windows 2000 → All
(Assignee)

Comment 20

17 years ago
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

17 years ago
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.
(Assignee)

Comment 22

17 years ago
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.

Updated

17 years ago
Depends on: 180415
(Assignee)

Comment 23

16 years ago
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

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

this patch is obsolete. Aaron, you will work on this, right?
Attachment #105544 - Attachment is obsolete: true
(Assignee)

Comment 25

16 years ago
Taking
Assignee: kyle.yuan → aaronl
(Assignee)

Comment 28

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

Updated

16 years ago
Attachment #121253 - Flags: review?(kyle.yuan)
(Assignee)

Updated

16 years ago
Summary: Send accessibility events for DOM mutation events → Send accessibility events for DOM mutation events, and invalidate appropriate part of accessibility cache

Comment 29

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

r=kyle
Attachment #121253 - Flags: review?(kyle.yuan) → review+
(Assignee)

Updated

16 years ago
Attachment #121253 - Flags: superreview?(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);
Attachment #121253 - Flags: superreview?(jst) → superreview-
(Assignee)

Comment 31

16 years ago
Attachment #121253 - Attachment is obsolete: true
(Assignee)

Comment 32

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

With Jst's improvements
Attachment #121862 - Flags: superreview?(jst)
Attachment #121862 - Flags: review+
(Assignee)

Comment 33

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

Carrying Kyle's r=
Attachment #121862 - Flags: superreview?(jst)
Attachment #121862 - Flags: superreview+
(Assignee)

Comment 34

16 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Did anyone approve this checkin for 1.4b?

Comment 36

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

Comment 37

16 years ago
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.
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."

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