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)
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: aaronlev, Assigned: aaronlev)
References
()
Details
Attachments
(2 files, 5 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
aaronlev
:
review+
aaronlev
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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•22 years ago
|
Target Milestone: --- → Future
Assignee | ||
Comment 1•22 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?
Requirement:
When implementing EVENT_OBJECT_REORDER, pls remember ATK need additional
informationm, whose data struct defined as AtkChildrenChange in file
nsRootAccessible.h.
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 | ||
Comment 5•22 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•22 years ago
|
||
Assignee | ||
Comment 7•22 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.
aaron, thanks for the good start.
taking...
Assignee: aaronl → kyle.yuan
Assignee | ||
Comment 9•22 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 | ||
Comment 10•22 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•22 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•22 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•22 years ago
|
||
only listen to DOMNodeInserted and DOMNodeRemoved, fire EVENT_REORDER to ATs.
aaron, any advice?
Attachment #103147 -
Attachment is obsolete: true
Comment 14•22 years ago
|
||
Attachment #105541 -
Attachment is obsolete: true
Assignee | ||
Comment 15•22 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.
Comment 16•22 years ago
|
||
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•22 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.
Comment 18•22 years ago
|
||
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•22 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•22 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•22 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•22 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.
Assignee | ||
Comment 23•22 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•22 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 26•22 years ago
|
||
Another testcase here:
http://www.mozilla.org/docs/dom/samples/dynatable-light/index.html
Assignee | ||
Comment 27•22 years ago
|
||
Attachment #103148 -
Attachment is obsolete: true
Assignee | ||
Comment 28•22 years ago
|
||
I'm not 100% sure whether replaceChild() is the only place where I need to impl
the mutation event SubtreeModified()
Assignee | ||
Updated•22 years ago
|
Attachment #121253 -
Flags: review?(kyle.yuan)
Assignee | ||
Updated•22 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•22 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•22 years ago
|
Attachment #121253 -
Flags: superreview?(jst)
Comment 30•22 years ago
|
||
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•22 years ago
|
||
Attachment #121253 -
Attachment is obsolete: true
Assignee | ||
Comment 32•22 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•22 years ago
|
||
Comment on attachment 121862 [details] [diff] [review]
With Jst's improvements
Carrying Kyle's r=
Updated•22 years ago
|
Attachment #121862 -
Flags: superreview?(jst)
Attachment #121862 -
Flags: superreview+
Assignee | ||
Comment 34•22 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 35•22 years ago
|
||
Did anyone approve this checkin for 1.4b?
Comment 36•22 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•22 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.
Comment 38•22 years ago
|
||
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.
Description
•