378042 - expose group position for toolbarbuttons

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Toolbarbuttons can be grouped logically by toolbarseparator, toolbarspring, toolbarspacer. We need to expose groupPosition()/ARIA group attributes for toolbarbuttons.

Comment 1

[alexander :surkov (:asurkov)]

Vasiliy, can you look at this one? It's also good bug for start :).

You should implement GetAttributesInternal() for toolbarbutton accessible. Calculate setsize and posinset attributes by looking at toolbarseparator, toolbarspring or toolbarspacer elements from the left and the right of toolbarbutton.

You may want to add new accessible class for toolbarbutton that is inherited from nsXULButtonAccessible. If you'll do this then please remember to implement nsIAccessibleProvider for toolbarbutton binding (see http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/toolbarbutton.xml#8). And please change nsAccessibilityService::GetAccessibleByType (see http://lxr.mozilla.org/mozilla/source/accessible/src/base/nsAccessibilityService.cpp#1344)

Comment 2

[Vasiliy Potapenko]

Attachment: patch

Comment 3

[alexander :surkov (:asurkov)]

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

>+  
>+  nsCOMPtr<nsIAccessible> sibling;
>+  
>+  PRInt32 SetSize = 1;
>+  
>+  GetPreviousSibling(getter_AddRefs(sibling));

I guess nsAccessible::GetNextChild() would be easier to use here.

>+  while(sibling) {
>+    nsCOMPtr<nsIDOMNode> siblingNode;
>+    nsCOMPtr<nsIAccessNode> accessNode(do_QueryInterface(sibling));

even every accessible implements nsIAccessNode but I would prefer to use NS_ENSURE_STATE

>+    accessNode->GetDOMNode(getter_AddRefs(siblingNode));
>+    nsCOMPtr<nsIContent> contentSibling(do_QueryInterface(siblingNode));

if accessible is out of date then you'll get a crash. Please ensure contentSibling is not null.

>+    if(contentSibling->NodeInfo()->Equals(NS_LITERAL_STRING("toolbarseparator")) ||

I would prefer to use atoms here.

>+  PRInt32 PosInSet = SetSize;

usually local variables first letter is in lower case.


>   <binding id="toolbarbutton" display="xul:button"
>            extends="chrome://global/content/bindings/button.xml#button-base">
>+    <implementation implements="nsIAccessibleProvider">

usually xbl:implementation is going after xbl:content

>+      <property name="accessibleType" readonly="true">
>+        <getter>
>+          <![CDATA[
>+              return Components.interfaces.nsIAccessibleProvider.XULToolbarButton;
>+            ]]>

you don't need cdata section

Comment 4

[Aaron Leventhal]

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

> even every accessible implements nsIAccessNode but I would prefer to use
NS_ENSURE_STATE

I don't think we have to do that, we don't in other places. Maybe an assertion is okay, but I don't think we should increase code size for it.

I'll wait for patch which addresses Surkov's comments before review.

Comment 5

[Vasiliy Potapenko]

Attachment: patch

Comment 6

[Aaron Leventhal]

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

1. Make IsSeparator() a static method because it doesn't need |this|. Then change aSibling to aAccessible (because a static method could be called from anywhere, and is usable for more than siblings of a node).
You should be able to change contentSibling->NodeInfo()->Equals() to just contentSibling->Tag() == atom. The right way is probably to keep NodeInfo()->Equals() and pss the XUL namespace in (for the first argument?)
Also, instead of if/else for the return, you can just return the value of the conditional, e.g.
return a || b || c;

2. Using sibling->GetPreviousSibling(getter_AddRefs(sibling)) is not ideal, because we don't cache backward pointers.
In other words, this is O(n^2)
However, there aren't usually all that many toolbar buttons in a toolbar, so I guess it's okay.
Although, I would have preferred a single loop that starts at the parent's first child, and sets posInSet to setSize when sibling == this

Part #2 is optional. If you do that, please submit a new patch for review.

Comment 7

[alexander :surkov (:asurkov)]

Is it enough to get sr from Neil to check in the patch since it contains some minor toolkit changes? :)

Comment 8

[[e-mail]]

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

>+        <getter>
>+              return Components.interfaces.nsIAccessibleProvider.XULToolbarButton;
Indentation is wrong ;-)

Comment 9

[Vasiliy Potapenko]

Attachment: patch

Comment 10

[Aaron Leventhal]

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

Unforunately this causes problems:
+      sibling->GetNextSibling(getter_AddRefs(sibling));
You need an |nsCOMPtr<nsIAccessible> tempSibling;| otherwise this can break. It's unfortunately but as far as I know it's still true. The most efficient thing to do is apparently:
sibling->GetNextSibling(getter_AddRefs(tempSibling));
sibling.swap(tempSibling);

Nit: you don't need to declare   nsCOMPtr<nsIAccessible> sibling until inside the |if (parent)| clause.

Nit: you don't need all those parenthesis in your return statement in IsSeparator()

Otherwise looks good.

Don't ask me for another review because I'm gone on vacation for a week.

Comment 11

[Vasiliy Potapenko]

Attachment: patch

Comment 12

[[e-mail]]

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

>+      setSize++;
>+      if (sibling == this)
>+        posInSet = setSize;
>+      if (IsSeparator(sibling)) {
>+        if (posInSet) {
>+          setSize--;
>+          break;
>+        }
>+        else
>+          setSize = 0;
>+      }
I thought this was very confusing. Does this mean the same thing?
if (IsSeparator(sibling)) { // end of a group of buttons
  if (posInSet)
    break; // we've found our group, so we're done
  setSize = 0; // not our group, so start a new group
} else {
  setSize++; // another button in the group
  if (sibling == this)
    posInSet = setSize; // we've found our button
}
[Note: comments included for my benefit, to make sure I understood the code]

Comment 13

[Vasiliy Potapenko]

Attachment: patch

I have included comment and corrected code.

Comment 14

[alexander :surkov (:asurkov)]

checked in