spun off bug 371594 1) do we need to support groupPosition for heading. Now headings expose ARIA level attribute but should they have setsize and posinset? Probably they shouldn't because a) we need to traverse whole document to expose them (performance issue) b) often users can determine visually what is setsize and posinset if they won't look through enetery document therefore it looks AT shouldn't gather this info. 2) bug 371594 comment 13: (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? and bug 371594 comment 21: "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?" I still don't understand how can I do it be different way. 3) bug 371594 comment 21: // 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. I guess we should move msaa code that calculates setsize and posinset into cross platform code.

1) Okay, don't do it (at least for now). I think the code could be simple though. Do a GetElementsByTagName for that heading level. The size of the returned array is setsize, and you can iterate through the array looking for the current heading for posinset. But we don't know if the AT vendors want that. 2) The optimization issue is more of code size, since speed isn't a problem here. Setting an integer is 1 line of machine code. Calling a method with a string argument means creating the string somewhere, pushing the argument on the stack, calling the method, and popping the argument off the stack. About 20 extra bytes let's say. Multiple the extra code by 6 and you probably have 120 extra bytes of code. It's fine if you leave it, but I wanted to mention it because I didn't understand what the change was for when it has the disadvantage (unless the compiler is a lot smarter than I give it credit for). 3) Good, fine.

2) and 3) plus ARIA attributes applied after attributes for certain accessible.

Comment on attachment 260848 [details] [diff] [review] patch No obvious problems. If you want you can factor out the manually calculated posinset/setsize logic inside nsAccessible::GetAttributes(). But r+ either way.

Comment on attachment 260848 [details] [diff] [review] patch checked in

(In reply to comment #4) > (From update of attachment 260848 [details] [diff] [review]) > No obvious problems. If you want you can factor out the manually calculated > posinset/setsize logic inside nsAccessible::GetAttributes(). I didn't get, manually calculated posinset/setsize/level login is inside nsAccessible::GetAttributes(). Where should it be?