GetAccessibleFor() doesn't look if ally nodes are allowed inside anon content

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({access})

Trunk
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 9 obsolete attachments)

(Assignee)

Description

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

12 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

12 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

12 years ago
Okay, maybe you're write.

BTW, it's the tree walker which uses GetAccessibleFor()

Comment 4

12 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

12 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

12 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

12 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

12 years ago
Blocks: 344896
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Assignee: aaronleventhal → surkov.alexander
Status: ASSIGNED → NEW
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 8

12 years ago
Created attachment 235627 [details] [diff] [review]
patch

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

12 years ago
Aaron, what's about the patch approach?
(Assignee)

Comment 10

12 years ago
Created attachment 242789 [details] [diff] [review]
updated
Attachment #235627 - Attachment is obsolete: true
Attachment #242789 - Flags: review?(aaronleventhal)
Attachment #235627 - Flags: review?(aaronleventhal)
(Assignee)

Comment 11

12 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

12 years ago
Created attachment 242790 [details] [diff] [review]
updated2

Sorry. Previously I attahced old one.
Attachment #242789 - Attachment is obsolete: true
Attachment #242790 - Flags: review?(aaronleventhal)
Attachment #242789 - Flags: review?(aaronleventhal)

Comment 13

12 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

12 years ago
Attachment #242790 - Flags: review?(aaronleventhal) → review-
(Assignee)

Comment 14

12 years ago
Created attachment 243478 [details] [diff] [review]
patch4

Will it go?
Attachment #242790 - Attachment is obsolete: true
Attachment #243478 - Flags: review?(aaronleventhal)
(Assignee)

Comment 15

12 years ago
(In reply to comment #14)
> Created an attachment (id=243478) [edit]
> patch4
> 
> Will it go?
> 

I forgot to update guid.

Comment 16

12 years ago
What callers will you have? nsRootAccessible::GetTargetNode and DOMi?
(Assignee)

Comment 17

12 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

12 years ago
Can you put the caller support into the patch to and I can r= all at the same time?
(Assignee)

Comment 19

12 years ago
Created attachment 243489 [details] [diff] [review]
patch5

I leaved presentation shell #0 using
Attachment #243478 - Attachment is obsolete: true
Attachment #243489 - Flags: review?(aaronleventhal)
Attachment #243478 - Flags: review?(aaronleventhal)

Comment 20

12 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

12 years ago
Created attachment 243504 [details] [diff] [review]
patch6

all in one
Attachment #243489 - Attachment is obsolete: true
Attachment #243504 - Flags: review?(aaronleventhal)

Comment 22

12 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

12 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

12 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

12 years ago
Attachment #243504 - Flags: review?(aaronleventhal) → review-
(Assignee)

Comment 25

12 years ago
Created attachment 243631 [details] [diff] [review]
patch7

(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

12 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

12 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

12 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

12 years ago
Created attachment 243794 [details] [diff] [review]
patch8

(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

12 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

12 years ago
Created attachment 243816 [details] [diff] [review]
patch9

Surkov accomplished the work on new patch :)
Attachment #243794 - Attachment is obsolete: true
Attachment #243816 - Flags: review?(aaronleventhal)

Updated

12 years ago
Attachment #243816 - Flags: review?(aaronleventhal) → review+
(Assignee)

Updated

12 years ago
Attachment #243816 - Flags: superreview?(neil)
(Assignee)

Updated

12 years ago
Blocks: 337249

Comment 32

12 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

12 years ago
Created attachment 243935 [details] [diff] [review]
check in
Attachment #243816 - Attachment is obsolete: true
(Assignee)

Comment 34

12 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

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 35

12 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

12 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

12 years ago
I'll put another patch for neil's comment.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 38

12 years ago
*** Bug 360298 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 39

12 years ago
Created attachment 245834 [details] [diff] [review]
hangs patch
Attachment #245834 - Flags: review?(hwaara)
(Assignee)

Comment 40

12 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

12 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

12 years ago
Attachment #245834 - Flags: superreview?(neil)

Updated

12 years ago
Attachment #245834 - Flags: superreview?(neil) → superreview+

Comment 42

12 years ago
Checked in.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.