523068 - group attributes should be calculated from groupPosition()

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Now groupPosition() gets group attributes and transforms strings to int. If group attributes are calculated (there is no proper ARIA markup) then we convert ints to strings and strings into ints when groupPostion() is called. Many elements (like xul:tree, xul:listbox and etc) don't need an ARIA markup to expose group attributes, as well ARIA widgets don't use ARIA level and etc attributes if group attributes can be calculated from DOM. Therefore it's reasonable to make group attributes calculation depended on groupPostion() method.

Comment 1

[alexander :surkov (:asurkov)]

Also we should calculate group position iif proper ARIA attributes aren't used because now we calculate them and then apply ARIA attribute if any.

Comment 2

[alexander :surkov (:asurkov)]

I'll put here some ideas how to improve group position implementation in general which might be not related a lot with a bug summary.

We could use lazy calculation of setsize and posinset if we could assume some containers don't have separator accessibles. In this case we could get childCount and indexInParent. This might be applicable for non flat trees and lists. On another hand we could cache group info because I realize it might be not very performant to traverse big trees.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 4

[Marco Zehe (:MarcoZ)]

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

I saw a few misspellings of "position" in comments (like postion), but other than that looks good from my end!

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Nice, r=me, but please fix typos (as Marco mentioned) and check my comments below.

>-      setSize--;
>+      (*aSetSize)--;

Nice catch. This looks like an additional bug fix; we might want to add something to this bug title so that we can more easily recall when this happened (later).

>-  NS_ENSURE_TRUE(parent, NS_ERROR_FAILURE);
>+  NS_ENSURE_TRUE(parent,);

Why no second param? (you do this in a few places)

>+  PRInt32 level =
>+    parentContent->NodeInfo()->Equals(nsAccessibilityAtoms::optgroup) ? 2 : 1;

Is this always true?

>+PRInt32
>+nsHyperTextAccessible::GetLevelInternal()
>+{
>+  nsCOMPtr<nsIContent> content = nsCoreUtils::GetRoleContent(mDOMNode);
>+  NS_ENSURE_TRUE(content, 0);

Why 0?

I only skimmed the tests. The coverage looks okay.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Oh and please update uuids if/as necessary :)

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> (From update of attachment 419663 [details] [diff] [review])
> Nice, r=me, but please fix typos (as Marco mentioned) and check my comments
> below.
> 
> >-      setSize--;
> >+      (*aSetSize)--;
> 
> Nice catch. This looks like an additional bug fix; we might want to add
> something to this bug title so that we can more easily recall when this
> happened (later).

I think it doesn't if I get right. Previously we used local variable setSize and SetAccGroupAttrs. Now we return out parameter.

> 
> >-  NS_ENSURE_TRUE(parent, NS_ERROR_FAILURE);
> >+  NS_ENSURE_TRUE(parent,);
> 
> Why no second param? (you do this in a few places)

because return type is void

> 
> >+PRInt32
> >+nsHyperTextAccessible::GetLevelInternal()
> >+{
> >+  nsCOMPtr<nsIContent> content = nsCoreUtils::GetRoleContent(mDOMNode);
> >+  NS_ENSURE_TRUE(content, 0);
> 
> Why 0?

0 means no level.

Everywhere where the call failed but the method was changed to return void or integer type instead of nsresult then I saved console warnings and used the notation
NS_ENSURE_TRUE(condition, valueOfReturnType)

> 
> >+  PRInt32 level =
> >+    parentContent->NodeInfo()->Equals(nsAccessibilityAtoms::optgroup) ? 2 : 1;
> 
> Is this always true?

No. If parent is optgroup then it's true, if it's select then this is false.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 419663 [details] [diff] [review] [details])
> > Nice, r=me, but please fix typos (as Marco mentioned) and check my comments
> > below.
> > 
> > >-      setSize--;
> > >+      (*aSetSize)--;
> > 
> > Nice catch. This looks like an additional bug fix; we might want to add
> > something to this bug title so that we can more easily recall when this
> > happened (later).
> 
> I think it doesn't if I get right. Previously we used local variable setSize
> and SetAccGroupAttrs. Now we return out parameter.

I'm pretty sure setSize-- decs the pointer, and (*setSize)-- decs the value the pointer points to... but I might be tired.

> > >-  NS_ENSURE_TRUE(parent, NS_ERROR_FAILURE);
> > >+  NS_ENSURE_TRUE(parent,);
> > 
> > Why no second param? (you do this in a few places)
> 
> because return type is void

OK. Thanks.

> > >+PRInt32
> > >+nsHyperTextAccessible::GetLevelInternal()
> > >+{
> > >+  nsCOMPtr<nsIContent> content = nsCoreUtils::GetRoleContent(mDOMNode);
> > >+  NS_ENSURE_TRUE(content, 0);
> > 
> > Why 0?
> 
> 0 means no level.

OK. Thanks.

> Everywhere where the call failed but the method was changed to return void or
> integer type instead of nsresult then I saved console warnings and used the
> notation
> NS_ENSURE_TRUE(condition, valueOfReturnType)

Yes I caught that in my initial review :) I just wondered what 0 would mean. Thanks.

> > >+  PRInt32 level =
> > >+    parentContent->NodeInfo()->Equals(nsAccessibilityAtoms::optgroup) ? 2 : 1;
> > 
> > Is this always true?
> 
> No. If parent is optgroup then it's true, if it's select then this is false.

I meant is it true that the level should always really be 2 for optgroups and 1 for selects? :)

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)

> I'm pretty sure setSize-- decs the pointer, and (*setSize)-- decs the value the
> pointer points to... but I might be tired.

I think this is true but previously setSize was PRInt32 not a pointer.

> > > >+  PRInt32 level =
> > > >+    parentContent->NodeInfo()->Equals(nsAccessibilityAtoms::optgroup) ? 2 : 1;
> > > 
> > > Is this always true?
> > 
> > No. If parent is optgroup then it's true, if it's select then this is false.
> 
> I meant is it true that the level should always really be 2 for optgroups and 1
> for selects? :)

Ah, ok :) Actually 2nd level is for options inside of optgroup, 1st level is for options inside of select. But we have a bug concerning group position for option and optgroup. I just tried to save existing behaviour.

Comment 10

[Marco Zehe (:MarcoZ)]

(In reply to comment #8)
> > > >+    parentContent->NodeInfo()->Equals(nsAccessibilityAtoms::optgroup) ? 2 : 1;
> > > 
> > > Is this always true?
> > 
> > No. If parent is optgroup then it's true, if it's select then this is false.
> 
> I meant is it true that the level should always really be 2 for optgroups and 1
> for selects? :)

Yes I think so. In an option within an optgroup, the optgroup is sort of a heading, and the options below it are visually indented IIRC. So it makes sense, to make it easier for screen reader users, to put this on a higher level because these indentations would otherwise not be reflected.

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > I'm pretty sure setSize-- decs the pointer, and (*setSize)-- decs the value the
> > pointer points to... but I might be tired.
> 
> I think this is true but previously setSize was PRInt32 not a pointer.

Doh! OK thanks :)

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #10)
> (In reply to comment #8)
> > > > >+    parentContent->NodeInfo()->Equals(nsAccessibilityAtoms::optgroup) ? 2 : 1;
> > > > 
> > > > Is this always true?
> > > 
> > > No. If parent is optgroup then it's true, if it's select then this is false.
> > 
> > I meant is it true that the level should always really be 2 for optgroups and 1
> > for selects? :)
> 
> Yes I think so. In an option within an optgroup, the optgroup is sort of a
> heading, and the options below it are visually indented IIRC. So it makes
> sense, to make it easier for screen reader users, to put this on a higher level
> because these indentations would otherwise not be reflected.

OK cool. I seem to recall them being reported as siblings (in Accprobe tree view).

Comment 13

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/3202d6d22192