481435 - Refactor the test_nsiAccessibleTable*.html files

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

The 4 numbered files all need to use the new infrastructure. They are very similar, so dealing with them all in one batch.

Comment 1

[Marco Zehe (:MarcoZ)]

Attachment: Patch

Note that I left the last check for accTable4.role as it is (only adjusted the constant), because testRole uses finalRole and fails. Since I don't recall whether the test for role rather than finalRole was intentional, I decided to not change the test behavior.

Comment 2

[alexander :surkov (:asurkov)]

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


>-    columnHeaderIndex =columnHeader.getIndexAt(0,2);
>+    columnHeaderIndex = columnHeader.getIndexAt(0,2);

nit: add space between 0 and 2 once you are here

>+  var accTable1 = getAccessible("table1", null, null, DONOTFAIL_IF_NO_ACC);

nit: use isAccessible function

>   accNotCreated = false;
>-  try {
>-    var accTable2 = accService.getAccessibleFor(table2);
>-  } catch (e) {
>+  var accTable2 = getAccessible("table2", null, null, DONOTFAIL_IF_NO_ACC);
>+  if (!accTable2)
>     accNotCreated = true;
>-  }
>   ok(accNotCreated, "wrongly created table accessible");

like above 
ok(!isAccessible("table2"), "wrongly created table accessible"); should be enough here

>+  var accCell = getAccessible("cell", null, null, DONOTFAIL_IF_NO_ACC);

the same


>+  var accTable3 = getAccessible("table3", [nsIAccessibleTable], null, DONOTFAIL_IF_NO_INTERFACE);

you could change isAccessible to take array of interfaces like
function isAccessible(aAccOrElmOrID, aInteraces)
{
  return getAccessible(aAccOrElmOrID, aInteraces, null, DONOTFAIL_IF_NO_ACC | DONOTFAIL_IF_NO_INTERFACE) ?
    true : false;
}

>+  var accTable4 = getAccessible("table4", [nsIAccessibleTable], null, DONOTFAIL_IF_NO_INTERFACE);

like above

>+    var accTr = getAccessible("tr", null, null, DONOTFAIL_IF_NO_ACC);

as well

>+    if (!accTr) 
>       accNotCreated = true;
>-    }
>     ok(!accNotCreated, "missed tr accessible");
> 
>-    is(accTable4.role, Ci.nsIAccessibleRole.ROLE_TABLE, "wrong role");
>+    is(accTable4.role, ROLE_TABLE, "wrong role");

any reason to test role here instead of finalRole?

>+  var accTable5 = getAccessible("table5", null, null, DONOTFAIL_IF_NO_ACC);

like above

>+  var accCell = getAccessible("t5_cell", null, null, DONOTFAIL_IF_NO_ACC);

the same

in general looks ok, r=me, thanks

Comment 3

[alexander :surkov (:asurkov)]

Ah, forgot to say. I think it's worth to rename these files to test_table_*.html files. What do you think?

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to comment #2)
> >+  var accTable4 = getAccessible("table4", [nsIAccessibleTable], null, DONOTFAIL_IF_NO_INTERFACE);
> 
> like above

No, since I need that still for more tests. I left this one unchanged.

> >-    is(accTable4.role, Ci.nsIAccessibleRole.ROLE_TABLE, "wrong role");
> >+    is(accTable4.role, ROLE_TABLE, "wrong role");
> 
> any reason to test role here instead of finalRole?

Yes, see comment #1.

(In reply to comment #3)
> Ah, forgot to say. I think it's worth to rename these files to
> test_table_*.html files. What do you think?

Agreed, done.

Comment 5

[alexander :surkov (:asurkov)]

Ok, thanks

Comment 6

[Marco Zehe (:MarcoZ)]

Pushed with surkov's comments addressed in changeset:
http://hg.mozilla.org/mozilla-central/rev/563c82941f41