/** @brief Returns grouping information. Used for tree items, list items, tab panel labels, radio buttons, etc. Also used for collectons of non-text objects. @param [out] groupLevel 0-based @param [out] similarItemsInGroup 1-based @param [out] positionInGroup 0-based */ [propget] HRESULT groupPosition ( [out] long *groupLevel, [out] long *similarItemsInGroup, [out, retval] long *positionInGroup ); for example, if accessible object is for html:option then groupLevel is 0 if option is child of html:select, 1 if it is child of optgroup similarItemsInGroup - count of html:option of html:select or of html:optgroup positionInGroup - index in group Do I understand the method right?

Level should start at 1, so that it's the same as ARIA. I'll check with Pete Brunet on that. Note: we already expose these items using object attributes, called level, setsize and posinset. Fruit Grapes Bananas Oranges Pineapples Veggies Broccoli Carrots For "Bananas", the position in group is 2, the setsize is 4.

I sent an email to the IA2 list to ask that they all be 1-based, so that it's the same as ARIA.

To approve. Do we need expose level/pointset/setsize attributes via nsIAccessible::getAttributes?

Also msaa accessibleWrap looks for those ARIA attributes (http://lxr.mozilla.org/mozilla/source/accessible/src/msaa/nsAccessibleWrap.cpp#311), should it rather use nsIAccessible::getAttributes?

If it uses GetAttributes that would be more general probably, yeah. Also they don't want to change the IA2 values to be 1-based. So you will have to convert. ARIA posinset/level -- >=1 IA2 positionInGroup, level >= 0

(In reply to comment #3) > Created an attachment (id=257039) [details] > group attributes for htmlradio > > To approve. Do we need expose level/pointset/setsize attributes via > nsIAccessible::getAttributes? > I possibly don't need review actually right now, I just need to be sure I think correctly.

nit: extra space before NS_OK setsize should be the number of sibling radios in the same group. It's not the number of children of the current dom node. level doesn't need to be exposed if there are no levels. For IA2 level, ask Pete Brunet what to expose for level in cases like this when there are no levels. It might be useful for AT's to get magic value for no level (-1 or something).

Talkin with PeteB on irc. Summary about IA2::attributes and IA2::groupPosition: 1) expose posinset/sizeset/level for IA2::attributes for elemnets that it makes sense for 2) don't expose level for IA2::attributes for elements that have not levels like radio button 3) map IA2::attributes to groupPosition, if IA2::attributes hasn't level then groupLevel should be 0 4) headings expose level for IA2::attributes but they don't support IA2::groupPosition excepting one thing Pete doesn't sure that dublication of IA2::attributes and groupPosition is needed. Though he said: it doesn't hurt to have the extra data in IA2::attributes so it's not a big deal for me.

to be sure that all goes with you

(In reply to comment #7) > level doesn't need to be exposed if there are no levels. For IA2 level, ask > Pete Brunet what to expose for level in cases like this when there are no > levels. It might be useful for AT's to get magic value for no level (-1 or > something). > Also, Pete said: <surkov> so, if I have radio button (html:input type="radio"), does level makes sense for it? <PeteB> level=0 <PeteB> level>0 for trees <PeteB> or nested lists So it means groupLevel is compatible with ARIA level property.

I introduced here new utility class nsAccessibilityUitls to hold there all helpers methods. Now all helper methods holds nsAccessible but nsAccessible is too big already. This class helps to keep helpers together and reduce nsAccessible file size.

For this part of the code, which we do for all 6 heading levels, I didn'[t think the compiler would be smart enough to optimize it for us, which is why I assigned the int in each. What do you think? - PRInt32 headLevel = 0; - if (tag == nsAccessibilityAtoms::h1) { - headLevel = 1; - } + if (tag == nsAccessibilityAtoms::h1) + headLevel.AssignLiteral("1");

(In reply to comment #12) > For this part of the code, which we do for all 6 heading levels, I didn'[t > think the compiler would be smart enough to optimize it for us, which is why I > assigned the int in each. What do you think? > > - PRInt32 headLevel = 0; > - if (tag == nsAccessibilityAtoms::h1) { > - headLevel = 1; > - } > > + if (tag == nsAccessibilityAtoms::h1) > + headLevel.AssignLiteral("1"); > I'm not strong in compiler optimization issue? What's difference between those lines from point of view of optimization?

(In reply to comment #13) > I'm not strong in compiler optimization issue? I am sleeping. There is no any question actually.

Comment on attachment 257204 [details] [diff] [review] patch Surkov, can you submit a new patch that removes all the GetDescription() implementations in accesible/src/msaa. We can override nsAccessibleWrap::get_accDescription() in that once place, and implement nsIAccessible::GroupPosition() for all the appropriate widgets

(In reply to comment #15) > (From update of attachment 257204 [details] [diff] [review]) > Surkov, can you submit a new patch that removes all the GetDescription() > implementations in accesible/src/msaa. We can override > nsAccessibleWrap::get_accDescription() in that once place Ok. , and implement > nsIAccessible::GroupPosition() for all the appropriate widgets > Why? The patch maps this to GetAttributes(). Don't you like that?

> Why? The patch maps this to GetAttributes(). Don't you like that? Okay, that's fine. So then as long as we're providing the same or better information to get_accDescription() that we do now, and remove all the platform-dependent GetDescription() impls.

Comment on attachment 259433 [details] [diff] [review] patch3 I still like the headLevel integer code better. Few function calls need to be used, therefore it is smaller. Why do you like it this new way? There should be an nsAccessibleWrap::get_groupPosition() implementation which maps GroupPosition() results to IA2. // XXX: The role of an accessible can be pointed by ARIA attribute but // ARIA posinset, level, setsize may be skipped. Therefore we calculate // here these properties to map them into description. This should be // handled in cross-platform code. Yes, please move this to cross platform code so that ATK/AT-SPI gets the same info. Otherwise bug 374712 should not be marked as a duplicate. Maybe we should file a follow-up bug to support object attributes and GroupPosition() for ARIA widgets. + nsAccessible::GroupPosition(PRInt32 *aGroupLevel, + PRInt32 *aSimilarItemsInGroup, + PRInt32 *aPositionInGroup) + { ... + if (!posInSet && !setSize) + return NS_OK; + *aGroupLevel = level; I think it's possible to have just a level, in the case of headings. Unless we want to be clever and calculate the posinset and setsize for headings, which would be cool. That might be dificult in the cases where the author didn't nest heading levels as we'd expect. Since it's unclear, we can file a follow-up bug for that, like "What should we do for GroupPosition() on headings?" I don't want to hold up this patch for that. + *aPositionInGroup = posInSet--; I believe you will return the original value and the post-decrement will only affect the stack variable. Shouldn't it be a pre-decrement like this?: *aPositionInGroup = --posInSet; + *aSimilarItemsInGroup = setSize--; Same thing here, but also I believe it shouldn't be decremented at all. For example a setize of 1 means there is 1 item in the group, and it doesn't make sense to represent that as 0.

Comment on attachment 259433 [details] [diff] [review] patch3 Since I need to go on vacation now, let's get this in (with the off-by-1 fixes to GroupPosition). You can just file follow-up bugs for the other comments and work on them separately. Maybe you can get some reviews from Sun while I'm gone (back on April 5).

(In reply to comment #21) > There should be an nsAccessibleWrap::get_groupPosition() implementation which > maps GroupPosition() results to IA2. Ah, right. I forgot! :) > + *aPositionInGroup = posInSet--; > I believe you will return the original value and the post-decrement will only > affect the stack variable. > Shouldn't it be a pre-decrement like this?: > *aPositionInGroup = --posInSet; > > + *aSimilarItemsInGroup = setSize--; Fixed. > Same thing here, but also I believe it shouldn't be decremented at all. > For example a setize of 1 means there is 1 item in the group, and it doesn't > make sense to represent that as 0. it is because similarItemsInGroup equals setSize minus this accessible. I'll open new bug for other stuffs.

I filed bug 375534 for groupPosition problems.

checked in