374487 - provide API to support multiple targets for the same relation

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

The propose:

1) Add nsRelationService that will create nsAccessibleRelations object (that implements inteface like IAccessibleRelation) by relation type for the given accessible.
2) nsAccessible delegates to nsRelationService to get nsAccessibleRelation and implements inteface like IAccessible2 proposes.

I like to move out relation logic mostly from nsAccessible to decrease size of nsAccessible (3k of code lines is a big thing). But probably it's more correct to keep relation logic inside nsAccessible.

Aaron, what do you prefer?

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

it's API only similar with IA2 API. I need to do some reorg stuffs to support this API in more consistent way.

Comment 2

[alexander :surkov (:asurkov)]

Aaron, what's the review state?

Comment 3

[Aaron Leventhal]

This was removed but I think it needs to be added to nsIAccessibleStates:

  /**
   * API states we map from opposite states
   *   VISIBLE -- mapped as the opposite of INVISIBLE
   *   SHOWING -- mapped as the opposite of OFFSCREEN
   *
   * ATK states we don't have in nsIAccessible:
   *   ARMED -- no clear use case, used briefly when button is activated
   *   HAS_TOOLTIP -- no known use case
   *   ICONIFIED -- Mozilla does not have elements which are collapsable into icons
   *   TRUNCATED -- need use case. Indicates that an object's onscreen content is truncated, e.g. a text value in a spreadsheet cell. No IA2 state.
   */   

Comment 4

[Aaron Leventhal]

Comment on attachment 264357 [details] [diff] [review]
patch

Please remove GetRelationsCount() -- it's not something we want to duplicate from IA2. They need it because of how COM passes arrays. In our case we're just grabbing the array and getting the length anyway, and I don't want to hide that in our API.

For perf reasons, GetRelations() should use GetRelation() and not the other way around. Right now if I want just a single relation this code gets them all, which is expensive.

I don't see how this patch helps the original problem. GetAccessibleRelated() needs to be able to return an array of nsIAccessibles, otherwise we still only ever have a single target.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> This was removed but I think it needs to be added to nsIAccessibleStates:
> 
>   /**
>    * API states we map from opposite states
>    *   VISIBLE -- mapped as the opposite of INVISIBLE
>    *   SHOWING -- mapped as the opposite of OFFSCREEN
>    * ATK states we don't have in nsIAccessible:
>    *   ARMED -- no clear use case, used briefly when button is activated
>    *   HAS_TOOLTIP -- no known use case
>    *   ICONIFIED -- Mozilla does not have elements which are collapsable into
> icons
>    *   TRUNCATED -- need use case. Indicates that an object's onscreen content
> is truncated, e.g. a text value in a spreadsheet cell. No IA2 state.
>    */   
> 

It's ATK info and it is presented in atk/nsStateMap.h. Isn't it?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (From update of attachment 264357 [details] [diff] [review])
> Please remove GetRelationsCount() -- it's not something we want to duplicate
> from IA2. They need it because of how COM passes arrays. In our case we're just
> grabbing the array and getting the length anyway, and I don't want to hide that
> in our API.

If I would return GetRelationsCount() then I should remove GetRelation() too. If GetRelations() is all we need in gecko API then ok. Though I suppose it's not always fine to deal with array.

> For perf reasons, GetRelations() should use GetRelation() and not the other way
> around. Right now if I want just a single relation this code gets them all,
> which is expensive.

Yes. I'm agree but it's IA2 like API only. I need to redesign the way how we handle relations. If you prefer to see it now then it's fine with me, just a patch won't be small.

> I don't see how this patch helps the original problem. GetAccessibleRelated()
> needs to be able to return an array of nsIAccessibles, otherwise we still only
> ever have a single target.
> 

The patch adds nsAccessibleRelated that in future will handle multiple relations. I leaved this for following up patches.

The patch makes gecko API closer to IA2 API and adds bogus implementation of new API, no more. It's the first step only.

Comment 7

[Aaron Leventhal]

Yes, that's a good place for it -- atk/nsStateMap.h

Comment 8

[Aaron Leventhal]

> If I would return GetRelationsCount() then I should remove GetRelation() too.
> If GetRelations() is all we need in gecko API then ok. Though I suppose it's
> not always fine to deal with array.

Just to be clear, if we have to choose between GetRelation and GetRelations, I'd rather have GetRelation(), and build up array. Because if I want to get one relation I don't want to have to calculate the whole array.

If you want both that's also okay, but then I still want GetRelations() to call GetRelation() instead of the other way.

Comment 9

[alexander :surkov (:asurkov)]

Currently GetRelation() returns relation by index so I need to know number of relations and threfore GetRelation() calls GetRelations().

Comment 10

[Aaron Leventhal]

I see, it's not supposed to be by relation enum then. I made a wrong assumption.

IA2 is bad then. It means the AT developer needs to get all the relations just to get a specific one. That's very expensive.

It would be better to have API like GetAccessibleRelated() where you could pass in a relation type and get nsIAccessibleRelation back.

Comment 11

[Aaron Leventhal]

Comment on attachment 264357 [details] [diff] [review]
patch

Okay just make sure to move that one comment into atk/nsStateMap.h

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> (From update of attachment 264357 [details] [diff] [review])
> Okay just make sure to move that one comment into atk/nsStateMap.h
> 

ATK has that comment already http://lxr.mozilla.org/mozilla/source/accessible/src/atk/nsStateMap.h#44

Comment 13

[alexander :surkov (:asurkov)]

checked in