437980 - 9 tests fail in table_indexes.html chrome test file.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

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

Comment 1

[Marco Zehe (:MarcoZ)]

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

Comment 2

[Marco Zehe (:MarcoZ)]

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.

Comment 3

[Marco Zehe (:MarcoZ)]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 5

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

You don't need sr for this, bernd's review is enough

Comment 7

[Bernd]

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.

Comment 8

[Bernd]

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

Comment 9

[alexander :surkov (:asurkov)]

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

Comment 10

[Bernd]

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

Comment 11

[alexander :surkov (:asurkov)]

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

Comment 12

[alexander :surkov (:asurkov)]

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

[alexander :surkov (:asurkov)]

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

rerequesting review

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 15

[Bernd]

sorry for the delay, will review over the weekend

Comment 16

[Bernd]

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.

Comment 17

[alexander :surkov (:asurkov)]

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

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch2

with David's and Brad's comments

Comment 19

[Marco Zehe (:MarcoZ)]

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!

Comment 20

[alexander :surkov (:asurkov)]

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