477876 - expose checkable="true" object attribute

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

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

Whenever we have a defined value for aria-checked it is recommended we expose checkable="true" as an object attribute.

Comment 1

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

Attachment: patch1

This is probably the cleanest fix. Thoughts?

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 362750 [details] [diff] [review]
patch1

r=me. Thanks!

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 362750 [details] [diff] [review]
patch1


>+  // Expose checkable object attribute for checkable roles, and aria-checked
>+  PRUint32 state = 0;
>+  GetState(&state, nsnull);
>+  if ((state & nsIAccessibleStates::STATE_CHECKABLE) ||
>+      nsAccUtils::HasDefinedARIAToken(content, nsAccessibilityAtoms::aria_checked))

It sounds it is excess check, (state & nsIAccessibleStates::STATE_CHECKABLE) should be enough, it it's not then I guess it's a bug.

>+    attributes->SetStringProperty(NS_LITERAL_CSTRING("checkable"), NS_LITERAL_STRING("true"),
>+                                  oldValueUnused);

while you are here I would prefer to have checkeable atom and use nsAccUtils::SetAccAttr() method.

>+      testAttrs("checked", {"checkable" : "true"}, true); 

regarding tests I would like to have tests for checkable roles from HTML and ARIA markup.

Comment 4

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

Attachment: patch2

Thanks Surkov. How's this one?

Comment 5

[alexander :surkov (:asurkov)]

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


>     "radio",
>     nsIAccessibleRole::ROLE_RADIOBUTTON,
>     eNoValue,
>     eSelectAction,
>-    kNoReqStates,
>+    nsIAccessibleStates::STATE_CHECKABLE,

>+  if ((state & nsIAccessibleStates::STATE_CHECKABLE) ||
>+      nsAccUtils::HasDefinedARIAToken(content, nsAccessibilityAtoms::aria_pressed))

Will we expose STATE_CHECKABLE for checked="undefined". I guess we will.

Why don't we expose STATE_CHECKABLE for defined aria_pressed but we want to expose "checkabe" object attribute?

Comment 6

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

(In reply to comment #5)
> (From update of attachment 362901 [details] [diff] [review])
> 
> >     "radio",
> >     nsIAccessibleRole::ROLE_RADIOBUTTON,
> >     eNoValue,
> >     eSelectAction,
> >-    kNoReqStates,
> >+    nsIAccessibleStates::STATE_CHECKABLE,
> 
> >+  if ((state & nsIAccessibleStates::STATE_CHECKABLE) ||
> >+      nsAccUtils::HasDefinedARIAToken(content, nsAccessibilityAtoms::aria_pressed))
> 
> Will we expose STATE_CHECKABLE for checked="undefined". I guess we will.

Right.

> 
> Why don't we expose STATE_CHECKABLE for defined aria_pressed but we want to
> expose "checkabe" object attribute?

Hmmm yeah we should probably do that.

Also, I wonder if we should fire a state change event in nsDocAccessible::ARIAAttributeChanged for CHECKABLE where applicable? Overkill?

Comment 7

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

Attachment: patch3

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 362928 [details] [diff] [review]
patch3


>+    {&nsAccessibilityAtoms::aria_pressed, kBoolState, nsIAccessibleStates::STATE_PRESSED | nsIAccessibleStates::STATE_CHECKABLE},

why pressed button should be checkable?

>+  // Expose checkable object attribute for checkable roles, aria-checked, aria-pressed

nit: expose checkable object attribute if the accessible has checkable state?

>+  PRUint32 state = 0;
>+  GetState(&state, nsnull);
>+  if ((state & nsIAccessibleStates::STATE_CHECKABLE) ||
>+      nsAccUtils::HasDefinedARIAToken(content, nsAccessibilityAtoms::aria_pressed))

I would vote to not expose checkable state for undefined aria_pressed. It's better to fix GetState method.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> 
> Also, I wonder if we should fire a state change event in
> nsDocAccessible::ARIAAttributeChanged for CHECKABLE where applicable? Overkill?

I guess so because STATE_CHECKABLE is mapped to STATE_MARQUEED for MSAA only, neither ATK or IA2 don't have this state and we don't fire state change event for checkable state at all in the meantime.

Comment 10

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

(In reply to comment #8)
> (From update of attachment 362928 [details] [diff] [review])
> 
> >+    {&nsAccessibilityAtoms::aria_pressed, kBoolState, nsIAccessibleStates::STATE_PRESSED | nsIAccessibleStates::STATE_CHECKABLE},
> 
> why pressed button should be checkable?

It is as specified in the aria impl BPG. The these buttons have two states, pressed, and unpressed. Essentially they are toggles (like checkbox).

> 
> >+  // Expose checkable object attribute for checkable roles, aria-checked, aria-pressed
> 
> nit: expose checkable object attribute if the accessible has checkable state?

Not sure. I'll try to think of something.

> 
> >+  PRUint32 state = 0;
> >+  GetState(&state, nsnull);
> >+  if ((state & nsIAccessibleStates::STATE_CHECKABLE) ||
> >+      nsAccUtils::HasDefinedARIAToken(content, nsAccessibilityAtoms::aria_pressed))
> 
> I would vote to not expose checkable state for undefined aria_pressed. It's
> better to fix GetState method.

Unfortunately the BPG states explicitly: if aria-pressed is not undefined expose object attribute checkable="true"

Comment 11

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

(In reply to comment #9)
> (In reply to comment #6)
> 
> > 
> > Also, I wonder if we should fire a state change event in
> > nsDocAccessible::ARIAAttributeChanged for CHECKABLE where applicable? Overkill?
> 
> I guess so because STATE_CHECKABLE is mapped to STATE_MARQUEED for MSAA only,
> neither ATK or IA2 don't have this state and we don't fire state change event
> for checkable state at all in the meantime.

Okay let's worry about that when it is impactful.

Comment 12

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

Attachment: patch4

Surkov, I liked your suggestions. Let me know what you think of this one.

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 363110 [details] [diff] [review]
patch4

looks ok, though I liked your test
// bug 477876 testAttrs("checked", {"checkable" : "true"}, true); 

because it tested the case when there is no checkable role

r=me

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 363110 [details] [diff] [review]
patch4


>+  // Expose checkable object attribute if the accessible has checkable state
>+  PRUint32 state = 0;
>+  GetState(&state, nsnull);

nit: you could use nsAccUtils::State(this) - it would be later more easy to deny nsnull as second argument.

Comment 15

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

Attachment: Patch to push

Please push this one.

Comment 16

[Marco Zehe (:MarcoZ)]

Pushed on David's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/e35c958e35e7

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 363117 [details] [diff] [review]
Patch to push

a191=beltzner

Comment 18

[Marco Zehe (:MarcoZ)]

Pushed to mozilla-1.9.1 on David's behalf in changeset:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5a57b1f11482

Comment 19

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre (.NET CLR 3.5.30729)