Closed Bug 416742 Opened 12 years ago Closed 12 years ago

Regression: multiple rowgroups interfere with getColumnAtIndex()

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jdiggs, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access, regression, Whiteboard: orca:urgent)

Attachments

(7 files, 2 obsolete files)

Attached file test case
Steps to reproduce:

1. View the attached test case.
2. Using Accerciser, highlight the table
3. Type the following:

    table = acc.queryTable()
    table.getColumnAtIndex(1)

Expected results: 0
Actual results: 1

Note that this is a regression that seems to have been introduced in the 7th February build.
Joan, could you show the table with indexes as you think because I didn't get why getColumnAtIndex(1) should return 0, for example,

<table>
  <tr><td>index 0</td><td>index  1</td></tr>
  <tr><td>index 2</td><td> index 3</td</tr>
</table>
Do you mean we should take into accout caption when we calculate indexes so that caption has reserved index 0 always?
Assignee: aaronleventhal → surkov.alexander
(In reply to comment #1)
> Joan, could you show the table with indexes as you think because I didn't get
> why getColumnAtIndex(1) should return 0, for example,
> 
> <table>
>   <tr><td>index 0</td><td>index  1</td></tr>
>   <tr><td>index 2</td><td> index 3</td</tr>
> </table>
> 
In your table, it should not. In your table, the accessible at index 1 is in the 2nd column (hence getColumnAtIndex(1) should return 1).

When I file bugs, whenever possible I try to "get to the point" as it were and narrow it down to the very step/command/condition which illustrates the problem.  Looks like I went a little too narrow this time. :-)  Please allow me to try again.

Steps to reproduce:

1. View the attached test case.

2. Using Accerciser, highlight the *first cell* in the hierarchy
   (i.e. the one that contains the text "col1" and is in the 1st
   column, aka column 0).

3. Type the following in the iPython console:

   table = acc.parent.queryTable()
   table.getColumnAtIndex(acc.getIndexInParent())

Expected result:  0 - because you have highlighted a cell in column 0, gotten its index in its parent, and asked for the column of the accessible at that index.

Actual result: 1 - because there is a bug. :-)  The bug seems to be related to the presence of the caption, because when the caption is not there, you get 0 as expected.

Note that if you repeat the process for subsequent cells, namely getting their index and then asking for the column at that index, you get similar (i.e. incorrect) results.

(In reply to comment #2)
> Do you mean we should take into accout caption when we calculate indexes so
> that caption has reserved index 0 always?

Maybe.... Maybe not, because I'm not sure I follow you.  :-) The issue (from my perspective) is not what the index of the caption is, because we do not make any assumptions about a cell's index in its parent.  We always ask for the index and get its column based on that index.   We just need to get the right column back in response. :-)  Does that make sense?  

Thanks for your help with this!
Whiteboard: orca:urgent
the problem is that getColumnAtIndex(3) returns 1 instead of zero.
Attached patch patch (obsolete) — Splinter Review
Alexander: I did ask for testing.....
Summary: Regression: Table caption interferes with getColumnAtIndex() → Regression: multiple rowgroups interfere with getColumnAtIndex()
Probably the other functions that use cellMapIdx and hit a miss in the rowgroup will suffer from the same.

I will this time insist on a mochitest before reviewing a patch.
Attached file mochitest
I can land it only after bug 416553. If you see how to wide it it would be great. Thanks.
Attached patch patch2 (obsolete) — Splinter Review
Attachment #302857 - Attachment is obsolete: true
Attachment #302983 - Flags: review?(bernd_mozilla)
Depends on: 416553
>If you see how to wide it it would be great.
Mad table design: I love it.

1. Tables with multiple thead, tfoot
2. empty tables <table/> in xhtml
3. empty rowgroups, empty rows, + those just in between sane rowgroups and rows
4. zero rowspans in strict mode
5. zero colspans in strict mode
6. cellmap holes 
7. overlapping row and colspans
8. rows with less cells than cols
(In reply to comment #11)
> 1. Tables with multiple thead, tfoot
> 2. empty tables <table/> in xhtml
> 3. empty rowgroups, empty rows, + those just in between sane rowgroups and rows

rowgroups = tbody?

> 4. zero rowspans in strict mode
> 5. zero colspans in strict mode

what is zero spans
strict mode = xhtml?

> 6. cellmap holes 

?
9. border collapsed tables

strict mode:

http://developer.mozilla.org/en/docs/Mozilla%27s_DOCTYPE_sniffing

I will write some tests....
Attached file sketch of testcase
I think additional to the marked failures the last 3 tables produce wrong results for getRowAtIndex. The rowIndex does not increase for the empty rows
But I might be completely wrong butI just want to open the discussion.
Attachment #302983 - Flags: review?(bernd_mozilla) → review+
Bernd, with the patch your tables from mochitest are ok excepting tableinsane4 with overlapping cells which is really insane. If you woudn't number cells then I won't ever get where are cells actually. I'm not sure how we should deal with the table therefore I would like to open new bug for this. Does it work for you?
Attached patch patch2Splinter Review
previous patch with Bernd's mochitest without insane table
Attachment #302983 - Attachment is obsolete: true
Attachment #303858 - Flags: approval1.9?
Comment on attachment 303858 [details] [diff] [review]
patch2

I guess I need sr firstly
Attachment #303858 - Flags: approval1.9? → superreview?(roc)
re comment 16 thats fine with me
(In reply to comment #19)
> re comment 16 thats fine with me
> 

I filed bug 418159 for this.
Status: NEW → ASSIGNED
Bernd, should we take this patch or are you going to work on making rowgroup line iterators number lines according to the rowgroup, in a way that will obsolete this patch?
I will make the index relative to the rowgroup which will obsolete the patch
Could you give me details you talk about?
we hijacked your bug, this conversation is for bug 388700 :-)
Would be nice to get this in soon as possible because it's regression.
Robert, any change to get this reviewed?
Isn't this patch obsolete now that bug 388700 is fixed?
NO, this is a separate issue which should go into the tree ASAP.
Attachment #303858 - Flags: superreview?(roc) → superreview+
Attachment #303858 - Flags: approval1.9?
Comment on attachment 303858 [details] [diff] [review]
patch2

a1.9=beltzner
Attachment #303858 - Flags: approval1.9? → approval1.9+
checked in

Checking in accessible/tests/mochitest/Makefile.in;
/cvsroot/mozilla/accessible/tests/mochitest/Makefile.in,v  <--  Makefile.in
new revision: 1.7; previous revision: 1.6
done
Checking in layout/tables/nsCellMap.cpp;
/cvsroot/mozilla/layout/tables/nsCellMap.cpp,v  <--  nsCellMap.cpp
new revision: 3.143; previous revision: 3.142
done
/cvsroot/mozilla/accessible/tests/mochitest/test_table_indexes.html,v  <--  test_table_indexes.html
initial revision: 1.1

Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Alexander, I just built from source and the original problem I reported in the opening comment is still present.  Any ideas?  Thanks!

(Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9b5pre) Gecko/2008031001 Minefield/3.0b5pre)
Attached file testcase that is wfm
so we are basically at comment 3 again. IMHO comment 0 is invalid, as shown by comment 1 + 2. Comment 3 basically morphed the bug into a completely different thing. However as the attached testcase shows even comment 3 is now WFM.
btw the first td has a indexInParent of 0.
Yeah, I wasn't quite sure about what all the morphing was about. :-)  Here's what I do know:

1. Comment 0 was merely a simplification of a problem I was reporting.

2. Comment 3 contains more detailed steps that illustrate the problem, namely that the presence of a caption cause getColumnAtIndex() to return incorrect results.

3. Your test case does include some Javascript which presents a dialog box which indeed suggests that the bug is fixed.

BUT

4. The steps (and the erroneous results) I reported in Comment 0 and in Comment 3 still apply, both to my original test case and to the test case you attached (run locally).  Here's what I get from Accerciser's iPython console:

  In [1]: acc.queryText().getText(0, -1)
  Out[1]: 'col1'
  In [2]: index = acc.getIndexInParent()
  In [3]: index
  Out[3]: 1
  In [4]: acc.parent.queryTable().getColumnAtIndex(index)
  Out[4]: 1
  In [5]: acc.queryText().getText(0, -1)
  Out[5]: 'col2'
  In [6]: index = acc.getIndexInParent()
  In [7]: index
  Out[7]: 2
  In [8]: acc.parent.queryTable().getColumnAtIndex(index)
  Out[8]: 2
  In [9]: acc.queryText().getText(0, -1)
  Out[9]: 'col3'
  In [10]: index = acc.getIndexInParent()
  In [11]: index
  Out[11]: 3
  In [12]: acc.parent.queryTable().getColumnAtIndex(index)
  Out[12]: 0

5. Prior to 7 Feb's build, the above steps produced the expected results.

Please tell me what I'm missing.  Thanks!
Could you please modify the attached test cases so that they fail as you describe. I have the feeling that there is some sort of miscommunication. Having a test case which excludes  Accerciser's iPython console seems to me beneficial ( I don't have this) as it reduces complexity and further our test framework will certainly not run the Accerciser's iPython console. In general I am very keen to get this ironed out.

see 
http://mxr.mozilla.org/mozilla/source/accessible/tests/mochitest/test_nsIAccessibleTable_1.html?raw=1

how to select cells.
Sure, as I'm very keen to get it ironed out as well. :-)  And I am also interested in finding a way other than Accerciser for us to communicate about these issues because I think that might prove speedier for all involved -- even though Accerciser really is an excellent tool for finding out what you are (and aren't) communicating to us via AT-SPI. ;-) ;-) ;-) 

I'll try to look at it later today, but I'm getting ready to leave for the CSUN conference and will be gone through Saturday.  On the flip side, Marco and others will be there and hopefully we're going to have some hacking sessions.... :-)  But  failing that, early next week I should hopefully have something.

Thanks again!
IMHO, privileged javascript in HTML or XUL is the way to go (ideally mochitests but that is  too much to ask for). There is little left to discuss if all that is needed to see the bug is to run the page (locally).
Joan, if it's not suitable for you to write JS test for us then could you at least to comment what accessibles you deal with in your steps.

acc // blabla
acc.queryText().getText(0, -1) // blabla

In the meantime I haven't linux to try reproduce it with accercieser and I hope your comments help me to understand the problem. As well it would be nice to see what's expected result.
It's not so much a matter of suitability as I am happy to do what I can to facilitate more efficient communication and demonstrate problems; aside from time, it's a matter of me finding a test case that will give you a ***javascript dialog*** with the wrong results.   To restate what I said earlier:  What the javascript dialog reports seems to be correct; however, what we get via AT-SPI *for the very same test case* is not correct.  :-( I would think that these things would match.  If indeed they should match,  but don't match and we have no idea why they don't match, will I be able to come up with a javascript test that fails?? <shrugs>  But I'll give it a try during CSUN.  Maybe Marco can even get me up to speed on Mochitests -- if so,  then it won't be too much to ask for Bernd. :-)  

Anyhoo, the reason I did all the getText() calls was to let you know where I was.  But let me try again.

acc // the th that contains col1
index = acc.getIndexInParent() // index is 1
acc.parent.queryTable().getColumnAtIndex(index) // col1 is in the 1st col (i.e. 0); we get 1

acc // the th that contains col2
index = acc.getIndexInParent() // index is 2
acc.parent.queryTable().getColumnAtIndex(index) // col2 is in the 2nd col (i.e. 1); we get 2

acc // the th that contains col3
index = acc.getIndexInParent() // index is 3
acc.parent.queryTable().getColumnAtIndex(index) // col3 is in the 3rd col (i.e. 2); we get 0

Does that make sense?

BTW, my name isn't Joan. :-)  It's Joanmarie. But that being a mouthful, friends and colleagues go with Joanie or JD.
>acc // the th that contains col1
>index = acc.getIndexInParent() // index is 1
js reports 0 see attachment 308384 [details]
>acc.parent.queryTable().getColumnAtIndex(index) // col1 is in the 1st col (i.e.
>0); we get 1
js reports 0 see attachment 308384 [details]

So the issue is that we have a difference between Accerciser and js?




(In reply to comment #40)
> >acc // the th that contains col1
> >index = acc.getIndexInParent() // index is 1
> js reports 0 see attachment 308384 [details]
> >acc.parent.queryTable().getColumnAtIndex(index) // col1 is in the 1st col (i.e.
> >0); we get 1
> js reports 0 see attachment 308384 [details]
> 
> So the issue is that we have a difference between Accerciser and js?
> 

we can have difference between Accerciser and js in the case #1 because on linux side accessible tree may different than crosslplatform tree but not in the case #2.
(In reply to comment #39)
> 
> BTW, my name isn't Joan. :-)  It's Joanmarie. But that being a mouthful,
> friends and colleagues go with Joanie or JD.
> 

sorry, please excuse me for my ignorance
> sorry, please excuse me for my ignorance

Hey, no problem.  I hadn't told you.   :-)  I have very few "pet peeves" in life; you just lucked out and found one of the ones I do have. ;-)  Anyhoo....

It may be irrelevant at this point, but whatever broke things w.r.t. this bug, at least from the AT-SPI perspective, took place in the 7 Feb build.
second round:

acc // the th that contains col1
> >index = acc.getIndexInParent() // index is 1
> js reports 0 see attachment 308384 [details] [details]

so you feed the following with 1
> >acc.parent.queryTable().getColumnAtIndex(index) // col1 is in the 1st col (i.e.
> >0); we get 1
and you should get 1 here as you pushed 1

So what we have is a platform difference on windows 
getIndexInParent() returns a different value than what you need to feed into getColumnAtIndex for tables with captions?


s/So what we have is a platform difference on windows/So what we have is a platform difference between linux und windows on linux/
Now it's my turn to ask forgiveness for my ignorance.  But does that mean that I should still try to find a javascript test case or generate a Mochitest for the problem in this bug, or does it mean that doing so won't produce something that illustrates the breakage?
Joanie, could you show your accessible tree?
Joanie and I got together at CSUN to discuss this, and here's what happens:

On Linux, the hierarchy looks like this:

table
  caption
  cell 0
  cell 1

The testcase fails.

On Windows, the IAccessible2 hierarchy, viewed in AccProbe, looks like this:

table
  caption
    text leaf of caption
  textframe
    table cell 0
      text leaf of table cell 0
  textframe
    table cell 1
      text leaf of table cell 1

On Windows, the testcase always is true, because getIndexInParent always returns 0, because the only child of the textframe parent *is* the table cell.

So, why are we creating textframes in IAccessible2 for each table cell while we don't do it on Linux where aparently there is no such thing as textframes.

Depends on: 423224
Spun off bug 423224 for this.
You need to log in before you can comment on or make changes to this bug.