443889 - nsIAccessible tests for lists and selects

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

HTML testcases for unordered, ordered, definition lists, and selects with size 1 (combobox) and size > 1 (listboxes).
XUL testcases for listboxes, comboboxes and richlistboxes.

Comment 1

[Marco Zehe (:MarcoZ)]

Attachment: Patch

Comment 2

[alexander :surkov (:asurkov)]

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


>+// Needed roles
>+const role_combobox = nsIAccessibleRole.ROLE_COMBOBOX;
>+const role_combobox_list = nsIAccessibleRole.ROLE_COMBOBOX_LIST;
>+const role_combobox_option = nsIAccessibleRole.ROLE_COMBOBOX_OPTION;
>+
>+// Maping needed states
>+const state_collapsed = nsIAccessibleStates.STATE_COLLAPSED;
>+const state_expanded = nsIAccessibleStates.STATE_EXPANDED;

I would suggested to keep them in uppercase:

const STATE_COLLAPSED = nsIAccessibleStates.STATE_COLLAPSED;

it should be easy to see the code. Also please move them into common.js

Comment 3

[alexander :surkov (:asurkov)]

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


>+/**
>+ * Tests a combobox or listbox accessible and its children.

please clarify terms 'combobox' and 'listbox', are they called by role?

>+ *
>+ * @param aID  The ID of the element to test.
>+ * @param aNames  The expected names for the combobox and its children.
>+ * @param aRoles  The expected roles for the combobox and its children.

nit: please find unique word for 'combobox' and 'listobx' or use them both here

nit: please clarify how names for combobox and its children are pointed by aNames

>+ * @param aStates  the states to test for.

nit: states for what?

>+ * @param aUndesiredStates  states we don't want to have.

nit: the same


>+ */
>+function testSelect(aID, aNames, aRoles, aStates, aUndesiredStates)
>+{
>+  // used to walk the tree starting at the aID's accessible.
>+  var acc = getAccessible(aID);
>+  if (!acc) {
>+    ok(false, "no accessible for " + aID + "!");
>+    return;
>+  }

getAccessible is protected already, you don't need to keep 'ok' function here.

>+  switch(role) {
>+    case role_combobox:
>+    case role_combobox_list:
>+    case role_label:
>+    case role_list:
>+      var acc = null;
>+      try {
>+        acc = aAcc.firstChild;
>+      } catch(e) {}
>+      testThis(aID, acc, aNames, aRoles, aStates, aUndesiredStates, ++aIndex);

this aID is for select (combobx or whatever), it's not for options. Please rethink this code, otherwise it will be hard to find out what's fail.

canceling review.

Comment 4

[Marco Zehe (:MarcoZ)]

Attachment: Patch2

Added more documentation, moved constants. Am not sure I have a better idea about the iterations, so commented how the actual accessible is indicated.

Comment 5

[alexander :surkov (:asurkov)]

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


>@@ -24,16 +24,36 @@ const nsIAccessibleImage = Components.in
> const nsIAccessibleImage = Components.interfaces.nsIAccessibleImage;
> const nsIAccessibleSelectable = Components.interfaces.nsIAccessibleSelectable;
> const nsIAccessibleTable = Components.interfaces.nsIAccessibleTable;
> const nsIAccessibleValue = Components.interfaces.nsIAccessibleValue;
> 
> const nsIObserverService = Components.interfaces.nsIObserverService;
> 
> const nsIDOMNode = Components.interfaces.nsIDOMNode;
>+
>+// Roles

nit:

////////////////////////////////////////////////////////////////////////////////
// Roles

>+const ROLE_COMBOBOX = nsIAccessibleRole.ROLE_COMBOBOX;
>+const ROLE_COMBOBOX_LIST = nsIAccessibleRole.ROLE_COMBOBOX_LIST;

>+
>+// Maping states

nit:

////////////////////////////////////////////////////////////////////////////////
// States

>+const STATE_COLLAPSED = nsIAccessibleStates.STATE_COLLAPSED;
>+const STATE_EXPANDED = nsIAccessibleStates.STATE_EXPANDED;


>+ *
>+ * @param aID  The ID of the base element to test.
>+ * @param aNames  The array of expected names for the accessible and its
>+ *                children.
>+ * @param aRoles  The array of expected roles for the accessible and its
>+ *                children.
>+ * @param aStates  the array of states to test for on each accessible.
>+ * @param aUndesiredStates  array of states we don't want to have for each
>+ *                          accessible.

nit: please line up params comments, should be:

>+ * @param aID                         The ID of the base element to test.
>+ * @param aNames                 The array of expected names for the accessible and its
>+ *                                                children.
>+ * @param aRoles                    The array of expected roles for the accessible and its
>+ *                children.
>+ * @param aStates                   the array of states to test for on each accessible.
>+ * @param aUndesiredStates  array of states we don't want to have for each
>+ *                                                 accessible.

nit: If you start param comments from uppercase then please do it everywhere

>+/**
>+ * Tests a single accessible.
>+ * Recursively calls itself until it reaches the last option item.
>+ * Called first time from the testSelect function.
>+ *
>+ * @param aID  the ID of the base element, used for the error messages along
>+ *             the aIndex parameter (see below).
>+ * @param aAcc  The accessible to test.
>+ * @param aNames  The names array passed in from the testSelect function.
>+ * @param aRoles  The roles array passed in from the testSelect function.
>+ * @param aStates  the states array passed in from the testSelect function.
>+ * @param aUndesiredStates  the array of undesired states.
>+ * @param aIndex  The index in the three arrays to use. Used for both the
>+ *                error message and for walking the tree downwards from the
>+ *                base accessible.
>+ */

you can use "@see testSelect" and miss params description. Sure It's not correct totally but I think it makes code a bit readable.

>+function testThis(aID, aAcc, aNames, aRoles, aStates, aUndesiredStates, aIndex)
>+{
>+  if (aIndex >= aNames.length)
>+    return;  // invalid index, end of test

nit: I don't like "invalid index" because it makes think there is error. So "end of test" is enough I think.

>+      // Select nested within label element.
>+      // XXX see bug 455482: The accName for the label, combobox, and
>+      // combobox_list contains the label text and the names of each option.
>+      // When fixing bug 455482, please fix below names and remove this comment.
>+      names = [
>+        "Search in: Newsticker Entire site", // label
>+        "Search in:", // text leaf
>+        "Search in: Newsticker Entire site", // combobox
>+        "Search in: Newsticker Entire site", // combobox_list
>+        "Newsticker", // option 1
>+        "Entire site" // Option 2
>+      ];
>+      roles = [
>+        ROLE_LABEL, // root
>+        ROLE_TEXT_LEAF, // inner text
>+        ROLE_COMBOBOX, // combobox accessible
>+        ROLE_COMBOBOX_LIST, // list accessible
>+        ROLE_COMBOBOX_OPTION, // first option
>+        ROLE_COMBOBOX_OPTION // second option
>+      ];
>+      states = [
>+        (0), // label
>+        (0), // text leaf
>+        (STATE_FOCUSABLE | STATE_HASPOPUP | STATE_COLLAPSED), // combobox
>+        (0), // combobox_list
>+        (STATE_SELECTABLE | STATE_SELECTED | STATE_FOCUSABLE | STATE_FOCUSED),

Do we expose current element as focused, right?

Comment 6

[Marco Zehe (:MarcoZ)]

Yes, we expose the selected option with STATE_FOCUSED, not on the select's accessible itself.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> Yes, we expose the selected option with STATE_FOCUSED, not on the select's
> accessible itself.

I mean we expose state focused even the accessible is not focused, that's we want to do?

Comment 8

[Marco Zehe (:MarcoZ)]

Yes, see this comment:
http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHTMLSelectAccessible.cpp#339. This is needed for JAWS.

Comment 9

[Marco Zehe (:MarcoZ)]

Pushed in changeset:
http://hg.mozilla.org/mozilla-central/rev/0d1c0185e3c5
with review comments.