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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

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

Comment 1

[alexander :surkov (:asurkov)]

(In reply to comment #0)

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

is not selectable I guess?

Comment 2

[Aaron Leventhal]

Alex, good catch. Yes, that's what I meant.

Comment 3

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

Attachment: untested patch.

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 :)

Comment 4

[alexander :surkov (:asurkov)]

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.

Comment 5

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

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.

Comment 6

[Aaron Leventhal]

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

Comment 7

[alexander :surkov (:asurkov)]

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

Comment 8

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

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

Comment 9

[alexander :surkov (:asurkov)]

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.

Comment 10

[alexander :surkov (:asurkov)]

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

Comment 11

[Aaron Leventhal]

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

Comment 12

[Aaron Leventhal]

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

Comment 13

[Aaron Leventhal]

Here is a testcase for aria-selected:
http://codetalks.org/source/simple/selected.html

Comment 14

[Aaron Leventhal]

Marco, Alex -- I'd love to get this into Firefox 3.1. Any chance of that still?

Comment 15

[Marco Zehe (:MarcoZ)]

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.

Comment 16

[Aaron Leventhal]

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.

Comment 17

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

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

Comment 18

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

Filed spinoffs: bug 472142 and bug 472143 for the related work Surkov has described.

Comment 19

[Marco Zehe (:MarcoZ)]

Marking the spin-off bugs as dependencies.

Comment 20

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

Attachment: WIP - works in manual testing.

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.

Comment 21

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

Attachment: Less heavy handed. A11y mochitests now pass.

Again sorry about the littering of trailing-space-removal in this patch.

Comment 22

[Aaron Leventhal]

Comment on attachment 355852 [details] [diff] [review]
Less heavy handed. A11y mochitests now pass.

Have surkov review first then I will.

Comment 23

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

Attachment: Possible fix -- see comment.

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

Comment 24

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

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.

Comment 25

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

Attachment: patch, passes tests. (found a few more tweaks)

Comment 26

[alexander :surkov (:asurkov)]

David, mochitests are needed.

Comment 27

[alexander :surkov (:asurkov)]

Do I understand correct ARIA reserve the word "undefined" and therefore the document can't have "undefined" ID?

Comment 28

[Marco Zehe (:MarcoZ)]

Only as the value for aria-* attributes if I got this right.

Comment 29

[alexander :surkov (:asurkov)]

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

Comment 30

[Aaron Leventhal]

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 31

[alexander :surkov (:asurkov)]

Comment on attachment 355906 [details] [diff] [review]
patch, passes tests. (found a few more tweaks)

canceling review.

Comment 32

[Aaron Leventhal]

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.

Comment 33

[Marco Zehe (:MarcoZ)]

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.

Comment 34

[alexander :surkov (:asurkov)]

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

Comment 35

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

Attachment: Removed unecessary checking of non token based attributes.

Comment 36

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

Attachment: Added fix for test that failed in suite with this fix (thanks Marco)

Comment 37

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

Attachment: Includes suite test fix.

Reposting. Forgot mime type on last patch upload

Comment 38

[Marco Zehe (:MarcoZ)]

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.

Comment 39

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

Attachment: this patch includes a mochitest

Added mochitest test_aria_undefined_token.html

Comment 40

[Marco Zehe (:MarcoZ)]

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!

Comment 41

[alexander :surkov (:asurkov)]

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 ?

Comment 42

[alexander :surkov (:asurkov)]

nit: file name test_aria_undefined_token.html -> test_aria_token_attrs or something? because you check not undefined values there only.

Comment 43

[alexander :surkov (:asurkov)]

ACCESSIBILITY_ATOM(aria_hidden, "aria-hidden") - what is this for?

Comment 44

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

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?

Comment 45

[alexander :surkov (:asurkov)]

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

Comment 46

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

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?

Comment 47

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

Attachment: Minor name changes and comments.

Comment 48

[alexander :surkov (:asurkov)]

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.

Comment 49

[alexander :surkov (:asurkov)]

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.

Comment 50

[alexander :surkov (:asurkov)]

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.

Comment 51

[alexander :surkov (:asurkov)]

btw, probably it's worth to put your mochitests into test_nsIAccessible_states.html becasue actually there are ARIA states checks only.

Comment 52

[alexander :surkov (:asurkov)]

btw, the change  addA11yLoadEvent(doTest); makes me nervous, do you have any idea why do mochitets fail with your patch applied without this line?

Comment 53

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

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

Comment 54

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

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

Comment 55

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

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

Comment 56

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

Attachment: increased test coverage. removed off-topic tweaks etc.

Comment 57

[alexander :surkov (:asurkov)]

Where is aria_channel in HasUniversalAriaProperty ?

Comment 58

[alexander :surkov (:asurkov)]

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.

Comment 59

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

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 ?

Comment 60

[alexander :surkov (:asurkov)]

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

Comment 61

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

Attachment: Addresses Surkovs comments. Now includes 56 passing (sub)tests.

Comment 62

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

(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 :)

Comment 63

[alexander :surkov (:asurkov)]

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);

?

Comment 64

[alexander :surkov (:asurkov)]

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
}

Comment 65

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

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

Comment 66

[alexander :surkov (:asurkov)]

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

Comment 67

[Aaron Leventhal]

Checkboxes, menuitemcheckboxes, radio buttons, menuitemradio are all always checkable. The role can just override the fact of aria-checked="undefined".

Comment 68

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

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

Comment 69

[alexander :surkov (:asurkov)]

(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

Comment 70

[alexander :surkov (:asurkov)]

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

[alexander :surkov (:asurkov)]

Comment on attachment 356338 [details] [diff] [review]
Addresses Surkovs comments. Now includes 56 passing (sub)tests.

thank you for patience ;), r=me

Comment 72

[Aaron Leventhal]

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+

Comment 73

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

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

Comment 74

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

Attachment: Patch to go in. (Addresses Aaron's nits)

Marco this is the patch to push. I confirmed all tests pass. Thanks!

Comment 75

[Aaron Leventhal]

Looks great, thanks!

Comment 76

[Marco Zehe (:MarcoZ)]

Pushed on Davidb's behalf:
http://hg.mozilla.org/mozilla-central/rev/499ed590749f

Comment 77

[Marco Zehe (:MarcoZ)]

Comment on attachment 356526 [details] [diff] [review]
Patch to go in. (Addresses Aaron's nits)

ARIA 1.0 compliance.

Comment 78

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 356526 [details] [diff] [review]
Patch to go in. (Addresses Aaron's nits)

a191=beltzner

Comment 79

[Marco Zehe (:MarcoZ)]

Pushed to mozilla-1.9.1 on David's behalf in changeset:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/91dc32f594e8

Comment 80

[Marco Zehe (:MarcoZ)]

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)