421565 - ARIA tweaks

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

While writing the ARIA user agent best practices document, I noticed some things we do wrong:

1. "list" and "listitem" should expose readonly, since they are not widgets like "listbox" and "option". This makes them like the HTML equivalents ul/li.
2. Under ATK we check to see if a listbox is parented by a combobox. If it is, we change the role to menu. We should check the NODE_CHILD_OF element as well (not just the parent).
3. We should expose an autocomplete object attribute to describe the type of autocomplete
4. Like ATK, for MSAA we should use an object attribute checkable="true" instead of relying on a hack for the state. Right now in MSAA we use MARQUEED.

Comment 1

[Aaron Leventhal]

We seem to be missing support for aria-multiline as well, but I thought that had been removed on purpose.

There is also something called aria-templateid. I've not heard of it before and am looking into it.

Comment 2

[Aaron Leventhal]

We need to make sure that ATK_STATE_EDITABLE is expose on role="textfield" but only when aria-readonly is not "true".

Comment 3

[Aaron Leventhal]

Attachment: Tweak ARIA

1) Add readonly to list/listitem which are doc structures
2) If LISTBOX is owned by (node child of) combobox, then change its role
3) Move checkable object attribute support to nsAccessible (out of ATK), but optimize so that GetFinalState() isn't called every time (it's relatively expensive compared to getting the role)
4) Expose any aria- property that isn't already expose some other way as an object attribute. This is a way of future-proofing in case new attributes are added, ATs can already get to them (just like they can with new roles)
5) For ROLE_ENTRY make sure we set STATE_EDITABLE correctly (check readonly state). Don't need to check for ROLE_PASSWORD here anymore -- it's not possible to make one with ARIA, and it couldn't be autocomplete or multiline anyway.
6) Do not change SELECTABLE/FOCUSABLE based on aria-disabled. It turns out that sometimes something can be disabled but focusable -- menuitems are an example, and sometimes authors want disabled form controls in the tab order

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)

> 4) Expose any aria- property that isn't already expose some other way as an
> object attribute. This is a way of future-proofing in case new attributes are
> added, ATs can already get to them (just like they can with new roles)

Why do you expose accessible relation related attributes like labelledby, flowsto and etc?

Could you use nsAccUtils::SetAccAttr instead SetStringProperty?

Comment 5

[alexander :surkov (:asurkov)]

I would like to get for this mochitest. Aaron if you haven't time for this then could you open bug for this?

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 308357 [details] [diff] [review]
Tweak ARIA


>     if (mRoleMapEntry && mRoleMapEntry->role == nsIAccessibleRole::ROLE_ENTRY) {
>       PRBool isMultiLine = content->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::aria_multiline,
>                                                 nsAccessibilityAtoms::_true, eCaseMatters);
>       *aExtraState |= isMultiLine ? nsIAccessibleStates::EXT_STATE_MULTI_LINE : nsIAccessibleStates::EXT_STATE_SINGLE_LINE;
>+      if (0 == (*aState & nsIAccessibleStates::STATE_READONLY))
>+        *aExtraState |= nsIAccessibleStates::EXT_STATE_EDITABLE; // Not readonly
>+      else  // We're readonly: make sure editable state wasn't set by impl class
>+        *aExtraState &= ~nsIAccessibleStates::EXT_STATE_EDITABLE;

Is it interfered with editable fields accessibles? Is the correct way to check editor?

>-  if (ariaState & nsIAccessibleStates::STATE_UNAVAILABLE) {
>-    // Disabled elements are not selectable or focusable, even if disabled
>-    // via DHTML accessibility disabled property
>-    ariaState &= ~(nsIAccessibleStates::STATE_SELECTABLE |
>-                   nsIAccessibleStates::STATE_FOCUSABLE);
>-  }
>-

why isn't it actual anymore?

Comment 7

[Aaron Leventhal]

(In reply to comment #4)
> (In reply to comment #3)
> 
> > 4) Expose any aria- property that isn't already expose some other way as an
> > object attribute. This is a way of future-proofing in case new attributes are
> > added, ATs can already get to them (just like they can with new roles)
> 
> Why do you expose accessible relation related attributes like labelledby,
> flowsto and etc?
We don't.
We call IsARIAPropForObjectAttr() which returns PR_TRUE if we need to expose it has an object attribute.

> Could you use nsAccUtils::SetAccAttr instead SetStringProperty?
It takes an atom, but I want to strip off aria- so that would be hard.

(In reply to comment #5)
> I would like to get for this mochitest. Aaron if you haven't time for this then
> could you open bug for this?
Yes I can open bug for it.

(In reply to comment #6)
> (From update of attachment 308357 [details] [diff] [review])
> 
> >+      if (0 == (*aState & nsIAccessibleStates::STATE_READONLY))
> >+        *aExtraState |= nsIAccessibleStates::EXT_STATE_EDITABLE; // Not readonly
> >+      else  // We're readonly: make sure editable state wasn't set by impl class
> >+        *aExtraState &= ~nsIAccessibleStates::EXT_STATE_EDITABLE;
> 
> Is it interfered with editable fields accessibles? Is the correct way to check
> editor?
The only way of doing an editor with an ARIA role is to use role="textbox". So it will have a role of ROLE_ENTRY. I want to make sure that READONLY affects EDITABLE, instead of using the value of EDITABLE set by the base impl (e.g. nsHTMLTextfieldAccessible) or by nsHypertextAccessible (because of contentEditable or designMode).

> >-  if (ariaState & nsIAccessibleStates::STATE_UNAVAILABLE) {
> >-    // Disabled elements are not selectable or focusable, even if disabled
> >-    // via DHTML accessibility disabled property
> >-    ariaState &= ~(nsIAccessibleStates::STATE_SELECTABLE |
> >-                   nsIAccessibleStates::STATE_FOCUSABLE);
> >-  }
> >-
> 
> why isn't it actual anymore?

We should not change SELECTABLE/FOCUSABLE based on aria-disabled. It turns out that sometimes something can be disabled but focusable -- menuitems are an example, and sometimes authors want disabled form controls in the tab order. So it's an incorrect assumption because sometimes you want to navigate to a disabled item.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (In reply to comment #4)
> > (In reply to comment #3)
> > 
> > > 4) Expose any aria- property that isn't already expose some other way as an
> > > object attribute. This is a way of future-proofing in case new attributes are
> > > added, ATs can already get to them (just like they can with new roles)
> > 
> > Why do you expose accessible relation related attributes like labelledby,
> > flowsto and etc?
> We don't.
> We call IsARIAPropForObjectAttr() which returns PR_TRUE if we need to expose it
> has an object attribute.

right, I misread this

> > Could you use nsAccUtils::SetAccAttr instead SetStringProperty?
> It takes an atom, but I want to strip off aria- so that would be hard.
> 

what about "checkable"?


> > >+      if (0 == (*aState & nsIAccessibleStates::STATE_READONLY))
> > >+        *aExtraState |= nsIAccessibleStates::EXT_STATE_EDITABLE; // Not readonly
> > >+      else  // We're readonly: make sure editable state wasn't set by impl class
> > >+        *aExtraState &= ~nsIAccessibleStates::EXT_STATE_EDITABLE;
> > 
> > Is it interfered with editable fields accessibles? Is the correct way to check
> > editor?
> The only way of doing an editor with an ARIA role is to use role="textbox". So
> it will have a role of ROLE_ENTRY. I want to make sure that READONLY affects
> EDITABLE, instead of using the value of EDITABLE set by the base impl (e.g.
> nsHTMLTextfieldAccessible) or by nsHypertextAccessible (because of
> contentEditable or designMode).

ok, what state will following examples have?
<span role="textbox" tabindex="0"/>
<span role="textbox" aria-readonly="false" tabindex="0"/>
<input aria-readonly="true"/>

Comment 9

[Aaron Leventhal]

> what about "checkable"?
I don't mind doing it for checkable, but since the method already has the extra string for the unused param, what do we save? We have to add checkable as an atom. Is it mostly for readability?

> ok, what state will following examples have?
> <span role="textbox" tabindex="0"/>
> <span role="textbox" aria-readonly="false" tabindex="0"/>
> <input aria-readonly="true"/>

All three should be readonly and not editable.

Comment 10

[Aaron Leventhal]

> <span role="textbox" aria-readonly="false" tabindex="0"/>
Sorry, actually for that one we should say it's editable.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> > what about "checkable"?
> I don't mind doing it for checkable, but since the method already has the extra
> string for the unused param, what do we save? We have to add checkable as an
> atom. Is it mostly for readability?
> 
> > ok, what state will following examples have?
> > <span role="textbox" tabindex="0"/>
> > <span role="textbox" aria-readonly="false" tabindex="0"/>
> > <input aria-readonly="true"/>
> 
> All three should be readonly and not editable.
> 

Does it correct that aria-readonly affects on input because actually it is editable? Why span is editable because it's not editable? Is it up to ARIA author?


(In reply to comment #10)
> > <span role="textbox" aria-readonly="false" tabindex="0"/>
> Sorry, actually for that one we should say it's editable.
> 

btw, why, we don't and shouldn't handle "false" value per spec?

Comment 12

[Aaron Leventhal]

We're in different timezones and I feel like we're taking too long to fix these last nits. Can you just give me a conditional r? In other words r+ if you do x and y.

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 308357 [details] [diff] [review]
Tweak ARIA

in general I'm ok with the patch, though I'm not sure aria must override everything. Though if we do this for roles then why we can't do it for states.

Comment 14

[Aaron Leventhal]

ARIA should override everything -- that's what it does. Otherwise, it's a mess of rules when it overrides and when it doesn't.

I have a change in another patch that fixes states again so they act like overrides.

Comment 15

[Damon Sicore (:damons)]

Comment on attachment 308357 [details] [diff] [review]
Tweak ARIA

a1.9+=damons

Comment 16

[Aaron Leventhal]

This was checked in a while ago.