443881 - take into account separators in xul menus when group attributes are calculating

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

take into account separators in xul menus when group attributes are calculating

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Marco Zehe (:MarcoZ)]

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

r=me for the functionality part, and from what I know of the code, it also looks good. But we'll count on Neil's sr to catch anything obvious.

Comment 3

[[e-mail]]

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

>-    nsCOMPtr<nsIDOMXULElement> currItem;
>-    container->GetItemAtIndex(index, getter_AddRefs(currItem));
>-    nsCOMPtr<nsIDOMNode> currNode(do_QueryInterface(currItem));
>+    nsCOMPtr<nsIDOMXULElement> item;
>+    container->GetItemAtIndex(index, getter_AddRefs(item));
>+    nsCOMPtr<nsIDOMNode> currNode(do_QueryInterface(item));
I'm not sure why you bothered with the rename.
I feel sure I've said this before, but I feel you should be able to pass item to GetAccessibleFor without having to explicitly QI it to nsIDOMNode first.

>+    PRUint32 itemRole = nsAccessible::Role(itemAcc);
itemAcc could be null, is Role null-safe?

>+    if (itemRole == nsIAccessibleRole::ROLE_SEPARATOR) {
>+      // It's not our group. Continue the searching.
I'd say // We need to start a new group.

>+      continue;
>     }
>+
>+    if (itemAcc && !(nsAccessible::State(itemAcc) &
>+                     nsIAccessibleStates::STATE_INVISIBLE)) {
I'm tempted to suggest using } else if instead of continue; } if although there's not much difference in readability.

I actually think it would be more efficient to calculate the position in set by starting at the child index and counting back down to 0 or a separator.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> (From update of attachment 328499 [details] [diff] [review])
> >-    nsCOMPtr<nsIDOMXULElement> currItem;
> >-    container->GetItemAtIndex(index, getter_AddRefs(currItem));
> >-    nsCOMPtr<nsIDOMNode> currNode(do_QueryInterface(currItem));
> >+    nsCOMPtr<nsIDOMXULElement> item;
> >+    container->GetItemAtIndex(index, getter_AddRefs(item));
> >+    nsCOMPtr<nsIDOMNode> currNode(do_QueryInterface(item));
> I'm not sure why you bothered with the rename.

Just below I use "itemAcc", "itemRole". Those I think are more consistent with "item" rather than with "currItem"

another comments are addressed in the new patch.

Comment 6

[[e-mail]]

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

>+        break; // We reached the begin of our group.
Nit: beginning

Comment 7

[alexander :surkov (:asurkov)]

checked in http://hg.mozilla.org/mozilla-central/index.cgi/rev/38ce8d8d0167

Comment 8

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008071003 Minefield/3.1a1pre.