368012 - expose STATE_CHECKABLE for each checkable widget

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

[Nian Liu(n/a in a long time)]

1.candidate widget are:
checkbox, checkable list item, checkable menu item, checkable tree item

any others?

Comment 1

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

Note possible related bug 365866 regarding xul cyclers (multistate widgets). The patch on that bug also checks for nsITreeColumn::TYPE_CHECKBOX and reports checkable state. Feel free to steal code if helpful.

Comment 2

[Nian Liu(n/a in a long time)]

(In reply to comment #1)
> Note possible related bug 365866 regarding xul cyclers (multistate widgets).
> The patch on that bug also checks for nsITreeColumn::TYPE_CHECKBOX and reports
> checkable state. Feel free to steal code if helpful.
> 

David, 365866 is focused on cycler(in other words, tree cells). this is focused on all checkable widget. if you can fix it in 365866, it will be great

Comment 3

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

Added 312457 dependency for bookkeeping purposes.

Comment 4

[alexander :surkov (:asurkov)]

David, probably you like to do it in one patch (I mean patch of bug 312457)? ;)

Comment 5

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

Attachment: proposed patch

Comment 6

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

Attachment: proposed patch

further tweaks to the solution.

Comment 8

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

Surkov, this patch should cover the remaining checkable widgets. Menu and list items have already been fixed (I assume after this bug was reported).

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 263413 [details] [diff] [review]
proposed patch

r=me

Btw, what's about radio buttons and  button[type="checkbox"]?

Comment 10

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

Attachment: expanded coverage

Thanks Surkov. I have added radio buttons, as well as XUL buttons of type="checkbox"|"radio".

Comment 11

[Aaron Leventhal]

Comment on attachment 263534 [details] [diff] [review]
expanded coverage

For cyclers let's expose an object attribute called "cycles" = "true". Otherwise looks good.

+  else {
+    PRBool isCycler;
+    mColumn->GetCycler(&isCycler);
+    if (isCycler) {
+      *aState |= nsIAccessibleStates::STATE_CHECKABLE;
+      // note cyclers can have many "checked" values (not just true/false)
+    }
+  }
+

Comment 12

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

Attachment: exposes cycles=true object attribute

Surkov, Aaron wanted a cycles attribute. Does this look correct?

Comment 13

[Aaron Leventhal]

Comment on attachment 263683 [details] [diff] [review]
exposes cycles=true object attribute

It's correct, with a few nits:

You can just pass NS_LITERAL_STRING("true") directly as an argument -- no temp variable _true necessary.

Consider whether you need to null check mColumn. I didn't look at the rest of that method to see if it's already been done.

+  PRBool isCycler;
+  mColumn->GetCycler(&isCycler);
+  if (isCycler) {
+    nsAutoString _true;
+    _true.AssignLiteral("true");
+    nsAccessibilityUtils::SetAccAttr(aAttributes, 
+          nsAccessibilityAtoms::cycles,
+          _true);
+  }
+

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 263683 [details] [diff] [review]
exposes cycles=true object attribute


nit: please add comment we set here "cycle" attribute and above we set group attributes.

>+  PRBool isCycler;
>+  mColumn->GetCycler(&isCycler);
>+  if (isCycler) {
>+    nsAutoString _true;
>+    _true.AssignLiteral("true");
>+    nsAccessibilityUtils::SetAccAttr(aAttributes, 
>+          nsAccessibilityAtoms::cycles,
>+          _true);

I would prefer to use NS_LITERAL_STRING("true") here

Btw, Aaron likes to expose "cycler" attribute instead checkable state or he wants to see them both?

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)

> Btw, Aaron likes to expose "cycler" attribute instead checkable state or he
> wants to see them both?
> 

If Aaron gave r+ then it's not a question any more.

Comment 16

[Aaron Leventhal]

Yes, I like the object attribute _instead of_ CHECKABLE when it's a cycler.

Comment 17

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

Attachment: patch to go in trunk

patch to go in. 

Thanks for the NS_LITERAL_STRING tip, I was looking for something like that.

Comment 18

[Aaron Leventhal]

Checked in.

Comment 19

[Aaron Leventhal]

I almost forgot to change the docs for AT developers on what object attributes we support:
http://developer.mozilla.org/en/docs/Accessibility/ATSPI_Support#Supported_AT-SPI_Object_Attributes

Also, I'm realizing when the checkable state is exposed, we should exposed an object attribute "checkable" = "true".

We need this because there is no state in ATK. Also, in MSAA we invented a state where there is none, and it would be better to have a more official way in IA2.