499653 - unify ARIA state attributes mapping rules

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Now every role map entry defines own state mapping rules and it's easy to write inconsistent code, for example, role="checkbox" has checkable state when aria-checked is undefined however role="listitem" is free from this. On another had nsAccessible.cpp has few state map implementations though they should be enclosed in nsARIAMap logic. Also this bug helps to implement inheritance of aria-selected from row to gridcell by the nice way.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

let's start

Comment 2

[Marco Zehe (:MarcoZ)]

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

A few nits/questions:

>   {
>     "document",
>     nsIAccessibleRole::ROLE_DOCUMENT,
>     kUseMapRole,
>     eNoValue,
>     eNoAction,
>     eNoLiveAttr,
>-    nsIAccessibleStates::STATE_READONLY,
>-    kEndEntry
>+    nsIAccessibleStates::STATE_READONLY
>   },

>   {
>     "progressbar",
>     nsIAccessibleRole::ROLE_PROGRESSBAR,
>     kUseMapRole,
>     eHasValueMinMax,
>     eNoAction,
>     eNoLiveAttr,
>-    nsIAccessibleStates::STATE_READONLY,
>-    kEndEntry
>+    nsIAccessibleStates::STATE_READONLY
>   },

Why do these two have nsIAccessibleStates::STATE_READONLY hardcoded, but others do have eARIAReadOnly?

>+ * ARIA role don't override the role from native markup.

"doesn't override ..."

>+  // Points if attribute is token (can be undefined)

"indicates if..."

Also, do we want to test a native textarea element here as well, or do we have it covered in other places?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)

> Why do these two have nsIAccessibleStates::STATE_READONLY hardcoded, but others
> do have eARIAReadOnly?

because we should expose readonly always on them :) eARIAReadonly is used on controls when they can be readonly if @readonly attribute is used.

> Also, do we want to test a native textarea element here as well, or do we have
> it covered in other places?

what kind of tests do you like to have for textarea?

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to comment #3)
> > Also, do we want to test a native textarea element here as well, or do we have
> > it covered in other places?
> what kind of tests do you like to have for textarea?

Forget it, I just realized that we have those covered in the test_textboxes.html file. Thanks!

Comment 5

[Marco Zehe (:MarcoZ)]

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

r=me for the test part, and from my knowledge, the code part looks OK, too, but will leave that to David's expert eye. Thanks!

Comment 6

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

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

Thanks. As per IRC, I like this change. I just want to make sure I understand the naming (see end), and...

>-static const nsStateMapEntry kEndEntry = {nsnull, 0, 0};  // To fill in array of state mappings
>+ */

Why did we have this?

>+PRBool
>+nsStateMapEntry::MapToStates(nsIContent *aContent,
>+                             PRUint32 *aState, PRUint32 *aExtraState,
>+                             eStateMapEntryID aStateMapEntryID)

Could pass a const eStateMapEntryID* here instead.

>+/**
>+ * ID for state map entry, used in nsRoleMapEntry.
>+ */
>+enum eStateMapEntryID
>+{

>+  eARIASelected

Why is this not eARIASelectable?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> (From update of attachment 384585 [details] [diff] [review])
> Thanks. As per IRC, I like this change. I just want to make sure I understand
> the naming (see end), and...
> 
> >-static const nsStateMapEntry kEndEntry = {nsnull, 0, 0};  // To fill in array of state mappings
> >+ */
> 
> Why did we have this?

I'm not sure how struct are initialized so it should be a protect to not go further - if we met nsnull as attribute then we don't try another nsStateMapEntry?

> 
> >+PRBool
> >+nsStateMapEntry::MapToStates(nsIContent *aContent,
> >+                             PRUint32 *aState, PRUint32 *aExtraState,
> >+                             eStateMapEntryID aStateMapEntryID)
> 
> Could pass a const eStateMapEntryID* here instead.

Could. But why?

> >+  eARIASelected
> 
> Why is this not eARIASelectable?

I called checkable to show it's checkable by default, i.e. when aria-checked is missed. Currently this is not applicable for aria-selected. I would assume it should be for some roles like for role="listitem". Deal with another bug?

Comment 8

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

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

r=me (with Marco's nits fixed) :)

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 384585 [details] [diff] [review] [details])
> > Thanks. As per IRC, I like this change. I just want to make sure I understand
> > the naming (see end), and...
> > 
> > >-static const nsStateMapEntry kEndEntry = {nsnull, 0, 0};  // To fill in array of state mappings
> > >+ */
> > 
> > Why did we have this?
> 
> I'm not sure how struct are initialized so it should be a protect to not go
> further - if we met nsnull as attribute then we don't try another
> nsStateMapEntry?

Yeah I guess that used to be checked somewhere.

> 
> > 
> > >+PRBool
> > >+nsStateMapEntry::MapToStates(nsIContent *aContent,
> > >+                             PRUint32 *aState, PRUint32 *aExtraState,
> > >+                             eStateMapEntryID aStateMapEntryID)
> > 
> > Could pass a const eStateMapEntryID* here instead.
> 
> Could. But why?

Yeah, no gain. I prefer you leave it as is actually :)

> 
> > >+  eARIASelected
> > 
> > Why is this not eARIASelectable?
> 
> I called checkable to show it's checkable by default, i.e. when aria-checked is
> missed. Currently this is not applicable for aria-selected. I would assume it
> should be for some roles like for role="listitem". Deal with another bug?

OK, we can rename in the future if we need to.

Comment 9

[alexander :surkov (:asurkov)]

checked in with comment on mozilla 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/555cfd2b4e31