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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: access)

Attachments

(2 files, 9 obsolete files)

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().
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
(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.
Okay, maybe you're write. BTW, it's the tree walker which uses GetAccessibleFor()
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.
(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?
(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?
(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?
Blocks: 344896
Status: NEW → ASSIGNED
Assignee: aaronleventhal → surkov.alexander
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
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)
Aaron, what's about the patch approach?
Attached patch updated (obsolete) — Splinter Review
Attachment #235627 - Attachment is obsolete: true
Attachment #242789 - Flags: review?(aaronleventhal)
Attachment #235627 - Flags: review?(aaronleventhal)
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.
Attached patch updated2 (obsolete) — Splinter Review
Sorry. Previously I attahced old one.
Attachment #242789 - Attachment is obsolete: true
Attachment #242790 - Flags: review?(aaronleventhal)
Attachment #242789 - Flags: review?(aaronleventhal)
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.
Attachment #242790 - Flags: review?(aaronleventhal) → review-
Attached patch patch4 (obsolete) — Splinter Review
Will it go?
Attachment #242790 - Attachment is obsolete: true
Attachment #243478 - Flags: review?(aaronleventhal)
(In reply to comment #14) > Created an attachment (id=243478) [edit] > patch4 > > Will it go? > I forgot to update guid.
What callers will you have? nsRootAccessible::GetTargetNode and DOMi?
(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.
Can you put the caller support into the patch to and I can r= all at the same time?
Attached patch patch5 (obsolete) — Splinter Review
I leaved presentation shell #0 using
Attachment #243478 - Attachment is obsolete: true
Attachment #243489 - Flags: review?(aaronleventhal)
Attachment #243478 - Flags: review?(aaronleventhal)
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+
Attached patch patch6 (obsolete) — Splinter Review
all in one
Attachment #243489 - Attachment is obsolete: true
Attachment #243504 - Flags: review?(aaronleventhal)
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
(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.
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.
Attachment #243504 - Flags: review?(aaronleventhal) → review-
Attached patch patch7 (obsolete) — Splinter Review
(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)
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.
(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?
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?
Attached patch patch8 (obsolete) — Splinter Review
(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 on attachment 243794 [details] [diff] [review] patch8 Surkov is working on a new patch.
Attachment #243794 - Flags: review?(aaronleventhal) → review-
Attached patch patch9 (obsolete) — Splinter Review
Surkov accomplished the work on new patch :)
Attachment #243794 - Attachment is obsolete: true
Attachment #243816 - Flags: review?(aaronleventhal)
Attachment #243816 - Flags: review?(aaronleventhal) → review+
Attachment #243816 - Flags: superreview?(neil)
Blocks: xformsa11y
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+
Attached patch check inSplinter Review
Attachment #243816 - Attachment is obsolete: true
(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?
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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).
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?
I'll put another patch for neil's comment.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
*** Bug 360298 has been marked as a duplicate of this bug. ***
Attached patch hangs patchSplinter Review
Attachment #245834 - Flags: review?(hwaara)
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 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+
Attachment #245834 - Flags: superreview?(neil)
Attachment #245834 - Flags: superreview?(neil) → superreview+
Checked in.
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: