Last Comment Bug 345780 - Support multiple targets for same relation
: Support multiple targets for same relation
Status: VERIFIED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: aria ia2 rela11y
  Show dependency treegraph
 
Reported: 2006-07-24 14:55 PDT by Aaron Leventhal
Modified: 2009-02-11 10:09 PST (History)
4 users (show)
aaronlev: wanted1.9.2?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
an idea (17.16 KB, patch)
2007-07-04 09:31 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
in progress (39.84 KB, patch)
2007-07-14 23:58 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
in progress2 (54.72 KB, patch)
2007-07-25 02:13 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
in progress3 (62.10 KB, patch)
2007-07-25 10:20 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
prepatch (72.77 KB, patch)
2009-01-25 11:00 PST, alexander :surkov
no flags Details | Diff | Splinter Review
patch (91.67 KB, patch)
2009-01-28 02:16 PST, alexander :surkov
mzehe: review+
Details | Diff | Splinter Review
patch2 (93.55 KB, patch)
2009-02-02 22:14 PST, alexander :surkov
dbolter: review+
neil: superreview+
Details | Diff | Splinter Review

Description Aaron Leventhal 2006-07-24 14:55:41 PDT
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 2006-09-13 12:57:20 PDT
Found an old bug for the multiple targets issue.
Comment 2 Ginn Chen 2006-09-20 01:56:16 PDT
Aaron, which relations should we return multiple targets at this time?
Or, you just want to make the interface change.
Comment 3 Aaron Leventhal 2006-09-20 06:53:03 PDT
- 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 2007-02-25 00:00:08 PST
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 2007-03-19 10:35:15 PDT
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 2007-06-29 03:47:00 PDT
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 2007-07-04 09:31:55 PDT
Created attachment 270918 [details] [diff] [review]
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 2007-07-04 13:57:54 PDT
I'll look tomorrow -- in the mean time you might also look at bug 381599.
Comment 9 Aaron Leventhal 2007-07-05 01:21:30 PDT
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 2007-07-05 03:01:30 PDT
(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 2007-07-05 03:45:22 PDT
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 2007-07-05 03:53:29 PDT
(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 2007-07-05 04:30:42 PDT
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 2007-07-13 01:57:19 PDT
(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 2007-07-13 09:29:16 PDT
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 2007-07-14 23:58:33 PDT
Created attachment 272375 [details] [diff] [review]
in progress
Comment 17 alexander :surkov 2007-07-15 00:00:04 PDT
(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 2007-07-15 00:05:37 PDT
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 2007-07-23 03:09:43 PDT
I agree #1 is better but I think nsIMutationObserver methods might be called before doc is finished loading.
Comment 20 alexander :surkov 2007-07-24 00:32:11 PDT
(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 2007-07-25 02:13:21 PDT
Created attachment 273756 [details] [diff] [review]
in progress2
Comment 22 alexander :surkov 2007-07-25 10:20:13 PDT
Created attachment 273801 [details] [diff] [review]
in progress3
Comment 23 alexander :surkov 2007-07-25 10:40:20 PDT
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 2007-07-27 03:48:15 PDT
How are non-ARIA relations more complicated?

Can you provide an example of the perf problem with tracking them?
Comment 25 alexander :surkov 2007-07-27 05:15:38 PDT
(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 2007-07-27 05:28:03 PDT
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 2007-07-27 07:25:19 PDT
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 2007-07-27 09:38:34 PDT
(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 2008-11-24 19:37:34 PST
bug 381599  is about relations cache.
Comment 30 alexander :surkov 2009-01-25 07:27:42 PST
taking, skip relation caching
Comment 31 alexander :surkov 2009-01-25 11:00:00 PST
Created attachment 358730 [details] [diff] [review]
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 2009-01-25 11:03:46 PST
(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 2009-01-28 02:16:41 PST
Created attachment 359238 [details] [diff] [review]
patch
Comment 34 Marco Zehe (:MarcoZ) 2009-02-02 08:40:47 PST
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) 2009-02-02 08:43:04 PST
Oops, nevermind my last question, forgot about bug 475325 and bug 475330.
Comment 36 alexander :surkov 2009-02-02 22:14:35 PST
Created attachment 360251 [details] [diff] [review]
patch2

with Marco's addressed comments
Comment 37 David Bolter [:davidb] 2009-02-04 11:39:49 PST
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 2009-02-04 20:42:06 PST
(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 2009-02-04 20:46:15 PST
(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 2009-02-04 20:47:55 PST
Comment on attachment 360251 [details] [diff] [review]
patch2

Is there the work for super reviewer there? :)
Comment 41 neil@parkwaycc.co.uk 2009-02-05 04:40:14 PST
Comment on attachment 360251 [details] [diff] [review]
patch2

I didn't spot any errors.
Comment 42 David Bolter [:davidb] 2009-02-06 07:08:39 PST
(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 2009-02-09 03:37:36 PST
Comment on attachment 360251 [details] [diff] [review]
patch2

Current reviews are enough.
Comment 44 Marco Zehe (:MarcoZ) 2009-02-10 02:04:50 PST
Pushed on Alexander's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/2681688f4cdf
Comment 45 alexander :surkov 2009-02-10 05:05:19 PST
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) 2009-02-10 23:21:06 PST
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090210 Minefield/3.2a1pre

Note You need to log in before you can comment on or make changes to this bug.