Closed Bug 149654 Opened 22 years ago Closed 22 years ago

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

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Future

People

(Reporter: aaronlev, Assigned: aaronlev)

References

()

Details

Attachments

(2 files, 5 obsolete files)

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.
Target Milestone: --- → Future
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?
Requirement: When implementing EVENT_OBJECT_REORDER, pls remember ATK need additional informationm, whose data struct defined as AtkChildrenChange in file nsRootAccessible.h.
Does anyone from Sun want this bug?
Blocks: atfmeta
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).
Depends on: 74218
Attached patch Some progress made; some problems remain (obsolete) (deleted) — Splinter Review
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.
Attached file Testcase (obsolete) (deleted) —
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
aaron, thanks for the good start. taking...
Assignee: aaronl → kyle.yuan
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
Depends on: 174910
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.
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?
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.
Attached patch first version (obsolete) (deleted) — Splinter Review
only listen to DOMNodeInserted and DOMNodeRemoved, fire EVENT_REORDER to ATs. aaron, any advice?
Attachment #103147 - Attachment is obsolete: true
Attached patch oops, the previous patch is wrong (obsolete) (deleted) — Splinter Review
Attachment #105541 - Attachment is obsolete: true
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.
> 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...
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
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.
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.
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.
Depends on: 180415
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 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
Taking
Assignee: kyle.yuan → aaronl
Attachment #103148 - Attachment is obsolete: true
I'm not 100% sure whether replaceChild() is the only place where I need to impl the mutation event SubtreeModified()
Attachment #121253 - Flags: review?(kyle.yuan)
Summary: Send accessibility events for DOM mutation events → Send accessibility events for DOM mutation events, and invalidate appropriate part of accessibility cache
Comment on attachment 121253 [details] [diff] [review] Also fixes bug 74218 -- impls SubtreeModified() mutation event r=kyle
Attachment #121253 - Flags: review?(kyle.yuan) → review+
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-
Attached patch With Jst's improvements (deleted) — Splinter Review
Attachment #121253 - Attachment is obsolete: true
Comment on attachment 121862 [details] [diff] [review] With Jst's improvements With Jst's improvements
Attachment #121862 - Flags: superreview?(jst)
Attachment #121862 - Flags: review+
Comment on attachment 121862 [details] [diff] [review] With Jst's improvements Carrying Kyle's r=
Attachment #121862 - Flags: superreview?(jst)
Attachment #121862 - Flags: superreview+
checked in
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Did anyone approve this checkin for 1.4b?
and again, we ask: did anyone approve this for 1.4beta? We should back this out if it is causing bug 203774
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.

Attachment

General

Created:
Updated:
Size: