Closed Bug 452388 Opened 11 years ago Closed 11 years ago

Support value of "undefined" for aria-checked/aria-pressed/aria-selected

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b4

People

(Reporter: aaronlev, Assigned: davidb)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: access, verified1.9.1)

Attachments

(2 files, 11 obsolete files)

23.87 KB, patch
surkov
: review+
aaronlev
: review+
Details | Diff | Splinter Review
27.67 KB, patch
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
In general this means the same thing as an empty ("") attribute value or if the attribute is not present, for these 3 properties.

aria-checked="undefined" means the item is not checkable
aria-pressed="undefined" means the item is not pressable (iow if it's a button it's not a ROLE_TOGGLE_BUTTON)(
aria-selected="undefined" means the item is not selected
(In reply to comment #0)

> aria-selected="undefined" means the item is not selected
> 

is not selectable I guess?
Alex, good catch. Yes, that's what I meant.
Attached patch untested patch. (obsolete) — Splinter Review
I took a stab at this during lunch. Let me know what you think of this approach. I added a util method to check for defined attribute values, as we'll probably end up using this all over the place. Completely untested. Compiles OK... going back to my day job :)
You should change nsAccessible::MappedAttrState to handle the mapping of "aria-pressed" and etc attributes to states because for example "undefined" of "aria-pressed" is mapped to STATE_PRESSED state. Another issue is handling the change of "aria-selected" attribute in nsDocAccessible::AttributeChangedImpl where "undefined" value is not handled.
Surkov, good points, and it makes sense to do that work on this bug. I'm not sure when I'll get to it though.

BTW are you saying aria-pressed='undefined' should be mapped to STATE_PRESSED, or that it currently is, and we need to fix it? I'm assuming the latter.
(In reply to comment #5)

> BTW are you saying aria-pressed='undefined' should be mapped to STATE_PRESSED,
> or that it currently is, and we need to fix it? I'm assuming the latter.

The latter.

Any "undefined" value for something of type NMTOKEN is the same as it not being present.
(In reply to comment #5)
> Surkov, good points, and it makes sense to do that work on this bug. I'm not
> sure when I'll get to it though.

I can take this one and ask you to be reviewer if you want.

> BTW are you saying aria-pressed='undefined' should be mapped to STATE_PRESSED,
> or that it currently is, and we need to fix it? I'm assuming the latter.

Right. We need to fix this.
(In reply to comment #7)
> (In reply to comment #5)
> > Surkov, good points, and it makes sense to do that work on this bug. I'm not
> > sure when I'll get to it though.
> 
> I can take this one and ask you to be reviewer if you want.
> 
> > BTW are you saying aria-pressed='undefined' should be mapped to STATE_PRESSED,
> > or that it currently is, and we need to fix it? I'm assuming the latter.
> 
> Right. We need to fix this.

Sorry I missed my bug mail. Sure please go ahead if this work is a priority.
Ok, concerting to attribute changes. Do I get right the following stuffs?

If aria-pressed is changed from defined to undefined value (and vice versa) then we should invalidate the accessible because role is changed.

if aria-selected/aria-checked are changed from defined to undefined value then we should fire STATE_SELECTABLE/STATE_CHECKABLE state change events.

If aria-selected/aria-checked are changed from undefined to defined value then we should fire STATE_SELECTABLE/STATE_CHECKABLE state change events plus STATE_SELECTED/STATE_CHECKED state change events.
Another issue we can't distinguish if the values of these attributes are changed from defined to undefined (and visa versa) because nsIMutationObserver::AttributeChanged doesn't provide old attribute value. I filed  bug 467144 for this. Also I filed the bug 467143 for related issue (mixed state change events are fired for focused accessible only).
(In reply to comment #9)
> If aria-pressed is changed from defined to undefined value (and vice versa)
> then we should invalidate the accessible because role is changed.
We should, in order to allow AT to update its cache.
In fact, something similar could happen with a button haspopup if it's now true or no longer true.
This could be done in a separate bug.

> if aria-selected/aria-checked are changed from defined to undefined value then
> we should fire STATE_SELECTABLE/STATE_CHECKABLE state change events.
> If aria-selected/aria-checked are changed from undefined to defined value then
> we should fire STATE_SELECTABLE/STATE_CHECKABLE state change events plus
> STATE_SELECTED/STATE_CHECKED state change events.

Technically true, but I don't think any AT will notice or use that, so it's a low priority spin-off bug IMO.
(In reply to comment #10)
> Another issue we can't distinguish if the values of these attributes are
> changed from defined to undefined (and visa versa) because
> nsIMutationObserver::AttributeChanged doesn't provide old attribute value. I
> filed  bug 467144 for this. Also I filed the bug 467143 for related issue
> (mixed state change events are fired for focused accessible only).

Right. I don't think these are major issues that will be noticed very often, but it would be good to eventually fix them if it doesn't create a perf problem in the main code.
Here is a testcase for aria-selected:
http://codetalks.org/source/simple/selected.html
Marco, Alex -- I'd love to get this into Firefox 3.1. Any chance of that still?
There is still a chance, however it would be good if you could summarize in one single comment why this is important for who. If it's not already in the description or a comment in a concise manner which is easy to triage. And of course we'd need to see if David's patch is good and get it reviewed.
Why is this important? Without this fix, authors who follow the specification and use "undefined" or "" for an ARIA property will get incorrect results in Firefox.

It is not a complex fix.
Flags: wanted1.9.1?
(In reply to comment #11)
> (In reply to comment #9)
> > If aria-pressed is changed from defined to undefined value (and vice versa)
> > then we should invalidate the accessible because role is changed.
> We should, in order to allow AT to update its cache.
> In fact, something similar could happen with a button haspopup if it's now true
> or no longer true.
> This could be done in a separate bug.

Did a separate bug get created for this?  I'd like to now the scope of this bug.
Filed spinoffs: bug 472142 and bug 472143 for the related work Surkov has described.
Status: NEW → ASSIGNED
Marking the spin-off bugs as dependencies.
Depends on: 472142, 472143
Assignee: nobody → david.bolter
Attached patch WIP - works in manual testing. (obsolete) — Splinter Review
This patch introduces a requirement that aria attributes have acceptable defined values before we process an ARIA mapping from aria attribute to accessible state.

It might be heavy handed in that it doesn't like value-less attributes. Do we want to allow something like <span aria-checked>, where an attribute has no value?
 
Sorry about spacing nits in the diff, I've since tweaked my IDE not to remove trailing whitespace.
Again sorry about the littering of trailing-space-removal in this patch.
Attachment #348222 - Attachment is obsolete: true
Attachment #355815 - Attachment is obsolete: true
Attachment #355852 - Flags: review?(marco.zehe)
Attachment #355852 - Flags: review?(aaronleventhal)
Attachment #355852 - Flags: review?(aaronleventhal)
Comment on attachment 355852 [details] [diff] [review]
Less heavy handed. A11y mochitests now pass.

Have surkov review first then I will.
Attachment #355852 - Flags: review?(marco.zehe)
Attached patch Possible fix -- see comment. (obsolete) — Splinter Review
I got rid of the white space 'fixes'. I've tested this manually with the page Aaron provides in comment 13. A11y mochitests all pass (no unexpected fails).
Attachment #355852 - Attachment is obsolete: true
Attachment #355860 - Flags: review?(surkov.alexander)
Attachment #355860 - Flags: review?(surkov.alexander)
Comment on attachment 355860 [details] [diff] [review]
Possible fix -- see comment.

Removed review request. I want to grep more of the code for other possible points of contact.
Attachment #355860 - Attachment is obsolete: true
Attachment #355906 - Flags: review?(surkov.alexander)
David, mochitests are needed.
Do I understand correct ARIA reserve the word "undefined" and therefore the document can't have "undefined" ID?
Only as the value for aria-* attributes if I got this right.
(In reply to comment #28)
> Only as the value for aria-* attributes if I got this right.

Marco, I think it may confuse. I think it's not good if ARIA spec says you can't have id attribute with "undefined" value if you want to use it with ARIA.
Be careful: "undefined" is only for boolean or NMTOKEN attributes, otherwise it would be conflicting with potential usage of "undefined" as a string or ID.
Comment on attachment 355906 [details] [diff] [review]
patch, passes tests. (found a few more tweaks)

canceling review.
Attachment #355906 - Flags: review?(surkov.alexander)
To clarify: "undefined" is for ARIA attributes where the possible values are a token from a finite list, or true/false.

"undefined" is for things like checked/pressed/expanded/live/etc.
In short, "undefined" is an additional possible value for an ARIA property when there is a finite set of possible values for that ARIA property.
To clarify: What Surkov and Aaronlev were saying is: Make sure "undefined" only becomes an additional attribute for those ARIA attributes that accept "true" and "false", or another *given* set of values, but not those ARIA attributes like aria-describedby or aria-activedescendant which can accept *anything* as a value, although that *anything* must be a valid ID for example. So you should enhance your function to return PR_TRUE if a certain set of attributes is not passed in.
(In reply to comment #33)
> So you should
> enhance your function to return PR_TRUE if a certain set of attributes is not
> passed in.

Not sure it's nice from performance point of view.
Attachment #355906 - Attachment is obsolete: true
Attachment #355996 - Attachment is obsolete: true
Attachment #356000 - Flags: review?(marco.zehe)
Attachment #355996 - Flags: review?
Attached patch Includes suite test fix. (obsolete) — Splinter Review
Reposting. Forgot mime type on last patch upload
Attachment #356000 - Attachment is obsolete: true
Attachment #356001 - Flags: review?(marco.zehe)
Attachment #356000 - Flags: review?(marco.zehe)
Comment on attachment 356001 [details] [diff] [review]
Includes suite test fix.

This looks correct, r+ for the test part. Please have Aaron or Surkov review it once again, too.
Attachment #356001 - Flags: review?(marco.zehe) → review+
Attachment #356001 - Flags: review?(surkov.alexander)
Attached patch this patch includes a mochitest (obsolete) — Splinter Review
Added mochitest test_aria_undefined_token.html
Attachment #356001 - Attachment is obsolete: true
Attachment #356024 - Flags: review?(marco.zehe)
Attachment #356001 - Flags: review?(surkov.alexander)
Attachment #356024 - Flags: review?(surkov.alexander)
Attachment #356024 - Flags: review?(marco.zehe) → review+
Comment on attachment 356024 [details] [diff] [review]
this patch includes a mochitest

Two nits:
>+  <title>An NMTOKEN based ARIA property is undefined if the ARIA attribute is not present, or is set to "" or "undefined"</title>

Should this not read "MTOKEN" (omit the N at the start)? Or is this the correct spelling?

>+      testStates("trueSelected", STATE_SELECTABLE);
>+      testStates("trueSelected", STATE_SELECTED);

In this and in all other similar cases, simply write one function call:

testStates("trueSelected", (STATE_SELECTABLE |STATE_SELECTED);

You can just fix these and attach a new patch without asking re-review. That is, if Surkov doesn't find anything he wants to see fixed and review again. From the test side, it's good!
David, it sounds your patch doesn't handle all things we need to support (see comment #4, comment #10). If you're not going to implement this here then please add XXX sections in code. It will be easy to review for me.

Possibly IsDefinedARIATokenAttr is too long, would short analogue IsDefinedAttr be better ?
nit: file name test_aria_undefined_token.html -> test_aria_token_attrs or something? because you check not undefined values there only.
ACCESSIBILITY_ATOM(aria_hidden, "aria-hidden") - what is this for?
Thanks for the reviews. Marco, I agree with your nits and will incorporate. Surkov, I agree partially with yours and will modify the patch :)

Surkov, I didn't go with IsDefinedAttr, because for some (non-aria) attributes foo-"undefined" is consider defined. I wanted the method name to be self documenting. The other work you mentioned is captured by spin off bugs, do you still want XXX comments?
(In reply to comment #44)

> Surkov, I didn't go with IsDefinedAttr, because for some (non-aria) attributes
> foo-"undefined" is consider defined. I wanted the method name to be self
> documenting.

It's up to you actually, just the name is too long.

> The other work you mentioned is captured by spin off bugs, do you
> still want XXX comments?

Yes, it will make review process easier. I will know what and where we need to fix later.
Just a drive by addition, I have filed bug 472826 for it. Sorry for confusion,
I should have mentioned what that was for.

(In reply to comment #43)
> ACCESSIBILITY_ATOM(aria_hidden, "aria-hidden") - what is this for?
Attached patch Minor name changes and comments. (obsolete) — Splinter Review
Attachment #356188 - Flags: review?(surkov.alexander)
Attachment #356024 - Flags: review?(surkov.alexander)
Comment on attachment 356188 [details] [diff] [review]
Minor name changes and comments.


>+  /**
>+   * Any ARIA property of type boolean or NMTOKEN is undefined if the ARIA
>+   * attribute is not present, or is set to "" or "undefined". Do not call 
>+   * this method for properties of type string, decimal, IDREF or IDREFS.
>+   * 
>+   * Return PR_TRUE if the ARIA property is defined. PR_FALSE if empty, or
>+   * literal: "undefined".

nit: and if there is no attribute, i.e. PR_FALSE if undefined, actually I think you can remove PR_FALSE or "otherwise PR_FALSE".

>+ACCESSIBILITY_ATOM(aria_hidden, "aria-hidden")

I would prefer to save this for bug 472826 like you said, please remove it.

> PRBool
> nsAccessibilityService::HasUniversalAriaProperty(nsIContent *aContent,
>                                                  nsIWeakReference *aWeakShell)
> {
>-         aContent->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_datatype) ||

nit: please disappear it in another bug 

>-          // For aria-pressed="false" or aria-pressed="true"
>-          // For simplicity, any pressed attribute indicates it's a toggle button

>+          // For button that has a defined aria-pressed value

nit: it sounds these comments are different semantically.

>       role == nsIAccessibleRole::ROLE_OUTLINEITEM ||
>-      content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_checked)) {
>+      nsAccUtils::HasDefinedARIAToken(content, nsAccessibilityAtoms::aria_checked)) {

should we separate this check because it sounds we don't need to check state? Sure we loose code unity but we exclude state calculation.

> 
>+  // Here we assume we are looking at a token based attribute

This sounds scary :) I guess we need more comprehensive comment at least.

>+  if (!nsAccUtils::HasDefinedARIAToken(aContent, *aStateMapEntry->attributeName)) {
>+    return PR_TRUE;
>+  }
>   nsAutoString attribValue;
>   if (aContent->GetAttr(kNameSpaceID_None, *aStateMapEntry->attributeName, attribValue)) {
>     if (aStateMapEntry->attributeValue == kBoolState) {
>-      // No attribute value map specified in state map entry indicates state cleared
>+      // Clear mapped state for false values

this code looks correct but it's better comment that we don't want to clear state if attribute is undefined, for example,
<xul:checkbox role="checkbox" checked="true" aria-checked="undefined"/> - here we will expose checked state even if aria-checked is presented and undefined.

more comments later.
I'm not sure changes in nsAccessible::MappedAttrState are enough. I don't see code how you won't expose checkable state for <span role="checkbox" aria-checked="undefined"/>. Please extend mochitests. Ideal you should include all aria roles there and plus html elements with aria markup. Sure it's complex therefore please choose main cases only.
testStates("emptySelected", false, false, (STATE_SELECTABLE | STATE_SELECTED));

testStates(aAccOrElmOrID, aState, aExtraState, aAbsentState,
                    aAbsentExtraState)

aState and aExtraState is not boolean arguments. 0 is better since they are numbers.
btw, probably it's worth to put your mochitests into test_nsIAccessible_states.html becasue actually there are ARIA states checks only.
btw, the change  addA11yLoadEvent(doTest); makes me nervous, do you have any idea why do mochitets fail with your patch applied without this line?
(In reply to comment #48)
> (From update of attachment 356188 [details] [diff] [review])

I agree with most nits.

Some clarifications:

> This sounds scary :) I guess we need more comprehensive comment at least.

I'll change the comment. Aaron and I discussed over IRC. We don't want to incur the performance hit of checking the attribute type.

> >+  if (!nsAccUtils::HasDefinedARIAToken(aContent, *aStateMapEntry->attributeName)) {
> >+    return PR_TRUE;
> >+  }
> >   nsAutoString attribValue;
> >   if (aContent->GetAttr(kNameSpaceID_None, *aStateMapEntry->attributeName, attribValue)) {
> >     if (aStateMapEntry->attributeValue == kBoolState) {
> >-      // No attribute value map specified in state map entry indicates state cleared
> >+      // Clear mapped state for false values
> 
> this code looks correct but it's better comment that we don't want to clear
> state if attribute is undefined, for example,
> <xul:checkbox role="checkbox" checked="true" aria-checked="undefined"/> - here
> we will expose checked state even if aria-checked is presented and undefined.
> 

I think the comment I included is sufficient here, with my changes to the previous comment as per your earlier nit :) Also, while we will do the right thing for the xul example your gave, I sure hope we don't see a lot of that!
(In reply to comment #52)
> btw, the change  addA11yLoadEvent(doTest); makes me nervous, do you have any
> idea why do mochitets fail with your patch applied without this line?

It was a timing issue that Marco and I discovered that this patch uncovered.
(In reply to comment #51)
> btw, probably it's worth to put your mochitests into
> test_nsIAccessible_states.html becasue actually there are ARIA states checks
> only.

More tests are always good. I'll add some.
Attachment #356188 - Flags: review?(surkov.alexander)
Attachment #356024 - Attachment is obsolete: true
Attachment #356188 - Attachment is obsolete: true
Attachment #356240 - Flags: review?(surkov.alexander)
Where is aria_channel in HasUniversalAriaProperty ?
Comment on attachment 356240 [details] [diff] [review]
increased test coverage. removed off-topic tweaks etc.


>-  return aContent->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_atomic) ||
>-         aContent->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_busy) ||
>-         aContent->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_channel) ||
>+  // We need to check special case token based aria attributes

I'm not strong in terms, should we say additionally that token = nmtoken, nmtokens, boolean?

>-          // For aria-pressed="false" or aria-pressed="true"
>-          // For simplicity, any pressed attribute indicates it's a toggle button
>+        if (nsAccUtils::HasDefinedARIAToken(content, nsAccessibilityAtoms::aria_pressed)) {
>+          // For simplicity, any pressed attribute except "undefined" indicates a toggle

nit: // For simplicity, any attribute with defined value indicates a toggle ? because you should say about empty and absent attribute as well.

> 
>+  // Here we *assume* we are passed a token (NMTOKEN or bool) based ARIA
>+  // attribute. Design note: we could check the attribute type, but it 
>+  // would incur a performance hit. This filter is required because the 
>+  // spec allows literal "undefined" to indicate equivalence with "", or 
>+  // the absence of the attribute.

Truly I don't like any assumption without any clarification why we can assume this. Do we have not token attributes in ARIA map?

>+  // XXX todo: report aria state changes for "undefined" literal value changes
>+  // filed as bug 472142 (requires bug 467144 and bug 467143)

is there somewhere comment about we should create/desctory an accessible when attribute is added/removed or its value is changed from defined to undefined?

>+  <script type="application/javascript">
>+    function doTest()
>+    {
>+      testStates("undefinedSelected", 0, 0, (STATE_SELECTABLE | STATE_SELECTED));
>+      testStates("absentSelected", 0, 0, (STATE_SELECTABLE | STATE_SELECTED));

I would suggest to give more descriptive id, i.e. it's nice to include role.

>+
>+      // test checkable and checked states
>+      testStates("absentChecked", 0, 0, (STATE_CHECKABLE | STATE_CHECKED));

Here you test menuitem. Would be nice to add similar tests for checkbox, they should be different from menuitems.
aria_channel as been removed from the spec.
This was another drive-by fix. I can add it back and remove it on a separate bug. 

(In reply to comment #57)
> Where is aria_channel in HasUniversalAriaProperty ?
(In reply to comment #59)
> aria_channel as been removed from the spec.
> This was another drive-by fix. I can add it back and remove it on a separate
> bug. 

yes, please. save for another bug I hope I didn't miss another similar stuffs? ;)
Attachment #356240 - Attachment is obsolete: true
Attachment #356338 - Flags: review?(surkov.alexander)
Attachment #356240 - Flags: review?(surkov.alexander)
(In reply to comment #60)
> (In reply to comment #59)
> > aria_channel as been removed from the spec.
> > This was another drive-by fix. I can add it back and remove it on a separate
> > bug. 
> 
> yes, please. save for another bug I hope I didn't miss another similar stuffs?
> ;)

Nope you caught them all. I can't sneak any fixes past you :)
aria-checked="undefined" means the item is not checkable

vs

      testStates("checkbox_checked_empty", STATE_CHECKABLE, 0, STATE_CHECKED);
      testStates("checkbox_checked_undefined", STATE_CHECKABLE, 0, STATE_CHECKED);
      testStates("checkbox_checked_absent", STATE_CHECKABLE, 0, STATE_CHECKED);

?
I would suggest to add function to avoid code duplication, for example,

function testTokenARIAAttr(aNodeOrID, aAttr, aStates)
{
  var node = getNode(aNodeOrID);
  node.setAttribute(aAttr, "true");
  testStates(node, aStates);
  .. // etc
}
(In reply to comment #63)
> aria-checked="undefined" means the item is not checkable

Except for checkboxes.  Makes sense right?  The case: <div role="checkbox" aria-checked="undefined"> is highly unusual anyways.

Aaron what do you think? Right now we expose STATE_CHECKABLE for this situation. To fix it will require an additional check somewhere.

(In reply to comment #64)
Sure I can extract a function.

Surkov any other nits before I roll another patch?
(In reply to comment #65)
> (In reply to comment #63)
> > aria-checked="undefined" means the item is not checkable
> 
> Except for checkboxes.  Makes sense right?  The case: <div role="checkbox"
> aria-checked="undefined"> is highly unusual anyways.

I don't mind if checkbox is exception. Just it must be well documented. But if we will have any exception then I'd like to see complete tests for ARIA roles (following nsARIAMap.cpp)

> Aaron what do you think? Right now we expose STATE_CHECKABLE for this
> situation. To fix it will require an additional check somewhere.

If checkbox is not an exception then I guess we can fix in nsARIAMap.cpp file.

> (In reply to comment #64)
> Sure I can extract a function.

It may not trivial with exception like checkbox. But any logical grouping is appretiated because your mochitests becomes big and not readable.
 
> Surkov any other nits before I roll another patch?

> if (!nsAccUtils::HasDefinedARIAToken(aContent, *aStateMapEntry->attributeName)) {

nit: iirc mozilla code style doesn't require to put {} for single if. Though a11y module holds many styles :)

>  // XXX todo:  invalidate accessible when aria state changes affect exposed role

that's not unique case, you should keep in mind and reflect here accessible creation/destruction based on aria attributes presence (see HasUniversalAriaProperty method)

> // XXX todo: get rid of channel and datatype (bug 472679)

it's not necessary, sorry if my words confused you. I asked for XXX comments for following up bugs only (related with this one) which makes sense. Though if you need this comment then I don't mind.

> + addA11yLoadEvent(doTest);

I introduced this method actually because of spellcheck attribute changes mochitests. There I was 100% sure we don't fire any events if document is busy. Here I don't know the reason. Possibly it makes sense to file new mochitests bug to look at problem deeply when we get the time because something may be wrong.
Checkboxes, menuitemcheckboxes, radio buttons, menuitemradio are all always checkable. The role can just override the fact of aria-checked="undefined".
(In reply to comment #66)
> (In reply to comment #65)
> > (In reply to comment #63)
> > > aria-checked="undefined" means the item is not checkable
> > 
> > Except for checkboxes.  Makes sense right?  The case: <div role="checkbox"
> > aria-checked="undefined"> is highly unusual anyways.
> 
> I don't mind if checkbox is exception. Just it must be well documented. But if
> we will have any exception then I'd like to see complete tests for ARIA roles
> (following nsARIAMap.cpp)

I'd like to see full test coverage for all roles as well but I'm wondering where this bug will end :)

> > (In reply to comment #64)
> > Sure I can extract a function.
> 
> It may not trivial with exception like checkbox. But any logical grouping is
> appretiated because your mochitests becomes big and not readable.

I'm not sure I agree, let's discuss over IRC.

> 
> > Surkov any other nits before I roll another patch?
> 
> > if (!nsAccUtils::HasDefinedARIAToken(aContent, *aStateMapEntry->attributeName)) {
> 
> nit: iirc mozilla code style doesn't require to put {} for single if. Though
> a11y module holds many styles :)

It sure does. What's your preference with single ifs?

> 
> >  // XXX todo:  invalidate accessible when aria state changes affect exposed role
> 
> that's not unique case, you should keep in mind and reflect here accessible
> creation/destruction based on aria attributes presence (see
> HasUniversalAriaProperty method)

OK but let's have that discussion on bug 472143.

> 
> > + addA11yLoadEvent(doTest);
> 
> I introduced this method actually because of spellcheck attribute changes
> mochitests. There I was 100% sure we don't fire any events if document is busy.
> Here I don't know the reason. Possibly it makes sense to file new mochitests
> bug to look at problem deeply when we get the time because something may be
> wrong.

Yes I think we should be wary that tests can fail inconsistently. This one was failing maybe 80% of the time and only when run in suite. I really hope we can figure it out soon. Adding your addA11yLoadEvent 'fixed' it for me locally. Know a good bug where we can do some follow up discussion?

Given Aaron's confirmation that roles can override aria-checked for STATE_CHECKABLE, I'll document it and repost a patch (probably tomorrow).
(In reply to comment #68)
> (In reply to comment #66)
> > (In reply to comment #65)
> > > (In reply to comment #63)
> > > > aria-checked="undefined" means the item is not checkable
> > > 
> > > Except for checkboxes.  Makes sense right?  The case: <div role="checkbox"
> > > aria-checked="undefined"> is highly unusual anyways.
> > 
> > I don't mind if checkbox is exception. Just it must be well documented. But if
> > we will have any exception then I'd like to see complete tests for ARIA roles
> > (following nsARIAMap.cpp)
> 
> I'd like to see full test coverage for all roles as well but I'm wondering
> where this bug will end :)
> 
> > > (In reply to comment #64)
> > > Sure I can extract a function.
> > 
> > It may not trivial with exception like checkbox. But any logical grouping is
> > appretiated because your mochitests becomes big and not readable.
> 
> I'm not sure I agree, let's discuss over IRC.
> 

let's save this for following bug if you like
(In reply to comment #68)

> > I introduced this method actually because of spellcheck attribute changes
> > mochitests. There I was 100% sure we don't fire any events if document is busy.
> > Here I don't know the reason. Possibly it makes sense to file new mochitests
> > bug to look at problem deeply when we get the time because something may be
> > wrong.
> 
> Yes I think we should be wary that tests can fail inconsistently. This one was
> failing maybe 80% of the time and only when run in suite. I really hope we can
> figure it out soon. Adding your addA11yLoadEvent 'fixed' it for me locally.
> Know a good bug where we can do some follow up discussion?

I guess please file new bug, point this bug there and list of unstable with this patch tests
Comment on attachment 356338 [details] [diff] [review]
Addresses Surkovs comments. Now includes 56 passing (sub)tests.

thank you for patience ;), r=me
Attachment #356338 - Flags: review?(surkov.alexander) → review+
Attachment #356338 - Flags: review?(aaronleventhal)
I think we have a couple more places to patch:
Here we are setting object attributes for ARIA properties which are undefined. We either need to not do that, or tell ATs to treat undefined and not present as the same thing (for token attributes).
http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessible.cpp#2034
It's a bit of a pain. I think what we should do is deal with the token-based ARIA object attribute mappings one at a time, and (for now until we have role extensibility) assume that everything else is a string.
Then we'd need to change nsAccUtils::IsARIAPropForObjectAttr(nsIAtom *aAtom) so that it is more like IsARIAStringPropForObjectAttr(nsIAtom *aAtom) and increase the black list to add the token ones we just did by hand.
That issue can be filed as a follow-up bug, since it's pretty big.

Nit, use use IsDefinedARIAToken in SetLiveContainerAttributes -- it needs to ignore "" and "undefined"
http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccUtils.cpp#267

Nit, use IsDefinedARIAToken for aria-level here:
http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccUtils.cpp#376

Those 2 nits are unlikely but may as well catch them now. Not so important, so r+
Attachment #356338 - Flags: review?(aaronleventhal) → review+
(In reply to comment #72)
> I think we have a couple more places to patch:
> Here we are setting object attributes for ARIA properties which are undefined.
> We either need to not do that, or tell ATs to treat undefined and not present
> as the same thing (for token attributes).
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessible.cpp#2034
> It's a bit of a pain. I think what we should do is deal with the token-based
> ARIA object attribute mappings one at a time, and (for now until we have role
> extensibility) assume that everything else is a string.
> Then we'd need to change nsAccUtils::IsARIAPropForObjectAttr(nsIAtom *aAtom) so
> that it is more like IsARIAStringPropForObjectAttr(nsIAtom *aAtom) and increase
> the black list to add the token ones we just did by hand.
> That issue can be filed as a follow-up bug, since it's pretty big.

Filed as bug 473164.

Remaining nits will be fixed soon. Thanks!
Marco this is the patch to push. I confirmed all tests pass. Thanks!
Looks great, thanks!
Pushed on Davidb's behalf:
http://hg.mozilla.org/mozilla-central/rev/499ed590749f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 356526 [details] [diff] [review]
Patch to go in. (Addresses Aaron's nits)

ARIA 1.0 compliance.
Attachment #356526 - Flags: approval1.9.1?
Flags: in-testsuite+
Comment on attachment 356526 [details] [diff] [review]
Patch to go in. (Addresses Aaron's nits)

a191=beltzner
Attachment #356526 - Flags: approval1.9.1? → approval1.9.1+
Pushed to mozilla-1.9.1 on David's behalf in changeset:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/91dc32f594e8
Keywords: fixed1.9.1
Target Milestone: --- → mozilla1.9.1b4
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre (.NET CLR 3.5.30729)
You need to log in before you can comment on or make changes to this bug.