html table accessible hierarchy is broken

RESOLVED FIXED in mozilla1.9

Status

()

P2
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: evan.yan, Assigned: evan.yan)

Tracking

({regression})

Trunk
mozilla1.9
x86
Linux
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

11 years ago
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>.
(Assignee)

Comment 1

11 years ago
Created attachment 305979 [details] [diff] [review]
patch
Attachment #305979 - Flags: review?(aaronleventhal)
(Assignee)

Comment 2

11 years ago
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.
(Assignee)

Updated

11 years ago
Blocks: 396346, 404881

Comment 3

11 years ago
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?

Updated

11 years ago
Attachment #305979 - Flags: review?(surkov.alexander)
(Assignee)

Comment 4

11 years ago
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

11 years ago
Evan, your example was supposed to be fixed by bug 410052 and the mochitests for that in bug 415730.
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.
(Assignee)

Comment 7

11 years ago
(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

11 years ago
We should definitely try to figure out what the best solution is for this one.
Priority: -- → P2
Target Milestone: --- → mozilla1.9

Comment 9

11 years ago
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.

Updated

11 years ago
Attachment #305979 - Flags: review?(surkov.alexander)
Attachment #305979 - Flags: review?(aaronleventhal)
Attachment #305979 - Flags: review-
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

11 years ago
>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.
Bernd, btw, how can I find table frame if I have table cell?
(Assignee)

Comment 13

11 years ago
Created attachment 306217 [details] [diff] [review]
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.
Attachment #305979 - Attachment is obsolete: true
Attachment #306217 - Flags: review?(aaronleventhal)

Updated

11 years ago
Attachment #306217 - Flags: review?(aaronleventhal) → review?(surkov.alexander)
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.
(Assignee)

Comment 15

11 years ago
(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>.
Evan what accessible tree do you want to get for the first example?
(Assignee)

Comment 17

11 years ago
It's like this:

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

Comment 18

11 years ago
(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).
Ok would it be enough

if (!tableContent)
  tryTagNameOrFrame = PR_FALSE;
(Assignee)

Comment 20

11 years ago
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?
Evan, please see bug 404881 comment #25, #26.
(Assignee)

Comment 22

11 years ago
Surkov, I think in comment #9 Aaron agreed we shouldn't create cell accessible when it's not in a table
Ok, Evan, do you want to put new patch per comment 20, right? Btw, does your patch use -w option?
(Assignee)

Comment 24

11 years ago
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.
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?
(Assignee)

Comment 26

11 years ago
(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.
(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.
(Assignee)

Comment 28

11 years ago
Created attachment 307704 [details] [diff] [review]
addressing surkov's comments, and including mochitest
Attachment #306217 - Attachment is obsolete: true
Attachment #307704 - Flags: review?(surkov.alexander)
Attachment #306217 - Flags: review?(surkov.alexander)
(Assignee)

Comment 29

11 years ago
Created attachment 307707 [details] [diff] [review]
forgot to diff Makefile.in in the former patch
Attachment #307704 - Attachment is obsolete: true
Attachment #307707 - Flags: review?(surkov.alexander)
Attachment #307704 - Flags: review?(surkov.alexander)
Evan, could you put tests for all cases, for example, td in td?
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.
(Assignee)

Comment 32

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

Comment 33

11 years ago
Created attachment 307908 [details] [diff] [review]
updated mochitest
Attachment #307707 - Attachment is obsolete: true
Attachment #307908 - Flags: review?(surkov.alexander)
Attachment #307707 - Flags: review?(surkov.alexander)
probably I missed something but how do you test <td>cell0<td>cell1</td></td>?
would be nice to check roles as well
(Assignee)

Comment 36

11 years ago
Created attachment 307928 [details] [diff] [review]
updated again according to the discuss on irc
Attachment #307908 - Attachment is obsolete: true
Attachment #307928 - Flags: review?(surkov.alexander)
Attachment #307908 - Flags: review?(surkov.alexander)
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
Attachment #307928 - Flags: review?(surkov.alexander)
Attachment #307928 - Flags: review+
Attachment #307928 - Flags: approval1.9?
Comment on attachment 307928 [details] [diff] [review]
updated again according to the discuss on irc

a1.9+=damons
Attachment #307928 - Flags: approval1.9? → approval1.9+
Created attachment 308086 [details] [diff] [review]
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
Attachment #307928 - Attachment is obsolete: true
(Assignee)

Comment 40

11 years ago
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!
Evan, I think the problem is we have different structure on linux and windows/mac, see bug 416742 comment #48.

Comment 42

11 years ago
(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.
(Assignee)

Comment 43

11 years ago
Created attachment 311120 [details] [diff] [review]
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.
Attachment #308086 - Attachment is obsolete: true
Attachment #311120 - Flags: review?(surkov.alexander)
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.
Attachment #311120 - Attachment description: update to trunk and fix mochitest → update to trunk and fix mochitest
Attachment #311120 - Flags: review?(surkov.alexander)
Attachment #311120 - Flags: review+
Attachment #311120 - Flags: approval1.9?
Comment on attachment 311120 [details] [diff] [review]
update to trunk and fix mochitest 

a=beltzner
Attachment #311120 - Flags: approval1.9? → approval1.9+
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

Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
/cvsroot/mozilla/accessible/tests/mochitest/test_nsIAccessibleTable_4.html,v  <--  test_nsIAccessibleTable_4.html
initial revision: 1.1
done
You need to log in before you can comment on or make changes to this bug.