551978 - Update universal ARIA attribute support to latest spec

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

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

We're a bit out of date. Patch coming.

Comment 1

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

Attachment: WIP

WIP. Includes small cleanup of unused param.

Comment 2

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

The aria-expanded issue needs more time and will happen on bug 551684

Comment 3

[alexander :surkov (:asurkov)]

David, it would help a lot for clean history if you quote the changed spec and provide a links :) Also it's nice to have patch description. At least the patch makes me think we want the one change only to not create accessibles if some aria attributes are presented.

Comment 4

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

Comment on attachment 432166 [details] [diff] [review]
WIP

yep. premature review request. i meant to add tests as well.

Comment 5

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

(In reply to comment #3)
> David, it would help a lot for clean history if you quote the changed spec and
> provide a links :) Also it's nice to have patch description. At least the patch
> makes me think we want the one change only to not create accessibles if some
> aria attributes are presented.

Sure. This is just making our method correct according to:
6.4 of http://www.w3.org/WAI/PF/aria/states_and_properties

I removed sort and required. We'll still create accessibles for these in 99.9% of cases, and the other cases are probably author error.

Comment 6

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

6.4 is currently:
    * aria-atomic
    * aria-busy (state)
    * aria-controls
    * aria-describedby
    * aria-disabled (state)
    * aria-dropeffect
    * aria-flowto
    * aria-grabbed (state)
    * aria-haspopup
    * aria-hidden (state)
    * aria-invalid (state)
    * aria-label
    * aria-labelledby
    * aria-live
    * aria-owns
    * aria-relevant

Comment 7

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

Attachment: patch 1

Comment 8

[alexander :surkov (:asurkov)]

aria-required should be removed from gWAIUnivStateMap and added to nsRoleMap
aria-expanded should be too but since it's in law now then it should be commented I think.

{  role: ROLE_TEXT_CONTAINER }, - please comment who is who

global_aria_states - global states and props?

btw, test_aira.html -> test_aria_globalstatesnprops.html or test_aria_global.html?

Comment 9

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

(In reply to comment #8)
> aria-required should be removed from gWAIUnivStateMap and added to nsRoleMap

Right.

> aria-expanded should be too but since it's in law now then it should be
> commented I think.
> 
> {  role: ROLE_TEXT_CONTAINER }, - please comment who is who

Sure.

> 
> global_aria_states - global states and props?

Right. Note the distinction between state and property is not important. Did you want me to change the name? (I couldn't tell)

> btw, test_aira.html -> test_aria_globalstatesnprops.html or
> test_aria_global.html?

I was thinking tree/test_aria could eventually contain other aria specific tests.

Comment 10

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

(In reply to comment #9)
> (In reply to comment #8)
 
> > global_aria_states - global states and props?
> 
> Right. Note the distinction between state and property is not important. Did
> you want me to change the name? (I couldn't tell)

I changed it :)

Comment 11

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

Attachment: patch 2

Addressed comments thanks Alexander.

I filed Bug 553117 for follow up on aria-required. If we remove it we would regress handy stuff like:
<div id="checkbox_required_true" role="checkbox" aria-required="true">

I don't think we should do that.

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 433215 [details] [diff] [review]
patch 2


> eStateMapEntryID nsARIAMap::gWAIUnivStateMap[] = {
>-  eARIARequired,
>-  eARIAInvalid,
>-  eARIAHasPopup,
>+  // Note the following universal ARIA attributes are intentionally not
>+  // included in this state map: atomic, controls, describedby, grabbed, label,
>+  // labelledby, live, owns, relevant. The states listed in this array are
>+  // interesting because they have platform state mappings.

In general (with haspopup exception) they aren't included because they aren't states and therefore they aren't mapped to accessible states. I think I don't find this comment useful. I think it should be enough the special comment for haspoup.

>   eARIABusy,
>   eARIADisabled,
>-  eARIAExpanded,
>+  eARIAExpanded,  // Currently under spec review but precedent exists
>+  eARIAHasPopup,  // Note this is technically a "property"

>-  {&nsAccessibilityAtoms::aria_checked,           ATTR_BYPASSOBJ | ATTR_VALTOKEN }, /* exposes checkable  obj attr */
>+  {&nsAccessibilityAtoms::aria_checked,           ATTR_BYPASSOBJ | ATTR_VALTOKEN }, /* exposes checkable obj attr */

nit: I didn't catch what was changed here.

Comment 13

[Marco Zehe (:MarcoZ)]

Comment on attachment 433215 [details] [diff] [review]
patch 2

r=me thanks!

Comment 14

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

(In reply to comment #12)
> (From update of attachment 433215 [details] [diff] [review])
> 
> > eStateMapEntryID nsARIAMap::gWAIUnivStateMap[] = {
> >-  eARIARequired,
> >-  eARIAInvalid,
> >-  eARIAHasPopup,
> >+  // Note the following universal ARIA attributes are intentionally not
> >+  // included in this state map: atomic, controls, describedby, grabbed, label,
> >+  // labelledby, live, owns, relevant. The states listed in this array are
> >+  // interesting because they have platform state mappings.
> 
> In general (with haspopup exception) they aren't included because they aren't
> states and therefore they aren't mapped to accessible states. I think I don't
> find this comment useful. I think it should be enough the special comment for
> haspoup.

Sure, if you don't find it useful I'll change it. (Note there might be more haspopup-like exceptions down the road)

> 
> >   eARIABusy,
> >   eARIADisabled,
> >-  eARIAExpanded,
> >+  eARIAExpanded,  // Currently under spec review but precedent exists
> >+  eARIAHasPopup,  // Note this is technically a "property"
> 
> >-  {&nsAccessibilityAtoms::aria_checked,           ATTR_BYPASSOBJ | ATTR_VALTOKEN }, /* exposes checkable  obj attr */
> >+  {&nsAccessibilityAtoms::aria_checked,           ATTR_BYPASSOBJ | ATTR_VALTOKEN }, /* exposes checkable obj attr */
> 
> nit: I didn't catch what was changed here.

touch up: removed one space from the comment

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)

> Sure, if you don't find it useful I'll change it. (Note there might be more
> haspopup-like exceptions down the road)

ok, it's up to you.

> > nit: I didn't catch what was changed here.
> 
> touch up: removed one space from the comment

ok

Comment 16

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

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