463645 - container-live object attribute should reflect role as well

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

We expose the computed politeness (aria-live) value for any given node via container-live.

This is calculated here:
http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccUtils.cpp#278

It should really check the current role as it traverses the tree. The following roles have a default aria-live property value:
timer: "off"
log: "polite"
status: "polite"
marquee: "off"

Comment 1

[Aaron Leventhal]

The current code is: 
278     if (live.IsEmpty() &&
279         ancestor->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_live, live))
280       SetAccAttr(aAttributes, nsAccessibilityAtoms::containerLive, live);

I believe it should be something like:
 if (live.IsEmpty()) {
   if (ancestor->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_live, live))
     SetAccAttr(aAttributes, nsAccessibilityAtoms::containerLive, live);
   else if (ancestor->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::role)) {
     nsRoleMapEntry *role = GetRoleMapEntry(ancestor);
     if (role && role->defaultLiveProp)
       live = role->defaultLiveProp;
       SetAccAttr(aAttributes, nsAccessibilityAtoms::containerLive, live);
   }
  }
 }
 
Then, each role map entry needs an extra defaultLiveProp field. It would be nsnull in most cases.

We'd need to add a new entry for "log", "timer" and "marquee". I guess they'd need a role of ROLE_NOTHING.

We'd need a better test to see if something is a landmark though. Right now we just look to see if it equals gLandmarkRoleMap.
http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessible.cpp#1924
But we need a better test for that anyway because of bug 459395.

Comment 2

[alexander :surkov (:asurkov)]

Should we expose "live" object attribute on accessible itself?

Comment 3

[Aaron Leventhal]

Surkov, I'd say yes.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 5

[Marco Zehe (:MarcoZ)]

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

r=me.

Comment 6

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

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

Great, just a few questions first:

>+    "log",
>+    nsIAccessibleRole::ROLE_NOTHING,

(FYI, Marco talked about exposing a log as a table (see bug 469688), but maybe that only makes sense if there is table html markup? In any case, that work probably doesn't belong on this bug)


>+enum ELiveAttrRule
>+{
>+  eNoLiveAttr,
>+  eOffLiveAttr,
>+  ePoliteLiveAttr
>+};
>+

(I notice the specs still talk about aria-live=assertive)


>   // Value mapping rule: how to compute nsIAccessible value
>   EValueRule valueRule;
> 
>   // Action mapping rule, how to expose nsIAccessible action
>   EActionRule actionRule;
>+
>+  // Default value of aria-live attribute
>+  ELiveAttrRule liveAttrDefValue;

Is this a rule or a value? Your other ERules have variables with "Rule" suffixes. I'm not saying to change it, but maybe you could explain this?

>+    if (mRoleMapEntry->role != nsIAccessibleRole::ROLE_NOTHING) {
>+      // We can now expose ROLE_NOTHING when there is a role map entry or used
>+      // role is nothing, which

What does "used role is nothing" mean here?


>+  // If there is no aria-live attribute then expose default value of 'live'
>+  // object attribute used for ARIA role of this accessible.

Nit: Why is 'live' in quotes here?

Comment 7

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

(In reply to comment #6)
> (From update of attachment 362549 [details] [diff] [review])

> >+  // If there is no aria-live attribute then expose default value of 'live'
> >+  // object attribute used for ARIA role of this accessible.
> 
> Nit: Why is 'live' in quotes here?

Nevermind this question -- it suddenly made sense to me :)

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> (FYI, Marco talked about exposing a log as a table (see bug 469688), but maybe
> that only makes sense if there is table html markup? In any case, that work
> probably doesn't belong on this bug)

We will expose it as table if there is html table markup for example. We shouldn't expose it as table originally I think.
 

> (I notice the specs still talk about aria-live=assertive)

Yes, but in the meantime we don't need it (I used cases defined in bug description).


> >+  // Default value of aria-live attribute
> >+  ELiveAttrRule liveAttrDefValue;
> 
> Is this a rule or a value? Your other ERules have variables with "Rule"
> suffixes. I'm not saying to change it, but maybe you could explain this?

Ok, I changed it to rule and changed some comments to make thing a bit clear. Though the question is it rule or value is question of terms I guess. Technically I like to use "rule" here because we do this above.

> 
> >+    if (mRoleMapEntry->role != nsIAccessibleRole::ROLE_NOTHING) {
> >+      // We can now expose ROLE_NOTHING when there is a role map entry or used
> >+      // role is nothing, which
> 
> What does "used role is nothing" mean here?

either role nothing or landmark which has role nothing as well. If we have role nothing here then we should allow to get role from native markup.

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 10

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

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

Thanks. r=me

Comment 11

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/mozilla-central/rev/b13b71bf3ab9

Comment 12

[alexander :surkov (:asurkov)]

I backuped one line (http://hg.mozilla.org/mozilla-central/rev/07c015cadf15)

- if (mRoleMapEntry != &nsARIAMap::gLandmarkRoleMap) {
+  if (mRoleMapEntry->role != nsIAccessibleRole::ROLE_NOTHING) {

button name tests fails because role presentation used on button has nothing role but it's not landmark, we shouldn't allow to calculate name from subtree of presentation button but we do with this line.

Comment 13

[alexander :surkov (:asurkov)]

timer: inherited from status, should have ROLE_STATUSBAR (missed in ARIA impl guide)
log: inherited from region, should have ROLE_PANE (missed in ARIA impl guide)
status: ROLE_STATUSBAR (corresponds to ARIA impl guide)
marquee: inherited from section, should have ROLE_PANE (corresponds to ARIA impl guide)

Before this bug timer, log, status and marquee had role nothing and timer, log and marquee allowed to get role from native markup. Iirc the fact accessible with log role gets role from native markup is used in chatzilla so I could regress it.

Can we clarify what ARIA roles
1)  should have own role
2)  should have default role (that can be overridden by role from native markup)
3)  should have no role (role nothing)
4)  should allow to get name from subtree.

Comment 14

[alexander :surkov (:asurkov)]

Any feedback on comment #13.

Comment 15

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

Regarding timer, log, status, and marquee, I think they should all have their own role (your #1 above). I'm not sure about #4 but I would probably lean toward not looking at the subtree for naming any of these. I imagine we'll discuss this in the UAI-TF.

Alexander, what do you think?

Comment 16

[alexander :surkov (:asurkov)]

Sounds right. Ok let's follow to #1. I filed bug 481114.

Comment 17

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

Remaining work fixed via bug 481114