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.

let's start

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?

(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?

(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 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 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?

(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 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.

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