429285 - Propagate aria-disabled to descendants

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

The PF group resolved that aria-disabled be propagated to descendants.

Comment 1

[Aaron Leventhal]

Attachment: Check parent chain of focusable items for aria-disabled. I kept it in the universal property map so that the current element is checked even if it is not focusable.

Comment 2

[Aaron Leventhal]

Attachment: Testcase, disabled state should be set on all descendant widgets, whether ARIA or HTML

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 315952 [details] [diff] [review]
Check parent chain of focusable items for aria-disabled. I kept it in the universal property map so that the current element is checked even if it is not focusable.


>     while ((ancestorContent = ancestorContent->GetParent()) != nsnull) {
>-      if (ancestorContent->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_activedescendant)) {
>-          // ancestor has activedescendant property, this content could be active
>-        *aState |= nsIAccessibleStates::STATE_FOCUSABLE;
>+      if (ancestorContent->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::aria_disabled,
>+                                       nsAccessibilityAtoms::_true, eCaseMatters)) {
>+          // ancestor has aria-disabled property, this is disabled
nit: please align it

>+        *aState |= nsIAccessibleStates::STATE_UNAVAILABLE;
>         break;
>       }

Comment 4

[alexander :surkov (:asurkov)]

needed mochitest for this

Comment 5

[Marco Zehe (:MarcoZ)]

(In reply to comment #4)
> needed mochitest for this

Yep I am planning to convert Aaron's testcase into a mochitest.

Comment 6

[Aaron Leventhal]

Comment on attachment 315952 [details] [diff] [review]
Check parent chain of focusable items for aria-disabled. I kept it in the universal property map so that the current element is checked even if it is not focusable.

We don't absolutely have to have this for 1.9.

Comment 7

[Marco Zehe (:MarcoZ)]

Attachment: Mochitest

Mochitest based on Aaron's testcase.

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 324614 [details] [diff] [review]
Mochitest


>+    function testChildren(aID, aAcc)
>+    {
>+      // Iterate over its children to see if they are disabled, too.
>+      var children = null;
>+      try {
>+        children = aAcc.children;
>+      } catch(e) {}
>+      ok(children, "Could not get children for " + aID +"!");
>+
>+      if (children) {
>+        // Iterate over the children to see if they are disabled, too.
>+        for (var i=0; i<children.length; i++) {
>+          var childAcc = children.queryElementAt(i, nsIAccessible);
>+          var role = childAcc.finalRole;
>+          if (role != nsIAccessibleRole.ROLE_TEXT_LEAF) {
>+            testDisabled(childAcc.name, childAcc);
>+          }
>+
>+          // If it's the listbox, iterate over its children as well.
>+          if (role = nsIAccessibleRole.ROLE_LIST) {
>+            testChildren("list", childAcc);
>+          }

the rule is any focusable descendant looks for aria-disabled through parents. Why do you like to consider specific cases instead?

Comment 9

[Marco Zehe (:MarcoZ)]

(In reply to comment #8)
> the rule is any focusable descendant looks for aria-disabled through parents.
> Why do you like to consider specific cases instead?

Because the text leafs aren't focusable. And, I want to make sure that every focusable item does indeed pick up the "disabled" state from the parent chain.

Comment 10

[alexander :surkov (:asurkov)]

Marco, I mean you could write general code: run through the tree, pick up focusable accessibles and check disabled state. I think I would like it more than check of specific cases.

Comment 11

[Marco Zehe (:MarcoZ)]

Attachment: Mercurial patch with Surkov's comment addressed

This Mercurial patch contains everything including Aaron's already reviewed code patch.

In the testChildren function, I iterate over any possible children if the accessible is not of role text_leaf. Is this what you had in mind, Alex?

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 324932 [details] [diff] [review]
Mercurial patch with Surkov's comment addressed


>+    function testDisabled(aID, aAcc)
>+    {
>+      // Mapping needed state flags for easier handling.
>+      const state_disabled = 
>+            Components.interfaces.nsIAccessibleStates.STATE_UNAVAILABLE;
>+      var state = {}, extraState = {};
>+      aAcc.getFinalState(state, extraState);
>+      is(state.value & state_disabled, state_disabled,
>+         "Wrong disabled state bit for " + aID + "!");
>+    }
>+      
>+    function testChildren(aID, aAcc)
>+    {
>+      // Iterate over its children to see if they are disabled, too.
>+      var children = null;
>+      try {
>+        children = aAcc.children;
>+      } catch(e) {}
>+      ok(children, "Could not get children for " + aID +"!");
>+
>+      if (children) {
>+        // Iterate over the children to see if they are disabled, too.
>+        for (var i=0; i<children.length; i++) {
>+          var childAcc = children.queryElementAt(i, nsIAccessible);
>+          var role = childAcc.finalRole;
>+          if (role != nsIAccessibleRole.ROLE_TEXT_LEAF) {

I would suggest to not check the role, please check focusable state. It should be enough.

>+
>+      if (groupAcc) {
>+        // Check state of group first.
>+        testDisabled("group", groupAcc);
>+        testChildren("group", groupAcc);

it's better to put testDisabled into testChildren function.

Comment 13

[Marco Zehe (:MarcoZ)]

Attachment: Address Sukov's comments

Removed testDisabled function. It's no longer called more than once, so folded it into testChildren.

Also testing for focusable state now instead of role of text leaf.

Comment 14

[alexander :surkov (:asurkov)]

Good, one thing: you call getFinalState() twice for the same accessible ;)

Comment 15

[Marco Zehe (:MarcoZ)]

(In reply to comment #14)
> Good, one thing: you call getFinalState() twice for the same accessible ;)

OK, the only way I can see to fix that is to introduce another parameter to the function, call getFinalState on the group element before going into the function first, and then only call it again for the current child within the for loop. Is that acceptable?

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 324946 [details] [diff] [review]
Address Sukov's comments

possibly like this? What do you think?

>+    function testChildren(aID, aAcc)
>+    {
>+      // Mapping needed state flags for easier handling.
>+      const state_disabled = 
>+            Components.interfaces.nsIAccessibleStates.STATE_UNAVAILABLE;
>+      const state_focusable = 
>+            Components.interfaces.nsIAccessibleStates.STATE_FOCUSABLE;
>+
>+      // Check state of aAcc first.
>+      var state = {}, extraState = {};
>+      aAcc.getFinalState(state, extraState);

        if (state.value & state_focused) {
>+      is(state.value & state_disabled, state_disabled,
>+         "Wrong disabled state bit for " + aID + "!");
>+
        }

>+      // Iterate over its children to see if they are disabled, too.
>+      var children = null;
>+      try {
>+        children = aAcc.children;
>+      } catch(e) {}
>+      ok(children, "Could not get children for " + aID +"!");
>+
>+      if (children) {
>+        for (var i=0; i<children.length; i++) {
>+          var childAcc = children.queryElementAt(i, nsIAccessible);
>+            // Test and recurse over its children as well.
>+            testChildren(childAcc.name, childAcc);
>+        }
>+      }
>+    }
>+
>+    function doTest()
>+    {
>+      gAccRetrieval = Components.classes["@mozilla.org/accessibleRetrieval;1"].
>+                      getService(nsIAccessibleRetrieval);
>+
>+      var groupItem = document.getElementById("group");
>+      var groupAcc = null;
>+      try {
>+        groupAcc = gAccRetrieval.getAccessibleFor(groupItem);
>+      } catch (e) {}
>+      ok (groupAcc,
>+          "No accessible for group element!");
>+
>+      if (groupAcc) {
              var state = {}, extraState = {};
              groupAcc.getFinalState(state, extraState);
              is(state.value & state_disabled, state_disabled,
                  "Wrong disabled state bit for " + aID + "!");

>+        testChildren("group", groupAcc);
>+      }
>+      SimpleTest.finish();
>+    }

Comment 17

[Marco Zehe (:MarcoZ)]

That will test the disabled state of the group accessible twice.

Comment 18

[Marco Zehe (:MarcoZ)]

Attachment: Call getFinalState on any accessible only once.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> That will test the disabled state of the group accessible twice.
> 

Not exactly, It will get state for the group twice but won't test disabled state twice :) but your case won't work if focusable children are contained by not focusable accessible.

Comment 20

[Marco Zehe (:MarcoZ)]

Attachment: Address Surkov's comment

This should address your comment, but still call getFinalState only once for each accessible.

Comment 21

[Marco Zehe (:MarcoZ)]

Attachment: Fix not checking state for group.

Looks like your way was the correct one after all, Alex. It appears to be unavoidable to call getFinalState on the group accessible twice.

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 325147 [details] [diff] [review]
Fix not checking state for group.

r=me, thank you.

Comment 23

[Marco Zehe (:MarcoZ)]

Fixed in changeset:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/583509de7d02

Comment 24

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008062603 Minefield/3.1a1pre