536344 - posinset and setsize aren't calculated right for the flatter trees

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Example from Hans

<table role="tree">
       <tr role="presentation"><td role="treeitem" aria-expanded="true" aria-level="1" aria-posinset="1" aria-setsize="3">vegetables</td></tr>
       <tr role="presentation"><td role="treeitem" aria-level="2" aria-posinset="1" aria-setsize="2">cucumber</td></tr>
       <tr role="presentation"><td role="treeitem" aria-level="2" aria-posinset="2" aria-setsize="2">carrot</td></tr>
       <tr role="presentation"><td role="treeitem" aria-expanded="false" aria-level="1" aria-posinset="2" aria-setsize="3">cars</td></tr>
       <tr role="presentation"><td role="treeitem" aria-level="2" aria-posinset="1" aria-setsize="3">mercedes</td></tr>
       <tr role="presentation"><td role="treeitem" aria-level="2" aria-posinset="2" aria-setsize="3">BMW</td></tr>
       <tr role="presentation"><td role="treeitem" aria-level="2" aria-posinset="1" aria-setsize="3">Audi</td></tr>
       <tr role="presentation"><td role="treeitem" aria-level="1" aria-posinset="3" aria-setsize="3">people</td></tr>
</table>

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

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

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

>+PRInt32
>+nsAccUtils::GetDefaultLevel(nsAccessible *aAcc)
>+{
>+  PRUint32 role = nsAccUtils::Role(aAcc);
>+
>+  if (role == nsIAccessibleRole::ROLE_OUTLINEITEM)
>+    return 1;
>+
>+  if (role == nsIAccessibleRole::ROLE_ROW) {
>+    nsCOMPtr<nsIAccessible> parent = aAcc->GetParent();
>+    if (Role(parent) == nsIAccessibleRole::ROLE_TREE_TABLE) {
>+      // It is a row inside flatten treegrid. Group level is always 1 until it

"flattened" (a few places in this patch).

>+      // is overriden by aria-level attribute.

"overridden"

>+PRInt32
>+nsAccUtils::GetARIAOrDefaultLevel(nsIAccessible *aAcc)

We could just call this "GetLevel" and add some comment(s).

(my review got cut off... to be continued)

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)

> >+PRInt32
> >+nsAccUtils::GetARIAOrDefaultLevel(nsIAccessible *aAcc)
> 
> We could just call this "GetLevel" and add some comment(s).

not sure because level = ARIA or calculated level, calculated > default.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch2

patch updated to trunk

Comment 5

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

Comment on attachment 421064 [details] [diff] [review]
patch2

r=me thanks! This might require a quick check with our vendors -- not sure. Also minor nits:

>@@ -3435,17 +3451,10 @@ nsAccessible::GetLevelInternal()
>       }
>     } else {
>       ++ level; // level is 1-index based
>     }
> 
>     return level;
>   }
> 
>-  if (role == nsIAccessibleRole::ROLE_ROW &&
>-      nsAccUtils::Role(parent) == nsIAccessibleRole::ROLE_TREE_TABLE) {
>-    // It is a row inside flatten treegrid. Group level is always 1 until it is
>-    // overriden by aria-level attribute.
>-    return 1;
>-  }
>-
>-  return 0;
>+  return level;

Nit: This looks a bit funny (could remove the first return level unless you are worried about someone adding code in between that and the second return).

>   /**
>+   * Convert attribute value of the give node to positive integer. If no

Nit: "given"

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)

> Nit: This looks a bit funny (could remove the first return level unless you are
> worried about someone adding code in between that and the second return).

If you like then I can use 'if else' statements here.

Comment 7

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

(In reply to comment #6)
> (In reply to comment #5)
> 
> > Nit: This looks a bit funny (could remove the first return level unless you are
> > worried about someone adding code in between that and the second return).
> 
> If you like then I can use 'if else' statements here.

No thanks. Why? :) To be clear I just think this looks funny:

    return level;
  }

 return level;
}

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > 
> > > Nit: This looks a bit funny (could remove the first return level unless you are
> > > worried about someone adding code in between that and the second return).
> > 
> > If you like then I can use 'if else' statements here.
> 
> No thanks. Why? :) To be clear I just think this looks funny:
> 
>     return level;
>   }
> 
>  return level;
> }

It is. 'else if' help to fix this.

Comment 9

[alexander :surkov (:asurkov)]

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