462527 - Refactor test_bug429285.html

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

Since this is only testing states on a group of elements, fold it into test_nsIAccessible_states.html.

Comment 1

[Marco Zehe (:MarcoZ)]

Attachment: Patch

I'm not really happy with this one yet, but this is a start. My main problem is the stupid multiple calls to getFinalState. Alex, any ideas to further optimize this are welcome!

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 345723 [details] [diff] [review]
Patch


> ////////////////////////////////////////////////////////////////////////////////
> // States
> 
> const STATE_COLLAPSED = nsIAccessibleStates.STATE_COLLAPSED;
>+const STATE_DISABLED = nsIAccessibleStates.STATE_UNAVAILABLE;

I'm not sure it's good idea to change name of state.

> 
>   <script type="application/javascript">
>+    function testDisabledChildren(aID, aAcc)
>+    {
>+      // Check state of aAcc first.
>+      var state = {}, extraState = {};
>+      aAcc.getFinalState(state, extraState);

you can use getStates function defined in nsIAccessible_states.js

>+      if (state.value & state_focusable)
>+        is(state.value & STATE_DISABLED, STATE_DISABLED,
>+           "Wrong disabled state bit for " + aID + "!");

sorry, is it correct that disabled accessible is focusable?

>     function doTest()
>     {
>       testStates("combobox", STATE_COLLAPSED);
>       testStates("combobox_expanded", STATE_EXPANDED);
>+
>+      // test disabled group and all its descendants to see if they are
>+      // disabled, too. See bug 429285.
>+      var groupAcc = getAccessible("group");
>+      if (groupAcc) {
>+        // Check disabled state for group first.
>+        testStates(groupAcc, STATE_DISABLED);

to be nice we can add testInheritedStates(aRootAcc, aStates) function. I guess we could resuse it in the future.

>+  <div id="group" role="group" aria-disabled="true">
>+    <button>hi</button>
>+    <div tabindex="0" role="listbox" aria-activedescendant="item1">
>+      <div role="option" id="item1">Item 1</div>
>+      <div role="option" id="item2">Item 2</div>
>+      <div role="option" id="item3">Item 3</div>
>+      <div role="option" id="item4">Item 4</div>
>+    </div>
>+    <div role="slider" tabindex="0">A slider</div>
>+  </div>

please put comment before "div" what we test here.

Comment 3

[Marco Zehe (:MarcoZ)]

(In reply to comment #2)
> sorry, is it correct that disabled accessible is focusable?

Unfortunately, that is a matter of platform and control type combinations. On Windows, for example, a disabled button in a dialog can never receive the focus. But it is a generally focusable control anyway.
A menu item, for example a grayed out "Copy" item in an "Edit" menu, is disabled, but keyboard focusable. You can arrow onto it, and it even receives focus, but it cannot be activated, or rather, activating it results in nothing happening.

On Linux, for buttons it's the same as on Windows, but grayed out menu items aren't focusable there. So there is no general rule.

In our case, I modeled the test after the fact: Even though it is disabled, has the STATE_UNAVAILABLE set, it is still focusable.

Comment 4

[alexander :surkov (:asurkov)]

Ok, should control be focusable to check if it's unavailable?

Comment 5

[Marco Zehe (:MarcoZ)]

Attachment: Patch2

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 351341 [details] [diff] [review]
Patch2


>+
>+if (state & STATE_UNAVAILABLE)
>+  is(state & STATE_FOCUSABLE, STATE_FOCUSABLE,
>+     "Disabled " + aAccOrElmOrID + " must be focusable!");

line up this properly (two spaces indent).

> 
> function getStringStates(aAccOrElmOrID)
> {
>   var [state, extraState] = getStates(aAccOrElmOrID);
>   var list = gAccRetrieval.getStringStates(state, extraState);
> 
>   var str = "";
>   for (var index = 0; index < list.length; index++)
>     str += list.item(index) + ", ";
> 
>   return str;
> }
> 
>+function TestStatesWithDescendants(aAccOrElmOrID, aState, aExtraState, aAbsentState)

1. place this method immediately after after testStates() function
2. use lowercase 't' in function name
3. possibly testStatesInSubtree?

>+{
>+  // test this accessible first.

nit: not sure this comment is useful actually, up to you.

>+  var acc = getAccessible(aAccOrElmOrID);
>+  testStates(acc, aState, aExtraState, aAbsentState);
>+
>+  // Iterate over its children to see if the state got propagated.
>+  var children = null;
>+  try {
>+    children = acc.children;
>+  } catch(e) {}
>+  ok(children, "Could not get children for " + aAccOrElmOrID +"!");
>+
>+  if (children) {
>+    for (var i=0; i<children.length; i++) {

nit: put spaces between operands: i=0 and i<children

>+      var childAcc = children.queryElementAt(i, nsIAccessible);
>+      // Test and recurse over its children as well.

nit: this comment interferes with previous one "Iterate over its children to see if the state got propagated." Not sure, how useful this one. up to you.

>@@ -42,10 +46,21 @@
>   <pre id="test">
>   </pre>

you may want to put additional <a> something here to keep reference on the bug.

Comment 7

[Marco Zehe (:MarcoZ)]

Attachment: Patch 3

Addressed review comments, but got several failed tests. All of them were text nodes, except for one: The grouping is unavailable, but not focusable.

1. Added a condition for ROLE_GROUPING to not be focusable even though disabled.
2. Excluded all ROLE_TEXT_LEAF nodes from having their states tested.

Re-requesting review because there were some significant changes necessary.

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 351368 [details] [diff] [review]
Patch 3


>+
>+/**
>+ * Return the role of the given accessible.
>+ *
>+ * @param aAccOrElmOrID  [in] The accessible, DOM element or element ID the
>+ *                       accessible role is being requested for.
>+ */
>+function getAccRole(aAccOrElmOrID)

getRole() should be better, at least similar with getStates() function.

>+{
>+  var acc = getAccessible(aAccOrElmOrID);
>+  if (!acc)
>+    return ROLE_NOTHING;

actually it's surprise thing from the point of a11y code of view because no role and no accessible are different things. Possibly you don't care here and possibly that's ok but please document this carefully then.

>+
>+  if ((state & STATE_UNAVAILABLE)
>+      && (getAccRole(aAccOrElmOrID) != ROLE_GROUPING))

It seems it's a bit dangerous or unusual at least condition. Please consider

>+    is(state & STATE_FOCUSABLE, STATE_FOCUSABLE,
>+       "Disabled " + aAccOrElmOrID + " must be focusable!");
>+}
>+
>+function testStatesInSubtree(aAccOrElmOrID, aState, aExtraState, aAbsentState)
>+{
>+  // test accessible and its subtree for propagated states.
>+  var acc = getAccessible(aAccOrElmOrID);
>+  if (!acc)
>+    return;
>+
>+  if (acc.finalRole != ROLE_TEXT_LEAF)
>+    // text leafs don't get the states propagated.
>+    testStates(acc, aState, aExtraState, aAbsentState);

I would be more accurate to formulate this (possibly "now this method is used bla-bla only, there we don't want to check states for text leafs or something like). I guess there may be states inherited by text leaf accessible (iirc, something should be in the case of linkable accessibles).

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 351368 [details] [diff] [review]
Patch 3

canceling review request until comments are addressed

Comment 10

[Marco Zehe (:MarcoZ)]

Attachment: Patch, updated to trunk, addressing review comments.

Hoping that this addresses all concerns...

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 364899 [details] [diff] [review]
Patch, updated to trunk, addressing review comments.


>+ */
>+function getRole(aAccOrElmOrID)

I think this function's place is in role.js file. This should be more logical and go with nsIAccessible_states.js structure. As well it makes sense to make testRole from role.js to use getRole.

>+  return acc.finalRole;

it makes sense to surround this by try/catch statement like we have for testRole function.

>+
>+function testStatesInSubtree(aAccOrElmOrID, aState, aExtraState, aAbsentState)

nit: I think it's worth to keep testStatesXXX methods together. 

As well java style documentation is welcome

r=me with this fixed.

Comment 12

[Marco Zehe (:MarcoZ)]

Pushed with Surkov's comments addressed in changeset:
http://hg.mozilla.org/mozilla-central/rev/fbff31b2a151