The exact log is: Passed: 248 Failed: 9 Todo: 0 not ok - tableinsane2: Can't get cell accessible at row = 1, column = 0 not ok - tableinsane2: Can't get cell accessible at row = 1, column = 1 not ok - tableinsane2: Can't get cell accessible at row = 1, column = 2 not ok - tableinsane2: cell index from object attributes of cell accessible isn't corrent. got 8, expected 9 not ok - tableinsane2: Can't get cell accessible at row = 4, column = 3 not ok - tableinsane2: Can't get cell accessible at row = 4, column = 4 not ok - tableinsane4: Can't get cell accessible at row = 1, column = 0 not ok - tableinsane4: Can't get cell accessible at row = 1, column = 1 not ok - tableinsane4: Can't get cell accessible at row = 1, column = 2

(In reply to comment #0) > not ok - tableinsane2: Can't get cell accessible at row = 1, column = 0 > not ok - tableinsane2: Can't get cell accessible at row = 1, column = 1 > not ok - tableinsane2: Can't get cell accessible at row = 1, column = 2 These are the three tests pointing at the tbody element that only contains a tr element without any td's. Have we ever created cell accessibles for these? The current tests would appear so. If this is no longer true, the tests need to be updated to reflect that. > not ok - tableinsane2: cell index from object attributes of cell accessible > isn't corrent. got 8, expected 9 Not sure about this one, would appear our index calculation is broken. Surkov, any ideas on this one? > not ok - tableinsane2: Can't get cell accessible at row = 4, column = 3 > not ok - tableinsane2: Can't get cell accessible at row = 4, column = 4 Seem to be the wacky rowspan and colspan cells that are pointed to here. > not ok - tableinsane4: Can't get cell accessible at row = 1, column = 0 > not ok - tableinsane4: Can't get cell accessible at row = 1, column = 1 > not ok - tableinsane4: Can't get cell accessible at row = 1, column = 2 Same as the first three: A row with no data cells.

The index put in the attribute is incremented for each tbody element. So the first cell with value "1" actually has an index of 6 (0 to 2 are the th's, 3 to 5 the three tbody's). In addition, the index for cells with values "2" and "3" both have a value of 7.

Disabled test_table_indexes test file until these failures are resolved. See changeset: http://hg.mozilla.org/mozilla-central/index.cgi/rev/67a0cb6a163c Please turn back on as part of fix for this bug.

That's not the right way to comment things out in makefiles; see bug 445586 and http://hg.mozilla.org/mozilla-central/index.cgi/rev/bf9ed43bd679 , where I fixed it.

Comment on attachment 383696 [details] [diff] [review] patch You don't need sr for this, bernd's review is enough

Comment on attachment 383696 [details] [diff] [review] patch Why do you need to change the cellmap code at all and not just the test case? for (PRInt32 colIdx = 0; colIdx <= colCount; colIdx++) { - CellData* data = row.SafeElementAt(colIdx); - if (data && data->IsOrig()) + data = row.SafeElementAt(colIdx); + // No data means row doesn't have more cells. + if (!data) + break; + + if (data->IsOrig()) index++; Is just plain wrong, no data means there is a cellmap hole nothing more there can be data. And this is a very common scenario. Rowspans create those entries easily.

s/there can be data/ there can be data after this/

(In reply to comment #7) > Is just plain wrong, no data means there is a cellmap hole nothing more there > can be data. And this is a very common scenario. Rowspans create those entries > easily. Right, but this code means there is no orig cells in this row. I failed to think of example where orig cell can be presented after cell holes in the same row.

This doesn't answer my first question what is wrong with this code? This code is heavily used by tables internally, so before approving a change I would like to know what this changes fixes

(In reply to comment #10) > This doesn't answer my first question what is wrong with this code? Sorry, I missed your question. The problem is GetIndexByRowAndColumn/GetRowAndColumnByIndex doesn't take into account empty rows, so they aren't consistent with GetCellInfoAt/GetRows. > This code > is heavily used by tables internally, so before approving a change I would like > to know what this changes fixes Afaik this code is used by a11y module only.

(In reply to comment #11) > (In reply to comment #10) > > This doesn't answer my first question what is wrong with this code? > > Sorry, I missed your question. The problem is > GetIndexByRowAndColumn/GetRowAndColumnByIndex doesn't take into account empty > rows, so they aren't consistent with GetCellInfoAt/GetRows. Another problem of existing code if we calculate cell index by row and column and the cell is result of rowspan like <table> <tr> <td rowspan="2">0</td><td colspan="2">1</td> </tr> <tr> <td>2</td><td>3</td> </tr> </table> so if I get index for "2" then I should get 0 but I get 1.

Comment on attachment 383696 [details] [diff] [review] patch rerequesting review

Comment on attachment 383696 [details] [diff] [review] patch r=me for the /accessible part. >- return tableLayout->GetIndexByRowAndColumn(aRow, aColumn, aIndex); >+ rv = tableLayout->GetIndexByRowAndColumn(aRow, aColumn, aIndex); >+ if (rv == NS_TABLELAYOUT_CELL_NOT_FOUND) >+ return NS_ERROR_INVALID_ARG; >+ >+ return NS_OK; Note you could do this in one line: return (tableLayout->GetIndexByRowAndColumn(aRow, aColumn, aIndex) == NS_TABLELAYOUT_CELL_NOT_FOUND) ? NS_ERROR_INVALID_ARG : NS_OK; But I don't know the moz preferred style yet. I skimmed the tests since Marco is r? But can take another look if needed. >+ is(obtainedRowIdx, origRowIdx, >+ id + ": row for index " + idx +" is nor correct"); NIT: nor/not

sorry for the delay, will review over the weekend

Comment on attachment 383696 [details] [diff] [review] patch r+ if you fix the comment aColCount [in] the number of columns in a row s/a row/the table/ you are guaranteed that never a row will have more entries than a table has columns. I like the matrix notation for the test cases.

(In reply to comment #14) > Note you could do this in one line: > > return (tableLayout->GetIndexByRowAndColumn(aRow, aColumn, aIndex) == > NS_TABLELAYOUT_CELL_NOT_FOUND) ? NS_ERROR_INVALID_ARG : NS_OK; Yes but I would prefer long style here: we don't break 80 chars restriction and it's easier to debug.

with David's and Brad's comments

Comment on attachment 385617 [details] [diff] [review] patch2 >+ var origRowIdx = rowIdx; >+ while (origRowIdx > 0 && >+ aIdxes[rowIdx][colIdx] == aIdxes[origRowIdx - 1][colIdx]) >+ origRowIdx--; Took me a moment to figure out that this actually does work right. :) r=me, thanks!

landed on mozilla 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/643cdff78555