expose group position for toolbarbuttons

RESOLVED FIXED

Status

()

Core
Disability Access APIs
--
minor
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: surkov, Assigned: Vasiliy Potapenko)

Tracking

(Blocks: 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

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

Updated

11 years ago
Severity: normal → minor
(Reporter)

Comment 1

11 years ago
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)
Assignee: aaronleventhal → vasiliy.potapenko
(Assignee)

Comment 2

11 years ago
Created attachment 270847 [details] [diff] [review]
patch
Attachment #270847 - Flags: review?(surkov.alexander)
(Reporter)

Comment 3

11 years ago
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
Attachment #270847 - Flags: review?(surkov.alexander)
Attachment #270847 - Flags: review?(aaronleventhal)
Attachment #270847 - Flags: review+

Comment 4

11 years ago
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.
Attachment #270847 - Flags: review?(aaronleventhal)
(Assignee)

Comment 5

11 years ago
Created attachment 270997 [details] [diff] [review]
patch
Attachment #270847 - Attachment is obsolete: true
Attachment #270997 - Flags: review?(aaronleventhal)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED

Comment 6

11 years ago
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.
Attachment #270997 - Flags: review?(aaronleventhal) → review+
(Reporter)

Comment 7

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

Comment 8

11 years ago
Comment on attachment 270997 [details] [diff] [review]
patch

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

Comment 9

11 years ago
Created attachment 271185 [details] [diff] [review]
patch
Attachment #270997 - Attachment is obsolete: true
Attachment #271185 - Flags: superreview?(neil)
Attachment #271185 - Flags: review?(aaronleventhal)

Comment 10

11 years ago
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.
Attachment #271185 - Flags: review?(aaronleventhal) → review+
(Assignee)

Comment 11

11 years ago
Created attachment 271190 [details] [diff] [review]
patch
Attachment #271185 - Attachment is obsolete: true
Attachment #271190 - Flags: superreview?(neil)
Attachment #271185 - Flags: superreview?(neil)

Comment 12

11 years ago
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]
(Assignee)

Comment 13

11 years ago
Created attachment 271612 [details] [diff] [review]
patch

I have included comment and corrected code.
Attachment #271190 - Attachment is obsolete: true
Attachment #271612 - Flags: superreview?(neil)
Attachment #271190 - Flags: superreview?(neil)

Updated

11 years ago
Attachment #271612 - Flags: superreview?(neil) → superreview+
(Reporter)

Comment 14

11 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Reporter)

Updated

8 years ago
Blocks: 537276
You need to log in before you can comment on or make changes to this bug.