Closed Bug 523068 Opened 10 years ago Closed 10 years ago

group attributes should be calculated from groupPosition()

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(1 file)

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.
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.
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.
Blocks: groupa11y
Attached patch patchSplinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #419663 - Flags: review?(marco.zehe)
Attachment #419663 - Flags: review?(bolterbugz)
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!
Attachment #419663 - Flags: review?(marco.zehe) → review+
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.
Attachment #419663 - Flags: review?(bolterbugz) → review+
Oh and please update uuids if/as necessary :)
(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.
(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? :)
(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.
(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.
(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 :)
(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).
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.