201922 - Make XUL tree accessibles keep track of their own tree item accessibles

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

Make XUL tree accessibles keep track of their own tree item accessibles, so that:

1. When a tree accessible is shutdown, it can shutdown all of it's nodeless
accessibles that aren't in the cache.
2. When nsDocAccessible::HandleEvent() needs a tree item accessible, it should
ask the tree accessible for it. That way, it can reuse an old accessible rather
than create a new one.

Also, do tree item accessibles really need strong references to 
  nsCOMPtr<nsITreeBoxObject> mTree;
  nsCOMPtr<nsITreeView> mTreeView;

Finally, a xul tree needs to set it's comptrs to null at Shutdown()

Comment 1

[Aaron Leventhal]

Kyle, will you take this bug?

Comment 2

[Kyle Yuan]

okay, taking...

Comment 3

[Aaron Leventhal]

One problem that I see is that tree accessibles don't have mutation events since
they're not part of the DOM. So we won't be able to invalidate the tree's own
cache when the tree changes.

Is it possible to make the tree item accessibles not use any com ptrs or
anything needing a destructor in another module?

Comment 4

[Kyle Yuan]

Aaron, I'm back for this one. 

>One problem that I see is that tree accessibles don't have mutation events 
>since they're not part of the DOM. So we won't be able to invalidate the 
> tree's own cache when the tree changes.
Did Bug 149654 also fix the mutation events for tree? If it did not, how can I 
maintain the cache?

>Is it possible to make the tree item accessibles not use any com ptrs or
>anything needing a destructor in another module?
It's possible, we can use a helper method to get the mTree & 
mTreeView everytime we needed. Is that relative to the first problem?

Comment 5

[Aaron Leventhal]

Kyle, I don't know how you're going to watch for mutations/changes to the tree,
since there is not a dom node for every tree item.

Using the helper for each tree item, instead of the com ptrs on each, will
prevent us from crashing.

In that case, perhaps we can still create the tree items on demand. I would need
to see how that is working for MSAA events on tree items. The callback needs to
find the tree item for the event in the cache, based on the unique ID. I'm
afraid that we will keep caching new accessibles for the same tree item and
never get rid of them all until the document goes away.

Comment 6

[Kyle Yuan]

>Using the helper for each tree item, instead of the com ptrs on each, will
>prevent us from crashing.
I'm curious why com ptrs cause crash?

>we will keep caching new accessibles for the same tree item and
>never get rid of them all until the document goes away.
If we can't get benefit from the cache, why do we use it?

Comment 7

[Aaron Leventhal]

>Using the helper for each tree item, instead of the com ptrs on each, will
>prevent us from crashing.
I'm curious why com ptrs cause crash?
What happens is this: the external assitive technology application releases
their IAccessible's at the wrong time, which causes a destructor to be called in
layout or content, after those dll's have already been unloaded.

>we will keep caching new accessibles for the same tree item and
>never get rid of them all until the document goes away.
If we can't get benefit from the cache, why do we use it?
We get a lot of benefits from the cache, for everything except for tree items
when they do not have a DOM node. They're the only special thing like that,
becuase everything else gets a node. If we did not have a cache, we would have
accessibles that we had no pointers for, which we could not shut down when
document are destroyed, thus causing the crashes described above. 

My apologies for not explaining this very well. I can take this bug if you prefer.

Comment 8

[Kyle Yuan]

>If we can't get benefit from the cache...
Sorry, here I meant the cache for XUL tree. I don't doubt the advantage of
cache, it's great! I just thought that if we use a cache for XUL tree
accessible, we can do nothing other than keep putting the new pointer in it.

Comment 9

[Aaron Leventhal]

Cool :-)  

I thought we might need the XUL tree to keep an array of N xul tree item
accessibles (0 - n-1). Otherwise a new xul tree item might be created every time
there is an event on a XUL tree item. In this case, these would still be cached
in the main accessibility cache, using a key based on the accessible ptr. If we
don't keep track of these somewhere, there is no way to reuse or release them
when a new event happens. I think this means we keep creating a new accessible
for the same event on the same tree item.

Comment 10

[Louie Zhao]

Attachment: patch v1

This patch create cache for Treeitem.

Comment 11

[Aaron Leventhal]

I've learned a lot about tree support in MSAA recently, we're doing it wrong.
(We might be doing it right for ATK, I don't know).

In MSAA, all of the tree items are direct children of the tree node, even if
they're at different levels of hierarchy. The hierarchy level is exposed in the
value attribute. There aren't tree items with other tree items as msaa children.
Tree items that are currently hidden are simply not part of the current tree.
The MSAA tree node owns everything. You never get a separate IAccessible* for
the tree items. MSAA hands back the tree node's IAccessible* and a child number.

Comment 12

[Aaron Leventhal]

There are some changes to the implementation of XUL tree item:
http://groups.google.com/groups?q=tree+widget+changes+group:netscape.public.mozilla.xpfe.*&hl=en&lr=&ie=UTF-8&group=netscape.public.mozilla.xpfe.*&selm=408D292D.50402%40sympatico.ca&rnum=1

Comment 13

[Aaron Leventhal]

Related: autocomplete dropdowns use a XUL tree to display items and we're not
firing focus events for the items in it, as a user arrows through the choices.

Comment 14

[Aaron Leventhal]

(In reply to comment #11)
> In MSAA, all of the tree items are direct children of the tree node, even if
> they're at different levels of hierarchy. The hierarchy level is exposed in the 
> value attribute. There aren't tree items with other tree items as msaa children.
> Tree items that are currently hidden are simply not part of the current tree.
> The MSAA tree node owns everything. You never get a separate IAccessible* for
> the tree items. MSAA hands back the tree node's IAccessible* and a child number.
Turns out that the way we do it now is just fine, although to prevent leaks etc.
we need to finish this bug anyway.

Comment 15

[Aaron Leventhal]

Comment on attachment 144301 [details] [diff] [review]
patch v1

Conditional r=aaronl
1) I would like you to add Javadoc comments to nsIAccessibleTreeCache.idl like
we have in nsIAccessible.idl. In the future I want all of the .idl files in
mozilla/accessible to have such comments. The comment should mention that -1
can be passed in for the key column. That should also be commented in the
method itself.

2) Please change the name of getCachedTreeitemAccessible() to something like
getOrCreateTreeitemAccessible(). This makes it clear that an accessible will be
created if necessary.
3) Please use a named for the value 100 that you multiply the row by, something
like kMaxTreeColumns.
4) When you merge, be careful in nsRootAccessible so that the recent changes to
work with autocomplete's tree widgets still work (HandleEvent() doesn't call
GetTreeBoxObject() anymore)

Thanks for fixing this Louie.

Comment 16

[Aaron Leventhal]

Louie, I'm going to delay more work on bug 246063 until you check this in.

Comment 17

[Louie Zhao]

Attachment: updated patch

> 1) I would like you to add Javadoc comments to nsIAccessibleTreeCache.idl
like
> we have in nsIAccessible.idl. In the future I want all of the .idl files in
> mozilla/accessible to have such comments. The comment should mention that -1
> can be passed in for the key column. That should also be commented in the
> method itself.

Done

> 
> 2) Please change the name of getCachedTreeitemAccessible() to something like
> getOrCreateTreeitemAccessible(). This makes it clear that an accessible will
> be created if necessary.

Creating item in cache if no exists is a basic machanism of "CACHE" and can be
hidden from caller. I suggest to remain the original name.

> 3) Please use a named for the value 100 that you multiply the row by,
something

Done

> 4) When you merge, be careful in nsRootAccessible so that the recent changes
to
> work with autocomplete's tree widgets still work (HandleEvent() doesn't call
> GetTreeBoxObject() anymore)

Because of http://bugzilla.mozilla.org/show_bug.cgi?id=221619, I made some
upate to the patch.

Comment 18

[Aaron Leventhal]

Comment on attachment 150424 [details] [diff] [review]
updated patch

r=aaronl

I'll let you decide who you want to request sr= from.

Comment 19

[Kyle Yuan]

-> louie

Comment 20

[Aaron Leventhal]

Pkw mentioned that it might be useful to make the new nsIAccessibleTreeCache
interface scriptable, and I agree.

We might need to get the nsIAccessible for a tree item in an in-process
accessibility tool.

It should be easy, just remove [noscript].

Comment 21

[Aaron Leventhal]

Oh I see, you changed it to take an nsITreeColumn instead of a column number. Oh
well, I guess we'll deal with it later if we need to.

> Pkw mentioned that it might be useful to make the new nsIAccessibleTreeCache
> interface scriptable, and I agree.
> 
> We might need to get the nsIAccessible for a tree item in an in-process
> accessibility tool.
> 
> It should be easy, just remove [noscript].

Comment 22

[Louie Zhao]

Henry, Aaron has given "r" to the updated patch (Unfortuately, I override it due
to wrong operation). This patch is important because it blocks further work of
bug 246063. Can you spare some time to take a look at it? Thanks a lot.

Comment 23

[Henry Jia]

Comment on attachment 150424 [details] [diff] [review]
updated patch

There is only one function 'getCachedTreeitemAccessible' in
'nsIAccessibleTreeCache'. Do we really need this interface? Can we merge it
into other interface/class?

Comment 24

[Aaron Leventhal]

Another way would be to use
nsIAccessibleTable::CellRefAt (in long row, in long column, nsIAccessible
**aAccOut);

The only problem is that it's currently implemented for atk only. I think it
would be useful to move a lot of these atk-only interfaces into the base code so
that they could be used on all platforms.

Since that's something we want to do eventually, can we just use
nsIAccessibleTreeCache for now and then do this other improvement to use
cellRefAt() later?

I think we should get this in asap because it's blocking other tree work that I
have.

Comment 25

[Louie Zhao]

Thanks for sr & r. Patch checked in.