419811 - html table accessible hierarchy is broken

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Evan Yan]

There are two problems.

1) for a "display:inline" table like below
<table style="display:inline;">
   <tr><td>cell</td></tr>
</table>

we have accessible hierarchy

hypertext
-- table cell

We shouldn't create table cell accessible here.
This is regressed by bug 404881.

2) for a <tr> with aria property like below
<table>
  <tr aria-live=polite><td>cell</td></tr>
</table>

the accessible hierarchy is

table
-- hypertext
   -- table cell

We shouldn't create accessible for the <tr>.

Comment 1

[Evan Yan]

Attachment: patch

Comment 2

[Evan Yan]

Marco, could you help test the patch to make sure it won't break the fix of 404881? I can't duplicate that bug on Linux.

Comment 3

[Marco Zehe (:MarcoZ)]

Evan, this looks correct and doesn't break anyof the test cases or Mochitests on Windows.
Have you made sure that table rows with onClick handlers still have accessibles created for them? Did you test whether bug 409009 also is OK still?

Comment 4

[Evan Yan]

Marco, thanks for the testing.

The patch won't break the fix of bug 409009. However, a table row with onClick won't have accessible.

I'm a little confused.
Sometimes we need to expose a table row. But exposing table row would bring problems for table interfaces.

Take a table like below for example. The first row has onClick or aria property.
 _____________________
| cell 0,0 | cell 0,1 |
|__________|__________|
| cell 1,0 | cell 1,1 |
|__________|__________|

Accessible hierarchy is as follows:

table
--hypertext (row)
  --cell 0,0
  --cell 0,1
--cell 1,0
--cell 1,1

Then, the following code shows the problem.

index = table.getIndexAt(0, 1);
cellA = table.getAccessibleAt(0, 1);
cellB = table.getChildAtIndex(index);
cellA != cellB

Aaron/Surkov, any idea?

Comment 5

[Marco Zehe (:MarcoZ)]

Evan, your example was supposed to be fixed by bug 410052 and the mochitests for that in bug 415730.

Comment 6

[alexander :surkov (:asurkov)]

Evan, why don't we want to expose row always? table rows can have ROLE_ROW
role. We do this for multicolumn listbox.

I think it's not correct to mix table interface and accessible children. If you
deal with table and want to operate with cells then you should use table
interface only I guess.

Comment 7

[Evan Yan]

(In reply to comment #5)
> Evan, your example was supposed to be fixed by bug 410052 and the mochitests
> for that in bug 415730.
> 
I think bug 410052 didn't address the call to getChildAtIndex. And in the mochitest  I didn't see a testcase has a <tr> with onClick/aria property, so that we will create accessible for a <tr>.

I'll confirm the example with latest trunk when I'm at office tomorrow.

(In reply to comment #6)
> I think it's not correct to mix table interface and accessible children. If you
> deal with table and want to operate with cells then you should use table
> interface only I guess.
> 
I thought on that way also, but the API document says the return index from table::getIndexAt() should be "in a form usable by Accessible::getChildAtIndex."
http://www.gnome.org/~billh/at-spi-idl/html/interfaceAccessibility_1_1Table.html#a1

Comment 8

[Marco Zehe (:MarcoZ)]

We should definitely try to figure out what the best solution is for this one.

Comment 9

[Aaron Leventhal]

I agree that #1 is a problem. If we create a table cell there should be a table parent somewhere.

However, on #2 it's not so simple.
First, we cannot avoid creating accessibles for <tr>'s or any other element when there are important properties on it. For example, this is especially the case of the <tr tabindex="0">. If there is no accessible then there is nothing to fire a focus event on, and focus will get lost if the user tabs there. But it's more than focusable tr's. Anything "interesting" such as a tr which is pointed to with an accessible relation, must have an accessible object.

So I think we need to find a way to make #2 not break ATK and Orca, but it should perhaps not be a fix in Mozilla. As long as the table interface still works I think we are doing our job. If we do need to fix something in Mozilla please file a separate bug because it is confusing to have these 2 issues together.

Comment 10

[alexander :surkov (:asurkov)]

Can we gather information how AT uses table?  "in a form usable by Accessible::getChildAtIndex." is not very descriptive but could be treat as Evan suggested. I would suggest to expose accessibles for tr always at least it means stability and AT shouldn't handle different cases separately.

Comment 11

[Bernd]

>I agree that #1 is a problem. If we create a table cell there should be a table
>parent somewhere.

There is always a table frame for it, but it can be a pseudo frame, so the content will point to the parent of the table cell.

It looks to me that a accessible tree for tables which is not a complete slave of the content/layout but subject to a changing spec is a sure recipe for failure.

At least if you can't ensure that slave mode, using the cellmap will create big problems.

Comment 12

[alexander :surkov (:asurkov)]

Bernd, btw, how can I find table frame if I have table cell?

Comment 13

[Evan Yan]

Attachment: fix problem #1, but leave the <tr> issue for now

I agree that <tr> could have something interesting, and not exposing it would hide info.

For accessible interface incompatible issue, let's leave it after Firefox 3, since we haven't got complaint from AT.

Comment 14

[alexander :surkov (:asurkov)]

Evan, if I get correctly then

if (!tableContent)
  roleMapEntry = &nsARIAMap::gEmptyRoleMap;

should be enough?

Aaron got reed tryFrameOrTagName because he wanted to create specific accessible object and assign hem unknown role.

Comment 15

[Evan Yan]

(In reply to comment #14)
> Evan, if I get correctly then
> 
> if (!tableContent)
>   roleMapEntry = &nsARIAMap::gEmptyRoleMap;
> 
> should be enough?
> 
> Aaron got reed tryFrameOrTagName because he wanted to create specific
> accessible object and assign hem unknown role.
> 
I don't get why we should do this. But we need tryFrameOrTagName to prevent unwanted table cell accessible created by frame->GetAccessible()

And we shouldn't do

if (!tableContent)
  roleMapEntry = &nsARIAMap::gEmptyRoleMap;

because that will make a lot of ROLE_UNKNOWN accessible created. Take the first example in comment #0, accessible will be created for every <tr> and <td>.

Comment 16

[alexander :surkov (:asurkov)]

Evan what accessible tree do you want to get for the first example?

Comment 17

[Evan Yan]

It's like this:

unknown (tbody)
--unknow (tr)
  --unknow (td)

Comment 18

[Evan Yan]

(In reply to comment #17)
> It's like this:
> 
> unknown (tbody)
> --unknow (tr)
>   --unknow (td)
> 
That's the accessible tree with 

if (!tableContent)
  roleMapEntry = &nsARIAMap::gEmptyRoleMap;

The wanted accessible tree is just a text accessible (no accessible on ATK).

Comment 19

[alexander :surkov (:asurkov)]

Ok would it be enough

if (!tableContent)
  tryTagNameOrFrame = PR_FALSE;

Comment 20

[Evan Yan]

I don't think so. At least we need to keep 

else if (tableFrame->GetType() == nsAccessibilityAtoms::tableCellFrame) {
  tryTagNameOrFrame = PR_FALSE;

Why we still want to try frame when assigned nsARIAMap::gEmptyRoleMap?

Comment 21

[alexander :surkov (:asurkov)]

Evan, please see bug 404881 comment #25, #26.

Comment 22

[Evan Yan]

Surkov, I think in comment #9 Aaron agreed we shouldn't create cell accessible when it's not in a table

Comment 23

[alexander :surkov (:asurkov)]

Ok, Evan, do you want to put new patch per comment 20, right? Btw, does your patch use -w option?

Comment 24

[Evan Yan]

Surkov, what change do you want in the new patch? The code in comment #20 is already in the current patch.

I'll make sure the indent is correct before commit.

Comment 25

[alexander :surkov (:asurkov)]

I'm not sure about change:
if (tableAccessible && nsAccessible::Role(tableAccessible) != nsIAccessibleRole::ROLE_TABLE) {
because comment 20 seems doesn't address this.

Also do we need to keep roleMapEntry = &nsARIAMap::gEmptyRoleMap;
else if (tableFrame->GetType() == nsAccessibilityAtoms::tableCellFrame) {
because we create general accessible in this case

I would like to have mochitest for this because this code is property of major changes last time. Could you add it to the patch?

Comment 26

[Evan Yan]

(In reply to comment #25)
> I'm not sure about change:
> if (tableAccessible && nsAccessible::Role(tableAccessible) !=
> nsIAccessibleRole::ROLE_TABLE) {

If the outer accessible is not table, I think we shouldn't try frame.
Do you have a test case for this situation? I can't think of a case that an accessible has tableOuterFrame but not ROLE_TABLE.

> Also do we need to keep roleMapEntry = &nsARIAMap::gEmptyRoleMap;
> else if (tableFrame->GetType() == nsAccessibilityAtoms::tableCellFrame) {
> because we create general accessible in this case

ah, right, that should be removed.

> I would like to have mochitest for this because this code is property of major
> changes last time. Could you add it to the patch?
> 
OK, I can do that.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #26)
> (In reply to comment #25)
> > I'm not sure about change:
> > if (tableAccessible && nsAccessible::Role(tableAccessible) !=
> > nsIAccessibleRole::ROLE_TABLE) {
> 
> If the outer accessible is not table, I think we shouldn't try frame.
> Do you have a test case for this situation? I can't think of a case that an
> accessible has tableOuterFrame but not ROLE_TABLE.

Just place @role attribute, right? It seems that's what Aaron meant in bug 404881 comment #26.

Comment 28

[Evan Yan]

Attachment: addressing surkov's comments, and including mochitest

Comment 29

[Evan Yan]

Attachment: forgot to diff Makefile.in in the former patch

Comment 30

[alexander :surkov (:asurkov)]

Evan, could you put tests for all cases, for example, td in td?

Comment 31

[alexander :surkov (:asurkov)]

Comment on attachment 307707 [details] [diff] [review]
forgot to diff Makefile.in in the former patch


>+function doTest()
>+{
>+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>+  var accService = Components.classes["@mozilla.org/accessibleRetrieval;1"].
>+                   getService(Components.interfaces.nsIAccessibleRetrieval);
>+


please add comment what you test before each table test.

some general rules I think we need to follow:
1) get accessible for table and check whether it exist
2) query table interface in try/catch block, check whether it is exposed
3) you could get accessible for tr/td via accessible service or via children of table, I think you shouldn't use methods related with indexes because they have different algorithm, you could compare result with cellRefAt(). If you want to test indexes related methods with your table which is fine idea then please put your tests into upcomming mochitest like from bug 416742 where we collect wild tables.

>+  var table1 = document.getElementById("table1");
>+  var accTable1 = accService.getAccessibleFor(table1);

why do we create accessible for this?

Would be nice to put table@role="log" additionally.

Comment 32

[Evan Yan]

I think cellRefAt() depends on table layout, as well as index methods. So they're using similar algorithm, right?

Comment 33

[Evan Yan]

Attachment: updated mochitest

Comment 34

[alexander :surkov (:asurkov)]

probably I missed something but how do you test <td>cell0<td>cell1</td></td>?

Comment 35

[alexander :surkov (:asurkov)]

would be nice to check roles as well

Comment 36

[Evan Yan]

Attachment: updated again according to the discuss on irc

Comment 37

[alexander :surkov (:asurkov)]

Comment on attachment 307928 [details] [diff] [review]
updated again according to the discuss on irc


>+function doTest()
>+{
>+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

you don't need this because we run from chrome.

>+  try {
>+    var accTable1 = accService.getAccessibleFor(table1);
>+  }
>+  catch (e) {
>+    accNotCreated = true;

nit: please use
try {
} catch(e) {
}

with fixed r=me

Comment 38

[Damon Sicore (:damons)]

Comment on attachment 307928 [details] [diff] [review]
updated again according to the discuss on irc

a1.9+=damons

Comment 39

[alexander :surkov (:asurkov)]

Attachment: patch

updated to trunk, with my fixed nits

test with tr accessible getting fail because we have hierarchy:
table table
 text container tbody
   text container tr
    cell td
    cell td

Comment 40

[Evan Yan]

surkov, I didn't see the tbody accessible on a 080303 build. If there is one on latest trunk, could you help me fix that please? I'll be on travel in a few hours and don't have a working environment right now. thanks a lot!

Comment 41

[alexander :surkov (:asurkov)]

Evan, I think the problem is we have different structure on linux and windows/mac, see bug 416742 comment #48.

Comment 42

[Marco Zehe (:MarcoZ)]

(In reply to comment #41)
> Evan, I think the problem is we have different structure on linux and
> windows/mac, see bug 416742 comment #48.

Spun off bug 423224 for this.

Comment 43

[Evan Yan]

Attachment: update to trunk and fix mochitest

Actually, to test whether the tr accessible is created, just accService.getAccessibleFor() is enough. We don't need to test table.firstChild, which seems platform dependent.

Comment 44

[alexander :surkov (:asurkov)]

Comment on attachment 311120 [details] [diff] [review]
update to trunk and fix mochitest 

don't forget to move the code block into right before checking in.

Comment 45

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311120 [details] [diff] [review]
update to trunk and fix mochitest 

a=beltzner

Comment 46

[alexander :surkov (:asurkov)]

Checking in accessible/src/base/nsAccessibilityService.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v  <--  nsAccessibilityService.cpp
new revision: 1.277; previous revision: 1.276
done
Checking in accessible/tests/mochitest/Makefile.in;
/cvsroot/mozilla/accessible/tests/mochitest/Makefile.in,v  <--  Makefile.in
new revision: 1.9; previous revision: 1.8
done

Comment 47

[alexander :surkov (:asurkov)]

/cvsroot/mozilla/accessible/tests/mochitest/test_nsIAccessibleTable_4.html,v  <--  test_nsIAccessibleTable_4.html
initial revision: 1.1
done