Closed
Bug 349885
Opened 18 years ago
Closed 18 years ago
GetAccessibleFor() doesn't look if ally nodes are allowed inside anon content
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
People
(Reporter: surkov, Assigned: surkov)
References
Details
(Keywords: access)
Attachments
(2 files, 9 obsolete files)
12.99 KB,
patch
|
Details | Diff | Splinter Review | |
1.30 KB,
patch
|
evan.yan
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
When nsIAccessibilityService::GetAccessibleFor() tries to create accessible node then it doesn't look if ally nodes are allowed inside anonymous content. As result we get there are accessible nodes inside anon content even if they are not allowed. I guess algorithm should be like in patch of bug 344896. But problem of that approach is it is used GetAccessibleFor().
Comment 1•18 years ago
|
||
Actually we want to be able to create accessible nodes for the nodes in there, for example that's how the impl for XUL textbox works. It creates a temporary accessible for the HTML input.
However, it's reasonable for the tree walker to check that, and not walk deeper. That's how it works for the *aIsHidden info it gets back from GetAccessibleFor().
Keywords: access
Assignee | ||
Comment 2•18 years ago
|
||
(In reply to comment #1)
> Actually we want to be able to create accessible nodes for the nodes in there,
> for example that's how the impl for XUL textbox works. It creates a temporary
> accessible for the HTML input.
XUL textbox doesn't use GetAccessibleFor() I guess. It calls:
nsHTMLTextFieldAccessible tempAccessible(inputField, mWeakShell);
> However, it's reasonable for the tree walker to check that, and not walk
> deeper. That's how it works for the *aIsHidden info it gets back from
> GetAccessibleFor().
>
I can't see where does GetAccessibleFor() use tree walker (http://lxr.mozilla.org/mozilla/source/accessible/src/base/nsAccessibilityService.cpp#1162).
For sure if you have in view nsAccessibleTreeWalker.
I think if we can assume that all nodes that have xbl binding implements nsIAccessibleProvider interface then it can be fixed it like in mentioned bug.
Comment 3•18 years ago
|
||
Okay, maybe you're write.
BTW, it's the tree walker which uses GetAccessibleFor()
Comment 4•18 years ago
|
||
Actually, if GetAccessibleFor() was to do this it would have to walk up the parent chain each time an accessible was requested.
Maybe it's better to do this in nsAccessibleTreeWalker. Fix GetFirstChild so it doesn't see anon nodes if the current accessible does't allow anon nodes.
Assignee | ||
Comment 5•18 years ago
|
||
(In reply to comment #4)
> Actually, if GetAccessibleFor() was to do this it would have to walk up the
> parent chain each time an accessible was requested.
>
Does it mean fix of bug 337674 uses wrong approach? I.e. we shouldn't use nsIAccessibilityService there and we should write new view that will use nsAccessibleTreeWalker. Right?
Comment 6•18 years ago
|
||
(In reply to comment #5)
> (In reply to comment #4)
> > Actually, if GetAccessibleFor() was to do this it would have to walk up the
> > parent chain each time an accessible was requested.
> >
>
> Does it mean fix of bug 337674 uses wrong approach? I.e. we shouldn't use
> nsIAccessibilityService there and we should write new view that will use
> nsAccessibleTreeWalker. Right?
>
I see what you mean.
Can you think of a way of doing this which is performant for the tree walking case (no extra parent walking) and still gives the correct information when GetAccessibleFor() is used on its own?
Assignee | ||
Comment 7•18 years ago
|
||
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Actually, if GetAccessibleFor() was to do this it would have to walk up the
> > > parent chain each time an accessible was requested.
> > >
> >
> > Does it mean fix of bug 337674 uses wrong approach? I.e. we shouldn't use
> > nsIAccessibilityService there and we should write new view that will use
> > nsAccessibleTreeWalker. Right?
> >
> I see what you mean.
>
> Can you think of a way of doing this which is performant for the tree walking
> case (no extra parent walking) and still gives the correct information when
> GetAccessibleFor() is used on its own?
>
Can we cache the nodes that can't be accessible? It lets us to check that only one time. Also we can put non accessible node and it's accessible parent node together. It lets us to skip accessible parent node seeking in GetTargetNode(). How about this?
Assignee | ||
Updated•18 years ago
|
Assignee: aaronleventhal → surkov.alexander
Status: ASSIGNED → NEW
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•18 years ago
|
||
I added two additional caches.
1) Cache for accessibles. I can't use nsDOCAccessible::mAccessNodeCache since it contains access nodes even if they are not accessible.
3) Cache for explicit parent accessibles. The cache allows us to skip everytime checking for bindings chain. The cache will be helpful for GetTargetNode() of nsRootAccessible.
Attachment #235627 -
Flags: review?(aaronleventhal)
Assignee | ||
Comment 9•18 years ago
|
||
Aaron, what's about the patch approach?
Assignee | ||
Comment 10•18 years ago
|
||
Attachment #235627 -
Attachment is obsolete: true
Attachment #242789 -
Flags: review?(aaronleventhal)
Attachment #235627 -
Flags: review?(aaronleventhal)
Assignee | ||
Comment 11•18 years ago
|
||
Probably it's not nice to have three caches. I can see other approach is to integrate into DOM tree so that every dom element will have property accessibleInfo. The property will show if we already tried to create accessible object and if we created it already then it can keep link on it.
Assignee | ||
Comment 12•18 years ago
|
||
Sorry. Previously I attahced old one.
Attachment #242789 -
Attachment is obsolete: true
Attachment #242790 -
Flags: review?(aaronleventhal)
Attachment #242789 -
Flags: review?(aaronleventhal)
Comment 13•18 years ago
|
||
I don't like all the new caches. I would prefer that there is a getter on nsIAccessibleRetrieval/nsIAccessibilityService that takes a boolean aIsDetachedOkay or something like that. Detached means it's an accessible that is not in the tree.
Whenever the DOM Inspector or nsRootAccessible's event handling is asking for the accessible, it can say detached accessibles are not okay. In other places where we're walking the tree we can skip that check as an optimization.
Updated•18 years ago
|
Attachment #242790 -
Flags: review?(aaronleventhal) → review-
Assignee | ||
Comment 14•18 years ago
|
||
Will it go?
Attachment #242790 -
Attachment is obsolete: true
Attachment #243478 -
Flags: review?(aaronleventhal)
Assignee | ||
Comment 15•18 years ago
|
||
(In reply to comment #14)
> Created an attachment (id=243478) [edit]
> patch4
>
> Will it go?
>
I forgot to update guid.
Comment 16•18 years ago
|
||
What callers will you have? nsRootAccessible::GetTargetNode and DOMi?
Assignee | ||
Comment 17•18 years ago
|
||
(In reply to comment #16)
> What callers will you have? nsRootAccessible::GetTargetNode and DOMi?
>
Both, but for DOMi I think we need additional method like getAccessibleInTreeInWindow(). I like this approach since when I search accessible then actually I create it, so I don't need a call of getAccessible() method.
Comment 18•18 years ago
|
||
Can you put the caller support into the patch to and I can r= all at the same time?
Assignee | ||
Comment 19•18 years ago
|
||
I leaved presentation shell #0 using
Attachment #243478 -
Attachment is obsolete: true
Attachment #243489 -
Flags: review?(aaronleventhal)
Attachment #243478 -
Flags: review?(aaronleventhal)
Comment 20•18 years ago
|
||
Comment on attachment 243489 [details] [diff] [review]
patch5
In the IDL comment can you say "tree of accessible objects" instead of "accessible tree", to make sure people know we're not talking about <tree>?
I would expect that the DOMi and nsRootAccessible::GetTargetNode() patches would come next.
Attachment #243489 -
Flags: review?(aaronleventhal) → review+
Assignee | ||
Comment 21•18 years ago
|
||
all in one
Attachment #243489 -
Attachment is obsolete: true
Attachment #243504 -
Flags: review?(aaronleventhal)
Comment 22•18 years ago
|
||
On mozilla.dev.tech.accessibility they're telling me not to assume 1 presshell per document. So, I was wrong. You can pass in the current view (document.defaultView?) and get the presshell for it:
http://lxr.mozilla.org/seamonkey/source/layout/inspector/src/inLayoutUtils.cpp#81
Assignee | ||
Comment 23•18 years ago
|
||
(In reply to comment #22)
> On mozilla.dev.tech.accessibility they're telling me not to assume 1 presshell
> per document. So, I was wrong. You can pass in the current view
> (document.defaultView?) and get the presshell for it:
> http://lxr.mozilla.org/seamonkey/source/layout/inspector/src/inLayoutUtils.cpp#81
>
But we don't assume 1 presshell per document, we assume the first one is needed. On mozilla.dev.tech.layout Peter Kasting said: "we can have two: the 0th
presshell, which is the "normal" one, and the 1st, which is the
"printing" presshell. ". I'd vote to leave as is since it's not clear for me what defaultView should return though it looks like both presshell #0 and defaultView work.
Comment 24•18 years ago
|
||
1.
I just noticed in http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLFormControlAccessible.cpp#523
that we say allowsAnonChildren is PR_FALSE, but we don't impl GetAllowsAnonChildren() for that object. Seems like a bug. We can deal with that in a separate bug..
2.
As I mentioned in the previous comment from mozilla.dev.tech.layout it looks like we do need to support passing in of the view. I suppose we can also deal with that in a separate bug if you want.
2.
We don't want to automatically create accessibles for pagehide events, see
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsRootAccessible.cpp#544
It's happening because of the GetAccessibleFor() at the end of your new method.
You could just return the node you have at that point instead of GetAccessibleFor(). This also fixes it so we don't call GetAccessibleFor() extra times.
Here's my suggestion:
Change your method to be called
GetNodeForAccessibleCheck() and just have it return a dom node for checking. Don't have it do a GetAccessibleFor() at the end. HandleEvent() will use this new method.
Make a new GetAttachedAccessibleFor() which you'll use in DOM Inspector. The new method will use GetNodeForAccessibleCheck() followed by GetAccessibleFor() on that node. You can't use the GetAttachedAccessibleFor() convenience in nsRootAccessible::HandleEvent() because of the pagehide issue.
Updated•18 years ago
|
Attachment #243504 -
Flags: review?(aaronleventhal) → review-
Assignee | ||
Comment 25•18 years ago
|
||
(In reply to comment #24)
> 1.
> I just noticed in
> http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLFormControlAccessible.cpp#523
> that we say allowsAnonChildren is PR_FALSE, but we don't impl
> GetAllowsAnonChildren() for that object. Seems like a bug. We can deal with
> that in a separate bug..
I will check and I guess I can fix it in bug 344896.
> 2.
> As I mentioned in the previous comment from mozilla.dev.tech.layout it looks
> like we do need to support passing in of the view. I suppose we can also deal
> with that in a separate bug if you want.
I'll post bug for this.
> 2.
> We don't want to automatically create accessibles for pagehide events, see
> http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsRootAccessible.cpp#544
> It's happening because of the GetAccessibleFor() at the end of your new method.
>
> You could just return the node you have at that point instead of
> GetAccessibleFor(). This also fixes it so we don't call GetAccessibleFor()
> extra times.
>
> Here's my suggestion:
> Change your method to be called
> GetNodeForAccessibleCheck() and just have it return a dom node for checking.
> Don't have it do a GetAccessibleFor() at the end. HandleEvent() will use this
> new method.
>
> Make a new GetAttachedAccessibleFor() which you'll use in DOM Inspector. The
> new method will use GetNodeForAccessibleCheck() followed by GetAccessibleFor()
> on that node. You can't use the GetAttachedAccessibleFor() convenience in
> nsRootAccessible::HandleEvent() because of the pagehide issue.
>
I added method GetNodeForParentAttachedAccessible() that return node of parent attached accessible if any. DOMi uses that method in conjunction with getAccessibleFor().
Attachment #243504 -
Attachment is obsolete: true
Attachment #243631 -
Flags: review?(aaronleventhal)
Comment 26•18 years ago
|
||
We've got to find a way to make this easier to understand.
I actually had 2 methods in mind, sorry if I didn't explain well:
1) A method to get the DOM node which should be used to get an accessible
2) A method to get the appropriate attached accessible, which could be used not only by DOMi but by any script. For example Fire Vox could use it. Would use method #1 and then GetAccessibleFor()
Too bad optional parameters are not possible for scriptability.
Assignee | ||
Comment 27•18 years ago
|
||
(In reply to comment #26)
> 2) A method to get the appropriate attached accessible, which could be used not
> only by DOMi but by any script. For example Fire Vox could use it. Would use
> method #1 and then GetAccessibleFor()
Actually DOMi doesn't need to get attached accessible, it need to check whether certain node is attached accessible. (Please look at patch, I added support of DOMi.) But I think it's nice to have method to get attached accessible for specific node. Though I think it will useless for DOMi. Am I wrong?
> Too bad optional parameters are not possible for scriptability.
>
Sorry?
Comment 28•18 years ago
|
||
Optional parameters, as in:
GetAccessibleFor(in nsIDOMNode domNode, in boolean isDetachedOkay = false);
Would be nice to have something like that for scripts.
Okay about DOMi not needing it.
BTW, I think your early exit is wrong in GetTargetNode for
if (nsevent) { return; }
Shouldn't the logic be reversed?
Assignee | ||
Comment 29•18 years ago
|
||
(In reply to comment #28)
> Optional parameters, as in:
> GetAccessibleFor(in nsIDOMNode domNode, in boolean isDetachedOkay = false);
> Would be nice to have something like that for scripts.
It will be big patch because I should change all c++ calls of GetAccessibleFor. Therefore I added one more method getAttachedAccessible for this (looks not bad :)). But if you like to extend GetAccessibleFor then I will do it.
> BTW, I think your early exit is wrong in GetTargetNode for
> if (nsevent) { return; }
> Shouldn't the logic be reversed?
>
Absolutely. I'm confused how my tests was passed.
Attachment #243631 -
Attachment is obsolete: true
Attachment #243794 -
Flags: review?(aaronleventhal)
Attachment #243631 -
Flags: review?(aaronleventhal)
Comment 30•18 years ago
|
||
Comment on attachment 243794 [details] [diff] [review]
patch8
Surkov is working on a new patch.
Attachment #243794 -
Flags: review?(aaronleventhal) → review-
Assignee | ||
Comment 31•18 years ago
|
||
Surkov accomplished the work on new patch :)
Attachment #243794 -
Attachment is obsolete: true
Attachment #243816 -
Flags: review?(aaronleventhal)
Updated•18 years ago
|
Attachment #243816 -
Flags: review?(aaronleventhal) → review+
Assignee | ||
Updated•18 years ago
|
Attachment #243816 -
Flags: superreview?(neil)
Assignee | ||
Updated•18 years ago
|
Blocks: xformsa11y
Comment 32•18 years ago
|
||
Comment on attachment 243816 [details] [diff] [review]
patch9
>+ nsCOMPtr<nsIDOMNode> relevantNode;
>+ nsresult rv =
>+ accService->GetRelevantContentNodeFor(eventTarget,
>+ getter_AddRefs(relevantNode));
>+
>+ if (NS_SUCCEEDED(rv) && relevantNode) {
>+ NS_ADDREF(*aTargetNode = relevantNode);
> return;
> }
You could save yourself a temporary by using *aTargetNode directly like this: if (NS_SUCCEEDED(accService->GetRelevantContentNodeFor(eventTarget, aTargetNode)) && *aTargetNode) return;
Attachment #243816 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Comment 33•18 years ago
|
||
Attachment #243816 -
Attachment is obsolete: true
Assignee | ||
Comment 34•18 years ago
|
||
(In reply to comment #24)
> 1.
> I just noticed in
> http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLGroupboxAccessiblensHTMLFormControlAccessible.cpp#523
> that we say allowsAnonChildren is PR_FALSE, but we don't impl
> GetAllowsAnonChildren() for that object. Seems like a bug. We can deal with
> that in a separate bug..
I'm not sure. GetAllowsAnonChildren() is inherited from nsAccessible that will return PR_TRUE. Is PR_TRUE valid for nsHTMLGroupboxAccessible?
Updated•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 35•18 years ago
|
||
Comment on attachment 243816 [details] [diff] [review]
patch9
>+ for (bindingParent = content->GetBindingParent(); bindingParent != nsnull;
>+ bindingParent = bindingParent->GetBindingParent()) {
>+ bindingsStack.AppendObject(bindingParent);
This doesn't actually work in all cases. So-called native anonymous elements are their own binding parent, so that the loop never exits and you exhaust available memory. I only hit this one because I have a patch that makes more XUL elements native anonymous (they aren't yet because of other problems).
Comment 36•18 years ago
|
||
I'm experiencing a hang with GetRelevantContentNodeFor() on the stack. It's eating up so much memory that I'm having trouble debugging it.
It seems to happen when I get a tooltip for the toolbar, or the a doc/root accessible. Maybe Neil's comment 35 should've been addressed in the patch?
Assignee | ||
Comment 37•18 years ago
|
||
I'll put another patch for neil's comment.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 38•18 years ago
|
||
*** Bug 360298 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 39•18 years ago
|
||
Attachment #245834 -
Flags: review?(hwaara)
Assignee | ||
Comment 40•18 years ago
|
||
Comment on attachment 245834 [details] [diff] [review]
hangs patch
Evan can you ensure that the patch fixes problem?
Attachment #245834 -
Flags: review?(hwaara) → review?(Evan.Yan)
Comment 41•18 years ago
|
||
Comment on attachment 245834 [details] [diff] [review]
hangs patch
I don't understand this part of code much, but it does fix the hang problem for me.
please ask someone who really understand the code for another review or sr.
Attachment #245834 -
Flags: review?(Evan.Yan) → review+
Assignee | ||
Updated•18 years ago
|
Attachment #245834 -
Flags: superreview?(neil)
Updated•18 years ago
|
Attachment #245834 -
Flags: superreview?(neil) → superreview+
Comment 42•18 years ago
|
||
Checked in.
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•