345780 - Support multiple targets for same relation

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

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.

Comment 1

[Aaron Leventhal]

Found an old bug for the multiple targets issue.

Comment 2

[Ginn Chen]

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

Comment 3

[Aaron Leventhal]

- 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.

Comment 4

[alexander :surkov (:asurkov)]

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.

Comment 5

[alexander :surkov (:asurkov)]

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.

Comment 6

[Aaron Leventhal]

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.

Comment 7

[alexander :surkov (:asurkov)]

Attachment: an idea

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

Comment 8

[Aaron Leventhal]

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

Comment 9

[Aaron Leventhal]

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.

Comment 10

[alexander :surkov (:asurkov)]

(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?

Comment 11

[Aaron Leventhal]

Are you sure it's not easier the other way around?

First introduce the relations cache, and then allow multiple targets?

Comment 12

[alexander :surkov (:asurkov)]

(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?

Comment 13

[Aaron Leventhal]

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.

Comment 14

[alexander :surkov (:asurkov)]

(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.

Comment 15

[Aaron Leventhal]

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).

Comment 16

[alexander :surkov (:asurkov)]

Attachment: in progress

Comment 17

[alexander :surkov (:asurkov)]

(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.

Comment 18

[alexander :surkov (:asurkov)]

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.

Comment 19

[Aaron Leventhal]

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

Comment 20

[alexander :surkov (:asurkov)]

(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.

Comment 21

[alexander :surkov (:asurkov)]

Attachment: in progress2

Comment 22

[alexander :surkov (:asurkov)]

Attachment: in progress3

Comment 23

[alexander :surkov (:asurkov)]

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.

Comment 24

[Aaron Leventhal]

How are non-ARIA relations more complicated?

Can you provide an example of the perf problem with tracking them?

Comment 25

[alexander :surkov (:asurkov)]

(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.

Comment 26

[alexander :surkov (:asurkov)]

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.

Comment 27

[Aaron Leventhal]

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.

Comment 28

[alexander :surkov (:asurkov)]

(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.

Comment 29

[alexander :surkov (:asurkov)]

bug 381599  is about relations cache.

Comment 30

[alexander :surkov (:asurkov)]

taking, skip relation caching

Comment 31

[alexander :surkov (:asurkov)]

Attachment: prepatch

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.

Comment 32

[alexander :surkov (:asurkov)]

(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 33

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 34

[Marco Zehe (:MarcoZ)]

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.

Comment 35

[Marco Zehe (:MarcoZ)]

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

Comment 36

[alexander :surkov (:asurkov)]

Attachment: patch2

with Marco's addressed comments

Comment 37

[David Bolter [:davidb] (NeedInfo me for attention)]

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)

Comment 38

[alexander :surkov (:asurkov)]

(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.

Comment 39

[alexander :surkov (:asurkov)]

(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 40

[alexander :surkov (:asurkov)]

Comment on attachment 360251 [details] [diff] [review]
patch2

Is there the work for super reviewer there? :)

Comment 41

[[e-mail]]

Comment on attachment 360251 [details] [diff] [review]
patch2

I didn't spot any errors.

Comment 42

[David Bolter [:davidb] (NeedInfo me for attention)]

(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 43

[Aaron Leventhal]

Comment on attachment 360251 [details] [diff] [review]
patch2

Current reviews are enough.

Comment 44

[Marco Zehe (:MarcoZ)]

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

Comment 45

[alexander :surkov (:asurkov)]

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.

Comment 46

[Marco Zehe (:MarcoZ)]

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