Closed Bug 536344 Opened 10 years ago Closed 10 years ago

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

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, 1 obsolete file)

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>
Blocks: groupa11y
Summary: posinset and setsize isn't calculated right for the flatter trees → posinset and setsize aren't calculated right for the flatter trees
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #420487 - Flags: review?(marco.zehe)
Attachment #420487 - Flags: review?(bolterbugz)
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)
(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.
Attached patch patch2Splinter Review
patch updated to trunk
Attachment #420487 - Attachment is obsolete: true
Attachment #421064 - Flags: review?(marco.zehe)
Attachment #421064 - Flags: review?(bolterbugz)
Attachment #420487 - Flags: review?(marco.zehe)
Attachment #420487 - Flags: review?(bolterbugz)
Attachment #421064 - Flags: review?(bolterbugz) → review+
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"
(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.
(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;
}
(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.
Attachment #421064 - Flags: review?(marco.zehe) → review+
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/b036d913b0bd
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.