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

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
Disability Access APIs
P5
major
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: MarcoZ, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

Trunk
mozilla1.9beta4
access
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 9 obsolete attachments)

(Reporter)

Description

11 years ago
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?
(Assignee)

Comment 1

11 years ago
Created attachment 294763 [details] [diff] [review]
wip

wonder how much idea looks correct
+'ing with P5 priority.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P5
(Reporter)

Comment 3

11 years ago
Created attachment 295625 [details] [diff] [review]
WIP2

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
(Reporter)

Comment 4

11 years ago
Created attachment 295626 [details] [diff] [review]
WIP3, possibly a patch candidate for review

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

Updated

11 years ago
Attachment #295626 - Flags: review?(surkov.alexander)
(Reporter)

Updated

11 years ago
Blocks: 400539
(Assignee)

Comment 5

11 years ago
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?
(Reporter)

Comment 6

11 years ago
(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.
(Reporter)

Comment 7

11 years ago
Created attachment 297404 [details] [diff] [review]
wip4

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)
(Assignee)

Comment 8

11 years ago
(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?
(Reporter)

Comment 9

11 years ago
(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.
(Reporter)

Comment 10

11 years ago
Created attachment 297706 [details] [diff] [review]
wip 5

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)
(Reporter)

Comment 11

11 years ago
Comment on attachment 297706 [details] [diff] [review]
wip 5

Roc, is the approach taken correct?
Attachment #297706 - Flags: review?(roc)
(Reporter)

Comment 12

11 years ago
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)
(Assignee)

Comment 13

11 years ago
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+
(Reporter)

Comment 14

11 years ago
(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)

Comment 16

11 years ago
Yes, I am familiar with this code and will review this.

Comment 17

11 years ago
One first question what is the "index", could that be documented? 
(Reporter)

Comment 18

11 years ago
(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.

Comment 19

11 years ago
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.
(Reporter)

Comment 20

11 years ago
(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?

Comment 21

11 years ago
>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.

Comment 22

11 years ago
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?
(Assignee)

Comment 23

11 years ago
(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.

Comment 24

11 years ago
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.
(Assignee)

Comment 26

11 years ago
Bernd, we'll follow your way, do you have another comments?

Comment 27

11 years ago
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.
(Assignee)

Comment 28

11 years ago
(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.

Comment 29

11 years ago
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.

Comment 30

11 years ago
Please provide a new patch for review.
(Assignee)

Comment 31

11 years ago
Created attachment 300334 [details] [diff] [review]
patch10
Assignee: marco.zehe → surkov.alexander
Attachment #297706 - Attachment is obsolete: true
Attachment #300334 - Flags: review?(bernd_mozilla)
Attachment #297706 - Flags: review?(bernd_mozilla)

Comment 32

11 years ago
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.












(Assignee)

Comment 33

11 years ago
Created attachment 300604 [details] [diff] [review]
patch2
Attachment #300334 - Attachment is obsolete: true
Attachment #300604 - Flags: review?(bernd_mozilla)
Attachment #300334 - Flags: review?(bernd_mozilla)

Comment 34

11 years ago
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;

Comment 35

11 years ago
This is my last nit, the reminder is nice code. 
(Assignee)

Comment 36

11 years ago
Created attachment 300814 [details] [diff] [review]
patch3

does this one looks correct?
Attachment #300604 - Attachment is obsolete: true
Attachment #300814 - Flags: review?(bernd_mozilla)
Attachment #300604 - Flags: review?(bernd_mozilla)
(Assignee)

Updated

11 years ago
Attachment #300814 - Flags: review?(marco.zehe)

Comment 37

11 years ago
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.
(Assignee)

Comment 38

11 years ago
Created attachment 300843 [details] [diff] [review]
patch4
Attachment #300814 - Attachment is obsolete: true
Attachment #300843 - Flags: review?(bernd_mozilla)
Attachment #300814 - Flags: review?(marco.zehe)
Attachment #300814 - Flags: review?(bernd_mozilla)
(Assignee)

Updated

11 years ago
Attachment #300843 - Flags: review?(marco.zehe)

Comment 39

11 years ago
nsTableCellMap::GetRowAndColumnByIndex(PRInt32 aIndex will return -1 for a cell in the first row, or not?
(Reporter)

Comment 40

11 years ago
(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.
(Reporter)

Comment 41

11 years ago
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-
(Reporter)

Comment 42

11 years ago
(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.
(Assignee)

Comment 43

11 years ago
(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;
(Assignee)

Comment 44

11 years ago
(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?
(Reporter)

Comment 45

11 years ago
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 46

11 years ago
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+
(Assignee)

Updated

11 years ago
Attachment #300843 - Flags: superreview?(roc)
(Assignee)

Comment 47

11 years ago
Created attachment 301228 [details] [diff] [review]
patch5
Attachment #300843 - Attachment is obsolete: true
Attachment #301228 - Flags: superreview?(roc)
Attachment #300843 - Flags: superreview?(roc)

Comment 48

11 years ago
Created attachment 301351 [details]
initial sketch of a test case

Comment 49

11 years ago
IMHO the whole interface needs mochitests 
Flags: in-testsuite?

Comment 50

11 years ago
Created attachment 301352 [details]
testcase that shows how broken the current design is.

as the previous test case  both need to run locally.

Comment 51

11 years ago
current means before the patch
(Assignee)

Comment 52

11 years ago
(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.
(Assignee)

Comment 53

11 years ago
Marco, what do you think as automated testing assignee?

Comment 54

11 years ago
I filed bug 415730 for this issue.
(Reporter)

Comment 55

11 years ago
(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+
(Assignee)

Comment 57

11 years ago
(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?
(Assignee)

Comment 58

11 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9beta3 → mozilla1.9beta4
(Assignee)

Comment 59

11 years ago
cc'ing roc for question of comment #57
yes, please file a bug on that
(Reporter)

Updated

11 years ago
Blocks: 417472
You need to log in before you can comment on or make changes to this bug.