The way we expose relations allows only 1 relation target. An accessible object cannot point to multiple accessibles for a given relation. We need to fix nsIAccessible::GetAccessibleRelated() and consumers to support multiple end points, where it makes sense. One place is the EMBEDS relation. It's easy to imagine a XUL app with multiple content windows.

Found an old bug for the multiple targets issue.

Aaron, which relations should we return multiple targets at this time? Or, you just want to make the interface change.

- The EMBEDS relation on root accessible - describedby, descriptionfor, labelledby, labelfor, controlledby, controllerfor, whenever it's reasonable to calculate that without looking at the entire DOM tree.

I'd like to expose equivalent of IAccessibleRelation that looks a nice way to handle several relations of one and the same type of accessible. Also it allows to keep closer IA2 API and Gecko Accessibility API.

I filed new bug 374487 to provide API to support multiple targets for the same relation because implementation of that API is some different thing.

When we fix this we might want to allow multiple legends to label the same XUL groupbox. I've never seen it, but it's possible. See the testcase in bug 385024.

It's not a patch, it's just an idea. If it goes with you then I'll continue.

I'll look tomorrow -- in the mean time you might also look at bug 381599.

I think that this will need to change a lot because of bug 381599. Don't you think we should just do both at the same time? In my opinion we should wait until most other things stabilize. Without a relation cache, I think calculating multiple relations will often be too expensive. If we can't finish the relation cache for Firefox 3, then I can probably live with that. Correctness and stability for everything else is more important to me.

(In reply to comment #9) > I think that this will need to change a lot because of bug 381599. > > Don't you think we should just do both at the same time? I'm afraid it would be huge patch and it would be hard to search regressions if they will be. So probably to introduce multiple relations code but do not calculate more than one relation. It won't be hard I guess. Then we can introduce relation cache by parts and enable multiple relations counting. How does it sound?

Are you sure it's not easier the other way around? First introduce the relations cache, and then allow multiple targets?

(In reply to comment #11) > Are you sure it's not easier the other way around? > > First introduce the relations cache, and then allow multiple targets? > This depends on how we will introduce the cache. If the cache would be nsAccessibleRelation objects that we will update on mutation events then I guess no. If you have in mind some another cache structure then probably not. How do you see organization of cache?

We need to be able to look up an accessible in the cache and see if it has any targets. That means the hash table needs to use nsIAccessible* as the key, right? I'm a bit tired right now so I might be off.

(In reply to comment #13) > We need to be able to look up an accessible in the cache and see if it has any > targets. That means the hash table needs to use nsIAccessible* as the key, > right? I think we just should have a cache of nsAccessibleRelations on every accessible. Since most of relations are paired then we would be able to update two relations at the same time ('by' relation and 'for' relation). When 'for' relation is changed then we update 'for' and 'by' relations together.

I think so too. On most accessibles there is no relation and it will just cost 4 bytes 4 the null pointer. It would be nice if there is a way to save the 4 bytes in the case of an nsHTMLTextAccessible (an HTML text node is not created via an element, has no ATK node, and cannot have a relation or be pointed to in a relation).

(In reply to comment #15) > It would be nice if there is a way to save the 4 bytes in the case of an > nsHTMLTextAccessible (an HTML text node is not created via an element, has no > ATK node, and cannot have a relation or be pointed to in a relation). > Unfortunately I can't see a way to do this with current hierarchy of accessible classes. I think we should have bug for this.

I think methods nsIMutationObserver won't be called until document is loaded. So we do not build relations by nsIMutationObserver usage. I can see two ways: 1) inspect whole document after load and build relations 2) build relations for certain accessible when relations are requested. 1) is better than 2) because relations for and by will be 1-1 2) is better than 1) because 2) won't be performance issue duiring inspecting of accessible document.

I agree #1 is better but I think nsIMutationObserver methods might be called before doc is finished loading.

(In reply to comment #19) > I agree #1 is better but I think nsIMutationObserver methods might be called > before doc is finished loading. > I asked before they said it won't be called. But in any way if it will be called then it won't broke us.

I start to think it's possibly not very good idea to cache all accessible relations because non ARIA accessible relations may be much complicated and it can be a performance problem to track them. Though if we won't track some accessible relation then we can do not get 1-1 relation between accessible relations. A kind of tricky bug.

How are non-ARIA relations more complicated? Can you provide an example of the perf problem with tracking them?

(In reply to comment #24) > How are non-ARIA relations more complicated? > > Can you provide an example of the perf problem with tracking them? > To update ARIA relations we should traverse down the inserted/removed tree and check attributes. Non ARIA relation (for example, to update label_by relation for html groupbox accessible (http://lxr.mozilla.org/mozilla/source/accessible/src/html/nsHTMLFormControlAccessible.cpp#634) we should traverse whole document tree to ensure nodes in inserted/removed tree do not influence on label_by relation of html groupbox accessible.

Probably we can traverse accessible tree but I can't think generic algorithm how to check whether traversed accessible isn't a relation accessible for some another accessible. Algorithm is following. 1) we have paired relations (let's call them direct relation and connected relation), for example, label_by/label_for) 2) direct relation describes how to calculate connected relation 3) connected relation doesn't know how to calculate direct relation 4) when direct relation is changed then calculated is changed too. It works good for ARIA because we easy can check attributes on DOM mutation callbacks. But non-ARIA direct relation may describe the rule how to get connected relation by the following ways: 1) child node in the subtree of specific tagname (or something similar) 2) accessible of certain role in accessible subtree So probably for non-ARIA relations we should require that connected relation should have a rule to compute a direct accessible. The problem is it may be hard to define 1-1 (reversible) rules. If we would have reversible rules for paired relations then when something is changed in tree we should can find all accessibles and check relations.

We should still create accessibles for objects that are display:none when they are pointed to by a relation. I think we should fix that at the same time. Just make sure they have SHOWING/HIDDEN states set correctly.

(In reply to comment #27) > We should still create accessibles for objects that are display:none when they > are pointed to by a relation. I think we should fix that at the same time. Just > make sure they have SHOWING/HIDDEN states set correctly. > Another good point I missed. Probably we can create inactive relation that we won't expose to AT and when accessible will be shown then activate it. Or probably we should update relations too when accessible is created.

bug 381599 is about relations cache.

taking, skip relation caching

Relation caching looks to be complex tasks and in the meantime I'm not sure relation caching is good for all types of relations. I think it's worth to introduce multiple relations now so that we won't much regress from the point view of performance. This patch exposes multiple relations for ARIA properties only, for example, when aria-labelledby points to several elements. Firefox has couple of examples where we use this. As well I think we should change our code for multiple relation support, I believe this will help to introduce relation caching correctly taking into account multiple relations peculiarities. This patch is not tested. I will add mochitests and ask for reviews.

(In reply to comment #31) > This patch exposes multiple relations for ARIA properties > only not only but mostly. I didn't mentioned derived class implementations but I think it shouldn't bring any performance problems.

Comment on attachment 359238 [details] [diff] [review] patch A few things I noticed: > /** >+ * >+ */ >+ static void GetElementsByIDRefsAttr(nsIContent *aContent, nsIAtom *aAttr, >+ nsIArray **aRefElements); Missing documentation. >+ @ @return an accessible Should be * @..., not @ @... >+ /** >+ * >+ */ >+ static nsresult AddTargetFromContent(PRUint32 aRelationType, >+ nsIAccessibleRelation **aRelation, >+ nsIContent *aContent); >+ /** >+ * >+ */ >+ static nsresult AddTargetFromIDRefAttr(PRUint32 aRelationType, >+ nsIAccessibleRelation **aRelation, >+ nsIContent *aContent, nsIAtom *aAttr); Also missing documentation. One question to the test part: You're only testing ARIA here. Do we also expose relations on real controls like xul:tree, html:select/html:option elements etc.? Should we test for this too? For ARIA, the tests look fine. r=me for that.

Oops, nevermind my last question, forgot about bug 475325 and bug 475330.

with Marco's addressed comments

Comment on attachment 360251 [details] [diff] [review] patch2 Wow. >+nsresult >+nsRelUtils::AddTargetFromNeighbour(PRUint32 aRelationType, >+ nsIAccessibleRelation **aRelation, >+ nsIContent *aContent, >+ nsIAtom *aNeighboutAttr, Nit: find and replace aNeighbout with "aNeighbour". Summary Surkov this is a huge patch and I gone over it twice and haven't caught anything else. I'm not sure I saw Aaron's comment 27 addressed here, is that intentional? I'm inclined to r=me but if there was an r "sortof=" me I would do that. If you want me to go over it again r? me again and I will. Your tests are decent. BTW are we going to hold off on the relations cache (bug 381599)? (Maybe we should plan to do some profiling in the Spring/Summer)

(In reply to comment #37) > Summary > Surkov this is a huge patch and I gone over it twice and haven't caught > anything else. I'm not sure I saw Aaron's comment 27 addressed here, is that > intentional? No, I didn't address this. At this point I didn't change code logic, just provided a way to keep multiple relations. I'll file bug for this. > I'm inclined to r=me but if there was an r "sortof=" me I would do that. If you > want me to go over it again r? me again and I will. Sorry, could you "translate" this? > BTW are we going to hold off on the relations cache (bug 381599)? (Maybe we > should plan to do some profiling in the Spring/Summer) I consider relations cache as not only performance issue but as ability to keep relations holistic. I believe profiling is a great thing.

(In reply to comment #38) > (In reply to comment #37) > > > Summary > > Surkov this is a huge patch and I gone over it twice and haven't caught > > anything else. I'm not sure I saw Aaron's comment 27 addressed here, is that > > intentional? > > No, I didn't address this. At this point I didn't change code logic, just > provided a way to keep multiple relations. I'll file bug for this. bug 476986. thanks for the catch.

Comment on attachment 360251 [details] [diff] [review] patch2 Is there the work for super reviewer there? :)

Comment on attachment 360251 [details] [diff] [review] patch2 I didn't spot any errors.

(In reply to comment #38) > (In reply to comment #37) > > > Summary > > Surkov this is a huge patch and I gone over it twice and haven't caught > > anything else. I'm not sure I saw Aaron's comment 27 addressed here, is that > > intentional? > > No, I didn't address this. At this point I didn't change code logic, just > provided a way to keep multiple relations. I'll file bug for this. > > > I'm inclined to r=me but if there was an r "sortof=" me I would do that. If you > > want me to go over it again r? me again and I will. > > Sorry, could you "translate" this? > Sorry. I was just offering to look the patch over again, but given that neil hasn't found any problem either I feel a lot more confident about my r=me :) I'm not sure we need to wait for Aaron's review, since you spawned a bug for the hidden node relations. I say push at will :)

Comment on attachment 360251 [details] [diff] [review] patch2 Current reviews are enough.

Pushed on Alexander's behalf in changeset: http://hg.mozilla.org/mozilla-central/rev/2681688f4cdf

Unfortunately I didn't put latest patch with David's nit fixed "Nit: find and replace aNeighbout with "aNeighbour".". I'll keep this in mind and fix on next iteration of a11y relations bugs.

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090210 Minefield/3.2a1pre