Refactor the test_nsiAccessibleTable*.html files

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: MarcoZ, Assigned: MarcoZ)

Tracking

({access})

Trunk
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

10 years ago
Created attachment 365448 [details] [diff] [review]
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.
Assignee: nobody → marco.zehe
Status: NEW → ASSIGNED
Attachment #365448 - Flags: review?(surkov.alexander)
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
Attachment #365448 - Flags: review?(surkov.alexander) → review+
Ah, forgot to say. I think it's worth to rename these files to test_table_*.html files. What do you think?
(Assignee)

Comment 4

10 years ago
(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.
Ok, thanks
(Assignee)

Comment 6

10 years ago
Pushed with surkov's comments addressed in changeset:
http://hg.mozilla.org/mozilla-central/rev/563c82941f41
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.