Closed Bug 410052 Opened 17 years ago Closed 16 years ago

Fix our nsHTMLAccessibleTable class so GetIndexAt and GetRowAtIndex and GetColumnAtIndex behave consistently.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: MarcoZ, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(3 files, 9 obsolete files)

While trying to fix bug 409439, Surkov and I found that nsHTMLTableAccessible::GetIndexAt will return wrong values if the table contains RowAccessibles. This may happen because a row may have an OnClick handler. In this case, the function will return the child index relative to the row, not the table. As a result, querying GetColumnAtIndex for the given index will not return the same cell.
Flags: blocking1.9?
Attached patch wip (obsolete) — Splinter Review
wonder how much idea looks correct
+'ing with P5 priority.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P5
Attached patch WIP2 (obsolete) — Splinter Review
1. Updated to trunk.
2. Made sure it compiles by adding prototypes to TableFrame.h and TableOuterFrame.h. Also removed an inappropriate return value.
Attachment #294763 - Attachment is obsolete: true
1. Added documentation to NSITableFrame.h.
2. Fixed parameters in NSHTMLTableAccessible::GetRowAtIndex, row and column parameters were swapped.

Surkov, what do you think of this one?
Attachment #295625 - Attachment is obsolete: true
Attachment #295626 - Flags: review?(surkov.alexander)
Blocks: 400539
Comment on attachment 295626 [details] [diff] [review]
WIP3, possibly a patch candidate for review


>+  nsITableLayout *tableLayout;

safely to use initialize it by nsnull

>+void
>+nsTableCellMap::GetRowAndColumnByIndex(PRInt32 aIndex,
>+                                       PRInt32 *aRow, PRInt32 *aColumn) const
>+{
>+  PRInt32 colCount = mCols.Count();
>+
>+  nsCellMap* cellMap = mFirstMap;
>+  while (cellMap) {
>+    PRInt32 rowCount = cellMap->GetRowCount();
>+    PRInt32 index = cellMap->GetIndexByRowAndColumn(colCount,
>+                                                    rowCount - 1, colCount - 1);
>+    if (aIndex > index)
>+      aIndex -= index;
>+    else
>+      cellMap->GetRowAndColumnByIndex(colCount, index, aRow, aColumn);

it should return aRow relative the cellMap so it can be not that we want to get, right?

>+  /**
>+   *
>+   */
>+  PRInt32 GetIndexByRowAndColumn(PRInt32 aRow, PRInt32 aColumn) const;

these should be documented as well, correct?
(In reply to comment #5)
> >+void
> >+nsTableCellMap::GetRowAndColumnByIndex(PRInt32 aIndex,
> >+                                       PRInt32 *aRow, PRInt32 *aColumn) const
> >+{
> >+  PRInt32 colCount = mCols.Count();
> >+
> >+  nsCellMap* cellMap = mFirstMap;
> >+  while (cellMap) {
> >+    PRInt32 rowCount = cellMap->GetRowCount();
> >+    PRInt32 index = cellMap->GetIndexByRowAndColumn(colCount,
> >+                                                    rowCount - 1, colCount - 1);
> >+    if (aIndex > index)
> >+      aIndex -= index;
> >+    else
> >+      cellMap->GetRowAndColumnByIndex(colCount, index, aRow, aColumn);
> 
> it should return aRow relative the cellMap so it can be not that we want to
> get, right?

Can you clarify? What line do you think should be changed? This is the code you wrote originally.
Attached patch wip4 (obsolete) — Splinter Review
Address Surkov's comments 1 and 3.
Attachment #295626 - Attachment is obsolete: true
Attachment #297404 - Flags: review?(surkov.alexander)
Attachment #295626 - Flags: review?(surkov.alexander)
(In reply to comment #6)
> (In reply to comment #5)
> > >+void
> > >+nsTableCellMap::GetRowAndColumnByIndex(PRInt32 aIndex,
> > >+                                       PRInt32 *aRow, PRInt32 *aColumn) const
> > >+{
> > >+  PRInt32 colCount = mCols.Count();
> > >+
> > >+  nsCellMap* cellMap = mFirstMap;
> > >+  while (cellMap) {
> > >+    PRInt32 rowCount = cellMap->GetRowCount();
> > >+    PRInt32 index = cellMap->GetIndexByRowAndColumn(colCount,
> > >+                                                    rowCount - 1, colCount - 1);
> > >+    if (aIndex > index)
> > >+      aIndex -= index;
> > >+    else
> > >+      cellMap->GetRowAndColumnByIndex(colCount, index, aRow, aColumn);
> > 
> > it should return aRow relative the cellMap so it can be not that we want to
> > get, right?
> 
> Can you clarify? What line do you think should be changed? This is the code you
> wrote originally.
> 

I'm not sure because I'm not familar with this code actually. It's better to ask firstly to review from appropriate module peer.

I just meant if found index is not in first cellMap then row probably will be relative that map. So, if we have two cellMap, they split the table on two rows and two cells. If we give index 5 then we will return 0 probably instead of 2. Though I'm not sure. Marco, did you do some tests?
(In reply to comment #8)
> I just meant if found index is not in first cellMap then row probably will be
> relative that map. So, if we have two cellMap, they split the table on two rows
> and two cells. If we give index 5 then we will return 0 probably instead of 2.
> Though I'm not sure. Marco, did you do some tests?

yep you're probably right about that. I'll adjust the function to count the number of rows that are in one cell map if the given index is not in that map, so we return a row index relative to the whole table.
I also now noticed that on the line:
> +      cellMap->GetRowAndColumnByIndex(colCount, index, aRow, aColumn);

it should be aIndex instead of index because that's the value adjusted for the already taken-care-of index.
Attached patch wip 5 (obsolete) — Splinter Review
1. address Alexander's comment in nsHTMLTableAccessible.
2. I *think* I've addressed the multiple maps problem properly. Will ask a layout peer to take a look at this to see if it is something workable or if it is completely off the mark.
Attachment #297404 - Attachment is obsolete: true
Attachment #297706 - Flags: review?(surkov.alexander)
Attachment #297404 - Flags: review?(surkov.alexander)
Comment on attachment 297706 [details] [diff] [review]
wip 5

Roc, is the approach taken correct?
Attachment #297706 - Flags: review?(roc)
Comment on attachment 297706 [details] [diff] [review]
wip 5

At Roc's request on IRC, re-sending the question to another peer. David, any comments?
Attachment #297706 - Flags: review?(roc) → review?(dbaron)
Comment on attachment 297706 [details] [diff] [review]
wip 5

looks correct, though I'm not fun of input arguments changing,  I mean you change aIndex, it's not good possibly.

in any way r=me for ally part :)
Attachment #297706 - Flags: review?(surkov.alexander) → review+
(In reply to comment #13)
> looks correct, though I'm not fun of input arguments changing,  I mean you
> change aIndex, it's not good possibly.

You're right. I'll change that with the next patch, hopefully with a couple comments from dbaron.
Comment on attachment 297706 [details] [diff] [review]
wip 5

Bernd might be better
Attachment #297706 - Flags: review?(dbaron) → review?(bernd_mozilla)
Yes, I am familiar with this code and will review this.
One first question what is the "index", could that be documented? 
(In reply to comment #17)
> One first question what is the "index", could that be documented? 

The index is a zero-based number with which each cell can be represented. Say you have a table with 2 columns and 2 rows. Index 0 would be the top left cell, index 3 the bottom right one.
I'll be on IRC later (nick MarcoZ) in case you want to discuss this one-on-one.
next question

*aRow += tempRowIdx; // if there were previous indexes, take them into account

Could you please explain why this is not a hack, but something sensible. From the function documentation, it is not obvious to me.

* @param aRow  the row coordinate to be returned

doesn't say a word that this is a in/out parameter.
(In reply to comment #19)
> next question
> 
> *aRow += tempRowIdx; // if there were previous indexes, take them into account
> 
> Could you please explain why this is not a hack, but something sensible. From
> the function documentation, it is not obvious to me.

The intent is to allow for multiple cell maps within a single table. As far as I understand, the row count for each map starts at 0. So, if I want to return the actual row within a table, which may not be in the first map, I need to add the rows I've already traversed in previous maps to get the actual row number.

> * @param aRow  the row coordinate to be returned
> 
> doesn't say a word that this is a in/out parameter.

OK, will fix that, thanks! Should it be inline in the function declaration or above that in the comment block?
>The intent is to allow for multiple cell maps within a single table
 You have one valid nsTableCellMap per table (http://mxr.mozilla.org/mozilla/source/layout/tables/nsTableFrame.h#778) There is a nextinflow issue.
See http://mxr.mozilla.org/mozilla/source/layout/tables/nsTableFrame.cpp#692
Does accessibility also work on print preview?

 and you have one nsCellMap per rowgroup as consequence multiple nsCellMap per table. see http://lxr.mozilla.org/seamonkey/source/layout/tables/nsCellMap.h#259
and http://lxr.mozilla.org/seamonkey/source/layout/tables/nsCellMap.h#294 the nsCellMaps are linked list.
But you never have multiple valid nsTableCellMap as it looks to me the patch assumes.

Sorry for beeing so slow, I thought that I was on CC on the bug but I wasn't.
NS_IMETHODIMP
+nsTableOuterFrame::GetIndexByRowAndColumn(PRInt32 aRow, PRInt32 aColumn,
+                                          PRInt32 *aIndex)
+{
+  if (!mInnerTableFrame)
+    return NS_ERROR_NOT_INITIALIZED;
+
+  nsITableLayout *inner;
+  if (NS_SUCCEEDED(CallQueryInterface(mInnerTableFrame, &inner)))
+    return inner->GetIndexByRowAndColumn(aRow, aColumn, aIndex);
+
+  return NS_ERROR_NULL_POINTER;
+}
+
Couldn't that be rewritten as:

nsTableOuterFrame::GetIndexByRowAndColumn(PRInt32 aRow, PRInt32 aColumn,
+                                          PRInt32 *aIndex)
+{
+  NS_ASSERTION(mInnerTableFrame, "no inner table frame yet?");
+ 
+  return mInnerTableFrame->GetIndexByRowAndColumn(aRow, aColumn, aIndex);
+}

Why do you QI here?
(In reply to comment #22)
> NS_IMETHODIMP
> +nsTableOuterFrame::GetIndexByRowAndColumn(PRInt32 aRow, PRInt32 aColumn,
> +                                          PRInt32 *aIndex)
> +{
> +  if (!mInnerTableFrame)
> +    return NS_ERROR_NOT_INITIALIZED;
> +
> +  nsITableLayout *inner;
> +  if (NS_SUCCEEDED(CallQueryInterface(mInnerTableFrame, &inner)))
> +    return inner->GetIndexByRowAndColumn(aRow, aColumn, aIndex);
> +
> +  return NS_ERROR_NULL_POINTER;
> +}
> +
> Couldn't that be rewritten as:
> 
> nsTableOuterFrame::GetIndexByRowAndColumn(PRInt32 aRow, PRInt32 aColumn,
> +                                          PRInt32 *aIndex)
> +{
> +  NS_ASSERTION(mInnerTableFrame, "no inner table frame yet?");
> + 
> +  return mInnerTableFrame->GetIndexByRowAndColumn(aRow, aColumn, aIndex);
> +}
> 
> Why do you QI here?
> 

iirc, it was copy/paste from antother methods, your approach should work as well.
I did clean those methods in bug 182499 in 2002, the checkin comment cites a wrong bug id. I don't see the point to QI now. 

Boris do you see any reason why we should not remove the QI.

Further to the point 

http://mxr.mozilla.org/mozilla/search?find=%252Flayout%252Ftables%252F&string=CallQueryInterface shows 5 occurrences of CallQueryInterface aren't they except the last one bogus?
The mInnerTableFrame QIs certainly look silly.  The other ones I think we do need.
Bernd, we'll follow your way, do you have another comments?
I am not a trained reviewer I am only a  happy camper in this code. I am just trying to understand what this is trying to do.

So far the nits are:

- no in/out *row arg 
- comment at one place what index is all about. Its a foreign concept for tables so it should be explained once and then referenced via @see
- remove the silly QI from outer table frame, also the ones that I added

Further I would like to learn why 

+if (data && !data->IsColSpan() && data->IsRowSpan())

the rowspan and the colspan are handled differently. Or should the code call IsOrig() instead which I would understand.

If the later is true, how has this code been tested? Or should we add some test cases to the test suites? This is not a review requirement its rather that I am curious.
(In reply to comment #27)
> I am not a trained reviewer I am only a  happy camper in this code. I am just
> trying to understand what this is trying to do.

that's ok, thank you.

> So far the nits are:
> 
> - no in/out *row arg 
> - comment at one place what index is all about. Its a foreign concept for
> tables so it should be explained once and then referenced via @see
> - remove the silly QI from outer table frame, also the ones that I added

ok. I would guess I need to up to date the patch to continue the review?

> Further I would like to learn why 
> 
> +if (data && !data->IsColSpan() && data->IsRowSpan())
> 
> the rowspan and the colspan are handled differently.

I'm sure it's misspelling. Should be !data->IsRowSpan().

> Or should the code call
> IsOrig() instead which I would understand.

I'm not sure because I'm not familiar with that code actually. All I need is to know is it colspan or rowspan area (excepting of course start cell which rowspan or colspan are started from). Is IsOrig() the same?
 
> If the later is true, how has this code been tested?

Marco will test (or tested by AT).

> Or should we add some test
> cases to the test suites? This is not a review requirement its rather that I am
> curious.
> 

I'm not sure it's needed to add some tests because it won't be used I guess outside of accessibility but we didn't take end decision about automating testing yet in the ally module. So if Marco will say it works then I would think it's enough in the meantime.
I guess you would like to use IsOrig() It returns true if 
there is a cell and it is not a colspan or rowspan.

see http://mxr.mozilla.org/mozilla/source/layout/tables/celldata.h

The proposed test will fail if IsDead() returns true for a cellmap hole.
Please provide a new patch for review.
Attached patch patch10 (obsolete) — Splinter Review
Assignee: marco.zehe → surkov.alexander
Attachment #297706 - Attachment is obsolete: true
Attachment #300334 - Flags: review?(bernd_mozilla)
Attachment #297706 - Flags: review?(bernd_mozilla)
please replace 
+      if (data && !data->IsDead() && data->IsOrig())
with 
+      if (data && data->IsOrig()) 

A originating cell is never dead.

+PRInt32
+nsTableCellMap::GetIndexByRowAndColumn(PRInt32 aRow, PRInt32 aColumn) const

...
if (index == -1)
+        return index;

seems wrong to me


it will  return -1  for the cell in row 3 as there is no cell in rowgroup 2

<table>
<tbody>
<tr><td>cell1 </td></tr></tbody>
<tbody>
<tr></tr></tbody>
<tbody>
<tr><td>cell2 </td></tr></tbody>
</table>

I can be wrong on this but thats how this code looks to me. 


The @see should be in other places where index is mentioned so that one knows where to look it up (http://java.sun.com/j2se/1.5.0/docs/tooldocs/windows/javadoc.html#@see).
It would have been nice if you would remove the other two  QI's as asked in comment. If thats not a option  please file a followup bug against me.












Attached patch patch2 (obsolete) — Splinter Review
Attachment #300334 - Attachment is obsolete: true
Attachment #300604 - Flags: review?(bernd_mozilla)
Attachment #300334 - Flags: review?(bernd_mozilla)
Comment on attachment 300604 [details] [diff] [review]
patch2

 PRInt32 cellMapIdx = cellMap->GetIndexByRowAndColumn(colCount,
+                                                           rowCount - 1,
+                                                           colCount - 1);
+      if (cellMapIdx != -1) {
+        index += cellMapIdx;

this correct if -1 is returned. The following is not. Or do miss something?

+    // if wanted index is in here.
+    PRInt32 index = cellMap->GetIndexByRowAndColumn(colCount,
+                                                    rowCount - 1, colCount - 1);
+    if (aIndex > index) {
+      // aIndex is not within this map, so decrease it by the index determined
+      // index and increase the total row index accordingly.
+      aIndex -= index;
This is my last nit, the reminder is nice code. 
Attached patch patch3 (obsolete) — Splinter Review
does this one looks correct?
Attachment #300604 - Attachment is obsolete: true
Attachment #300814 - Flags: review?(bernd_mozilla)
Attachment #300604 - Flags: review?(bernd_mozilla)
Attachment #300814 - Flags: review?(marco.zehe)
No, I did miss another on, but now that I understand it better it pretty obvious

+        row += rowCount - 1;

is wrong

lets assume a table with 3 cellmaps, 4 rows in the first 5 rows in the second and 3 rows in the last, the target cell is in the last row.

aRow should return 4+5+3-1 = 11

what will happen with this code,

row is 0 

after the first cellmap
row = 0+4-1=3
after the second cellmap:
row = 3+5-1 =7
then we find the cell in the last row
aRow= 7+ 2= 9


I am really concerned that I review a patch which has not been properly tested.

> Marco will test (or tested by AT).

Does not make me feel more confident. I prefer test code in the tree that I can judge. I kindly ask you to make this testing part of test frame work that is executed on tinderbox. This is not 2002, where such a answer would be a good answer. Even then we did run rtest.
Attached patch patch4 (obsolete) — Splinter Review
Attachment #300814 - Attachment is obsolete: true
Attachment #300843 - Flags: review?(bernd_mozilla)
Attachment #300814 - Flags: review?(marco.zehe)
Attachment #300814 - Flags: review?(bernd_mozilla)
Attachment #300843 - Flags: review?(marco.zehe)
nsTableCellMap::GetRowAndColumnByIndex(PRInt32 aIndex will return -1 for a cell in the first row, or not?
(In reply to comment #39)
> nsTableCellMap::GetRowAndColumnByIndex(PRInt32 aIndex will return -1 for a cell
> in the first row, or not?

Yes it will. aRow gets initialized with -1 at the beginning of that method, and if we find that the index is within the current cell map, if the row returned is 0, aRow will stay at -1.

nit: In nsITableLayout.h:
> +   * @note  The index is order number of the cell calculated from top left cell
> +   *        to the right bottom cell of the table.

should be "the index is the order number ...", note the missing "the" before order number.

Question: What does the hunk #1 in nsTableOuterFrame.cpp have to do with this patch? Also the other hunks in that file, except the last one, look kind of strange.
Comment on attachment 300843 [details] [diff] [review]
patch4

Please fix the logic in nsCellMap::GetRowAndColumnByIndex. You need special handlign for index 0, which is the top left cell. The way it is implemented now, index will fall to -1 immediately, and you'd get a wrong value back for the column.

Also please fix the hunks in nsTableOuterFrame.cpp, it looks like this was not diffed against latest Trunk source.
Attachment #300843 - Flags: review?(marco.zehe) → review-
(In reply to comment #40)
> (In reply to comment #39)
> > nsTableCellMap::GetRowAndColumnByIndex(PRInt32 aIndex will return -1 for a cell
> > in the first row, or not?
> 
> Yes it will. aRow gets initialized with -1 at the beginning of that method, and
> if we find that the index is within the current cell map, if the row returned
> is 0, aRow will stay at -1.

Actually I got that all wrong (shouldn't do that directly after dinner). The error is in nsCellMap::GetRowAndColumnByIndex, which needs to handle index 0 specially.
(In reply to comment #42)
> (In reply to comment #40)
> > (In reply to comment #39)
> > > nsTableCellMap::GetRowAndColumnByIndex(PRInt32 aIndex will return -1 for a cell
> > > in the first row, or not?
> > 
> > Yes it will. aRow gets initialized with -1 at the beginning of that method, and
> > if we find that the index is within the current cell map, if the row returned
> > is 0, aRow will stay at -1.
> 
> Actually I got that all wrong (shouldn't do that directly after dinner). The
> error is in nsCellMap::GetRowAndColumnByIndex, which needs to handle index 0
> specially.
> 

Could you give me details what's wrong exactly? I tried the table:
<table>
      <tr>
        <td>Cell 1</td>
        <td>Cell 2</td>
      </tr>
      <tr>
        <td>Cell 3</td>
        <td>Cell 4</td>
      </tr>
    </table>

GetRowAndColumnByIndex(0) return 0 and 0;
(In reply to comment #41)

> Also please fix the hunks in nsTableOuterFrame.cpp, it looks like this was not
> diffed against latest Trunk source.
> 

Strange because I haven't hunks (though possibly because my patch version has been up to date already, not sure). But do you mean hunks are failed?
Comment on attachment 300843 [details] [diff] [review]
patch4

You're right, I misread the if statement. R=me with nits and the diffs from the last file fixed.
Attachment #300843 - Flags: review- → review+
Comment on attachment 300843 [details] [diff] [review]
patch4

my suspicion is wrong, it only documents that renaming "row" into previousRows would be beneficial. r+ with the renaming and Marco's comments addressed.
Attachment #300843 - Flags: review?(bernd_mozilla) → review+
Attachment #300843 - Flags: superreview?(roc)
Attached patch patch5Splinter Review
Attachment #300843 - Attachment is obsolete: true
Attachment #301228 - Flags: superreview?(roc)
Attachment #300843 - Flags: superreview?(roc)
IMHO the whole interface needs mochitests 
Flags: in-testsuite?
as the previous test case  both need to run locally.
current means before the patch
(In reply to comment #49)
> IMHO the whole interface needs mochitests 
> 

We can add mochitest but we can't enable this because some another mochitests become broken with enabled accessibility. I think we'll take care of this after Firefox3.
Marco, what do you think as automated testing assignee?
I filed bug 415730 for this issue.
(In reply to comment #53)
> Marco, what do you think as automated testing assignee?

I think that we should find a way to make all accessibility-related Mochitests happen after all others are done. That way, these other don't depend on whether accessibility is on or not. I will check with Rob Campbell (robcee) to see how this can be done.

And yes, it is definitely my plan to add Mochitests for this and other interfaces that we have in the ally module.
Comment on attachment 301228 [details] [diff] [review]
patch5

rubber-stamp=me

But nsITableLayout should be deCOMtaminated.
Attachment #301228 - Flags: superreview?(roc) → superreview+
(In reply to comment #56)
> (From update of attachment 301228 [details] [diff] [review])
> rubber-stamp=me
> 
> But nsITableLayout should be deCOMtaminated.
> 

Should I file another bug for this?
checked in
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9beta3 → mozilla1.9beta4
cc'ing roc for question of comment #57
Blocks: a11ytestdev
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: