The default bug view has changed. See this FAQ.

Support multiple targets for same relation

VERIFIED FIXED

Status

()

Core
Disability Access APIs
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: Aaron Leventhal, Assigned: surkov)

Tracking

(Blocks: 3 bugs, {access})

Trunk
access
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Found an old bug for the multiple targets issue.
Blocks: 352105

Comment 2

11 years ago
Aaron, which relations should we return multiple targets at this time?
Or, you just want to make the interface change.
(Reporter)

Comment 3

11 years ago
- 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.
(Reporter)

Updated

10 years ago
Blocks: 368873
No longer blocks: 342901, 352105
(Assignee)

Comment 4

10 years ago
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.
Assignee: aaronleventhal → surkov.alexander
(Assignee)

Comment 5

10 years ago
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.
(Reporter)

Comment 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
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.
Attachment #270918 - Flags: review?(aaronleventhal)
(Reporter)

Comment 8

10 years ago
I'll look tomorrow -- in the mean time you might also look at bug 381599.
(Reporter)

Comment 9

10 years ago
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.
(Reporter)

Updated

10 years ago
Attachment #270918 - Flags: review?(aaronleventhal)
(Assignee)

Comment 10

10 years ago
(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?
(Reporter)

Comment 11

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

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

Comment 12

10 years ago
(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?
(Reporter)

Comment 13

10 years ago
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.
(Assignee)

Comment 14

10 years ago
(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.
(Reporter)

Comment 15

10 years ago
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).
(Assignee)

Comment 16

10 years ago
Created attachment 272375 [details] [diff] [review]
in progress
Attachment #270918 - Attachment is obsolete: true
(Assignee)

Comment 17

10 years ago
(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.
(Assignee)

Comment 18

10 years ago
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.
(Reporter)

Comment 19

10 years ago
I agree #1 is better but I think nsIMutationObserver methods might be called before doc is finished loading.
(Assignee)

Comment 20

10 years ago
(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.
(Assignee)

Comment 21

10 years ago
Created attachment 273756 [details] [diff] [review]
in progress2
Attachment #272375 - Attachment is obsolete: true
(Assignee)

Comment 22

10 years ago
Created attachment 273801 [details] [diff] [review]
in progress3
Attachment #273756 - Attachment is obsolete: true
(Assignee)

Comment 23

10 years ago
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.
(Reporter)

Comment 24

10 years ago
How are non-ARIA relations more complicated?

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

Comment 25

10 years ago
(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.
(Assignee)

Comment 26

10 years ago
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.
(Reporter)

Comment 27

10 years ago
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.
(Assignee)

Comment 28

10 years ago
(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.
(Reporter)

Updated

10 years ago
Blocks: 343213
(Assignee)

Comment 29

9 years ago
bug 381599  is about relations cache.
(Reporter)

Updated

8 years ago
Flags: wanted1.9.2?
(Assignee)

Comment 30

8 years ago
taking, skip relation caching
Status: NEW → ASSIGNED
(Assignee)

Comment 31

8 years ago
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.
Attachment #273801 - Attachment is obsolete: true
(Assignee)

Comment 32

8 years ago
(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.
(Assignee)

Updated

8 years ago
Blocks: 475297
(Assignee)

Comment 33

8 years ago
Created attachment 359238 [details] [diff] [review]
patch
Attachment #358730 - Attachment is obsolete: true
Attachment #359238 - Flags: review?(marco.zehe)
(Assignee)

Updated

8 years ago
Attachment #359238 - Flags: review?(david.bolter)

Updated

8 years ago
Attachment #359238 - Flags: review?(marco.zehe) → review+
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.
(Assignee)

Comment 36

8 years ago
Created attachment 360251 [details] [diff] [review]
patch2

with Marco's addressed comments
Attachment #359238 - Attachment is obsolete: true
Attachment #360251 - Flags: review?(david.bolter)
Attachment #359238 - Flags: review?(david.bolter)
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)
Attachment #360251 - Flags: review?(david.bolter) → review+
(Assignee)

Comment 38

8 years ago
(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.
(Assignee)

Comment 39

8 years ago
(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.
(Assignee)

Comment 40

8 years ago
Comment on attachment 360251 [details] [diff] [review]
patch2

Is there the work for super reviewer there? :)
Attachment #360251 - Flags: superreview?(neil)
Attachment #360251 - Flags: review?(aaronleventhal)

Updated

8 years ago
Attachment #360251 - Flags: superreview?(neil) → superreview+
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 :)
(Reporter)

Comment 43

8 years ago
Comment on attachment 360251 [details] [diff] [review]
patch2

Current reviews are enough.
Attachment #360251 - Flags: review?(aaronleventhal)
Pushed on Alexander's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/2681688f4cdf
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 45

8 years ago
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.