395223 - Positional descriptions incorrect for headings

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

We're exposing descriptions for level 1 headings as
L1, 1 of 1
and level 2 headings
L2, 2 of 2

In actual fact, we're not calculating the set size or position in set for the heading, and we either:
1) shouldn't expose it or 
2) should calculate it correctly

If we do #2, it could be very useful information, but not so simple. It's more complicated because heading levels may not be used correctly. And, for a given heading level n, you want the setsize/posinset to be calculated for just the headings in the local area, from the previous heading of an earlier level to the next heading of an earlier level. 

Suggested algorithm for #2,
Create a DOMTreeWalker with a DOMNodeFilter for headings <= current level.

1) Use walker to go back until earlier heading is found, counting posinset
2) setsize = posinset + 1 (current heading)
3) Use walker to go forward ntil next heading is found, adding to setsize for each

Comment 1

[Aaron Leventhal]

For now we could do #1, and save #2 for after Firefox 3.

Comment 2

[Marco Zehe (:MarcoZ)]

We're also exposing this kind of information as description for tree items, where the level is always given correctly, but the position is always "1 of 1", even when there's more than one item on the same hierarchical level under the same parent node. Should we broaden this bug, or file a separate one for the other instance?

Comment 3

[Aaron Leventhal]

Thanks Marco, good info. Let's broaden this bug.

Also broken for tree items.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> We're also exposing this kind of information as description for tree items,
> where the level is always given correctly, but the position is always "1 of 1",
> even when there's more than one item on the same hierarchical level under the
> same parent node. Should we broaden this bug, or file a separate one for the
> other instance?
> 

I tested bookmarks manager with accExplorer it exposes correct information. How did you get this?

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

headings, #1 approach

1) I do not see problems with trees
2) I'm not sure about #2 because it may be performance problem and it provides info easy unaccessible to usual users. Aaron, if you like to have this please file another bug.

Comment 6

[Marco Zehe (:MarcoZ)]

(In reply to comment #4)
> (In reply to comment #2)
> > We're also exposing this kind of information as description for tree items,
> > where the level is always given correctly, but the position is always "1 of 1",
> > even when there's more than one item on the same hierarchical level under the
> > same parent node. Should we broaden this bug, or file a separate one for the
> > other instance?
> I tested bookmarks manager with accExplorer it exposes correct information. How
> did you get this?

This must have been fixed very recently by a different patch. I now get correct information for trees in both Firefox Bookmarks Manager and the Thunderbird Folder List, where I didn't use to get correct information just last week.

So, let's ignore the Trees part and concentrate on headings.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> This must have been fixed very recently by a different patch. I now get correct
> information for trees in both Firefox Bookmarks Manager and the Thunderbird
> Folder List, where I didn't use to get correct information just last week.

You're right. It was bug 393033.

Comment 8

[Aaron Leventhal]

Can you describe the patch or add more comments please? Perhaps provide a no-whitespace-change patch (via cvs diff -w).

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch -w

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 280690 [details] [diff] [review]
patch -w

>Index: accessible/src/base/nsAccessibilityUtils.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/base/nsAccessibilityUtils.cpp,v
>retrieving revision 1.11
>diff -u -p -8 -w -r1.11 nsAccessibilityUtils.cpp
>--- accessible/src/base/nsAccessibilityUtils.cpp	5 Sep 2007 07:39:10 -0000	1.11
>+++ accessible/src/base/nsAccessibilityUtils.cpp	13 Sep 2007 02:40:38 -0000
>@@ -62,16 +62,18 @@
> #include "nsIInterfaceRequestorUtils.h"
> 
> static NS_DEFINE_IID(kRangeCID, NS_RANGE_CID);
> 
> void
> nsAccUtils::GetAccAttr(nsIPersistentProperties *aAttributes, nsIAtom *aAttrName,
>                        nsAString& aAttrValue)
> {
>+  aAttrValue.Truncate();
>+

otherwise GetAccGroupAttrs returns wrong results if there is not attributes, i.e. if level attribute is presented but posinset is not then now it returns posinset equealled level attribute.

the rest changes are described in comments :)

Comment 11

[Aaron Leventhal]

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

Okay, but two things
1) You changed similarItemsInGroup +1 to itemsInGroup (without +1). I assume that's because you're getting that from a different method, and you've verified it is correct. Right?
2) A nit:
s/arguements/arguments

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> (From update of attachment 280547 [details] [diff] [review])
> Okay, but two things
> 1) You changed similarItemsInGroup +1 to itemsInGroup (without +1). I assume
> that's because you're getting that from a different method, and you've verified
> it is correct. Right?

yes. It's a difference between groupPosition and setSize.

Comment 13

[alexander :surkov (:asurkov)]

checked in