375934 - support rowspan/colspan for HTML table cell accessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Evan Yan]

Attachment: simple table with rowspan

separated from bug 360184.

Currently we calculate rowIndex/colIndex of table cell accessible by treating the table as a 2D array, which has the same number of cells in each row/col.
However, when a html table cell has attribute rowspan/colspan, it won't work.

Comment 1

[Evan Yan]

Attachment: python testcase to show the problem

while run this python script, caret browse the above attached html table testcase, you'll see wrong row/col indexes printed.

Comment 2

[Evan Yan]

We could get row and column indexes of cell from DOM node, but it need to traverse the table's every row and the row's every cell to get the indexes.

I suggest we store the indexes in table cell accessible, so that we don't need to do the traversing every time. However, we will need to add an IAccessible interface of nsIAccessibleTableCell, in order to call table cell accessible's method of getRow/ColumnIndex().

I'm listening for opinions of this solution.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> We could get row and column indexes of cell from DOM node, but it need to
> traverse the table's every row and the row's every cell to get the indexes.
> 
> I suggest we store the indexes in table cell accessible, so that we don't need
> to do the traversing every time. However, we will need to add an IAccessible
> interface of nsIAccessibleTableCell, in order to call table cell accessible's
> method of getRow/ColumnIndex().
> 
> I'm listening for opinions of this solution.
> 

I'm fine to cache indexes. But I didn't get exactly how you like to find accessible cell by row and cell from table accessible and how to find row/column by index. Can you demonstrate small piece of code for this?

Comment 4

[Evan Yan]

Actually I'm going to find row/column index by the DOM node of the cell.
The code will look like this

nsCOMPtr<nsIDOMHTMLTableCellElement> cellElement(do_QueryInterface(mDOMNode));
nsCOMPtr<nsIDOMNode> rowNode;
cellElement->GetParentNode(getter_AddRefs(rowNode));
nsCOMPtr<nsIDOMHTMLTableRowElement> rowElement(do_QueryInterface(rowNode));

PRInt32 rowIndex, colIndex;
rowElment->GetRowIndex(&rowIndex);
cellElement->GetCellIndex(&colIndex);


The call of rowElement->GetRowIndex() and cellElement->GetCellIndex() need to traverse all rows in a table and all cells in a row, which is time consuming, that's why I want to cache indexes.

Comment 5

[Evan Yan]

Attachment: patch

My last comment is incorrect. That way will get the index in dom tree, but not the index rendered.
We can just use nsITableCellLayout to get row/col index.

Comment 6

[Aaron Leventhal]

I hope the AT does not assume a 2d array.

Is there any guidance in the specs? Are we sure it wouldn't be better to have some object attribute on rowspan/colspan cells, like
cell-root-index: [cell#]

That might sound crazy, but I just want to know we're doing the right thing here.

Comment 7

[Evan Yan]

Will,
any comments on comment #6?

Comment 8

[Aaron Leventhal]

I also sent an email to gnome-accessibility-devel

Comment 9

[Willie Walker]

(In reply to comment #7)
> Will,
> any comments on comment #6?
> 

I haven't given this much thought.  I'll ask about, though.

Comment 10

[Aaron Leventhal]

Comment on attachment 260993 [details] [diff] [review]
patch

I'm going to wait until it's documented at least on a mailing list, what we should do:

On gnome-accessibility-devel list, Harry Yu wrote:
> OK, I see the problem.
> Evan,  could you please have a discussion meeting with Ginn, Li Yuan and Jeff together to see whether you could find a solution? Please send the results back to this list.
> Bill, Peter and Willie, we'd like to hear your feedback for this issue, too.

Comment 11

[parente]

The AT-SPI Table interface has:

long 	getRowExtentAt (in long row, in long column)
long 	getColumnExtentAt (in long row, in long column)

in addition to the normal:

long 	getRowAtIndex (in long index)
long 	getColumnAtIndex (in long index)

Do these factor into the solution at all?

Comment 12

[Joanmarie Diggs]

I'm not Bill, Peter, or Will, but....

getRowExtentAt() and getColumnExtentAt() already seem to be giving the correct values when a cell spans more than one row and/or column.

If getRowAtIndex() and getColumnAtIndex() were to return the correct values, I think we'd be set.  Right now we're not getting correct values for any cell (including those that don't span multiple rows/columns) which follows a cell that spans more than one row and/or column.

As for what information to provide for the cells that do span multiple rows/columns.  I'd suggest: getRowAtIndex() would return the uppermost row occupied by the cell. Likewise, getColumnAtIndex() would return the leftmost column occupied by the cell.

If I'm missing the point/question/whathaveyou, my apologies.

Comment 13

[Willie Walker]

> As for what information to provide for the cells that do span multiple
> rows/columns.  I'd suggest: getRowAtIndex() would return the uppermost row
> occupied by the cell. Likewise, getColumnAtIndex() would return the leftmost
> column occupied by the cell.

That sounds good to me, too.  I didn't write the spec, though, so I may not be fully qualified to interpret it.  :-)

Comment 14

[Evan Yan]

Comment on attachment 260993 [details] [diff] [review]
patch

Aaron,
do you think the solution mentioned in above comments is ok?

With this patch, GetRowAtIndex() and GetColumnAtIndex() will return correct value for cells with rowspan/colspan.

GetRowExtentAt() and GetColumnExtentAt() have correct implementation and don't need to be modified. They will return correct value with given correct row index and col index parameters.

Comment 15

[Aaron Leventhal]

I think the solution mentioned sounds good. Why does the patch change GetIndexAt() ?

Comment 16

[Evan Yan]

(In reply to comment #15)
> I think the solution mentioned sounds good. Why does the patch change
> GetIndexAt() ?
> 
In GetIndexAt(), we calculated index assuming it's a perfect 2D array, which is not correct when there are cells with rowspan/colspan.

Comment 17

[Aaron Leventhal]

I'm not sure we're on the same page yet. Could everyone look at this?

0,0      1,0
       rowspan
0,1      1,1

Let's say that both cells in column 2 make up a rowspan.

I'd suggest the cell indices are still number 0, 1, 2, 3 (with 3 being for cell 1,1).

getRowAtIndex(3) would return 0
getColumnAtAndex(3) would return 1

Comment 18

[Evan Yan]

Do you mean for such a table like below, we should create 4 table cell accessibles? We should create a fake one? 
I think for DOM tree, there is only 3 table cell element, and we create 3 table cell accessibles correspondingly right now.
______
|__|  |
|__|__|

Comment 19

[Evan Yan]

Does any AT expect table cell's child index in its parent equals to (rowIndex * columnCount + columnIndex) ?

The patch will break that. But we made this:

When caret browsing in a html table, we have caret-moved event, then

    index = event.source.getIndexInParent()
    row = table.getRowAtIndex(index)
    col = table.getColumnAtIndex(index)
    rowExtent = table.getRowExtentAt(row, col)
    colExtent = table.getColumnExtentAt(row, col)

row, col, rowExtent, colExtent all have correct value, and

     index == getIndexAt(row, col)

Is that OK?

Comment 20

[Willie Walker]

> Does any AT expect table cell's child index in its parent equals to (rowIndex *
> columnCount + columnIndex) ?

Orca used to have to do this until a different bug in Firefox was fixed.  But, we try not to rely upon assumptions such as row major order.  Instead, we use the AT-SPI interfaces.  Thanks for asking!

Comment 21

[parente]

> Does any AT expect table cell's child index in its parent equals to (rowIndex *
> columnCount + columnIndex) ?

Not a requirement for LSR, as long as the transforms from 1D index to 2D row, col and vice versa work.

Comment 22

[Aaron Leventhal]

Evan, thanks for the clarification. Your solution sounds correct.

Comment 23

[Aaron Leventhal]

Moving code review to Surkov since I don't have time to give a detailed review right now.

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 260993 [details] [diff] [review]
patch

fine with me

Comment 25

[Evan Yan]

patch checked in.