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.

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

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.

(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?

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.

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.

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.

Will, any comments on comment #6?

I also sent an email to gnome-accessibility-devel

(In reply to comment #7) > Will, > any comments on comment #6? > I haven't given this much thought. I'll ask about, though.

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.

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?

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.

> 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 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.

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

(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.

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

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. ______ |__| | |__|__|

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?

> 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!

> 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.

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

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

Comment on attachment 260993 [details] [diff] [review] patch fine with me

patch checked in.