Closed Bug 437980 Opened 12 years ago Closed 11 years ago

9 tests fail in table_indexes.html chrome test file.

Categories

(Core :: Disability Access APIs, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: MarcoZ, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 441974
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.
Blocks: tablea11y
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #383696 - Flags: superreview?(roc)
Attachment #383696 - Flags: review?(marco.zehe)
Attachment #383696 - Flags: review?(bolterbugz)
Attachment #383696 - Flags: review?(bernd_mozilla)
Comment on attachment 383696 [details] [diff] [review]
patch

You don't need sr for this, bernd's review is enough
Attachment #383696 - Flags: review?(bernd_mozilla) → review-
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.
Attachment #383696 - Flags: review?(bernd_mozilla)
Comment on attachment 383696 [details] [diff] [review]
patch

rerequesting review
Attachment #383696 - Flags: review?(bolterbugz) → 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
Attachment #383696 - Flags: review?(bernd_mozilla) → review+
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.
Attached patch patch2Splinter Review
with David's and Brad's comments
Attachment #383696 - Attachment is obsolete: true
Attachment #385617 - Flags: review?(marco.zehe)
Attachment #383696 - Flags: review?(marco.zehe)
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!
Attachment #385617 - Flags: review?(marco.zehe) → review+
landed on mozilla 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/643cdff78555
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.