416742 - Regression: multiple rowgroups interfere with getColumnAtIndex()

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Attachment: 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.

Comment 1

[alexander :surkov (:asurkov)]

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>

Comment 2

[alexander :surkov (:asurkov)]

Do you mean we should take into accout caption when we calculate indexes so that caption has reserved index 0 always?

Comment 3

[Joanmarie Diggs]

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

Comment 4

[Bernd]

Attachment: testcase run local as it requires privilegs

Comment 5

[Bernd]

the problem is that getColumnAtIndex(3) returns 1 instead of zero.

Comment 6

[Bernd]

Attachment: patch

Comment 7

[Bernd]

Alexander: I did ask for testing.....

Comment 8

[Bernd]

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.

Comment 9

[alexander :surkov (:asurkov)]

Attachment: mochitest

I can land it only after bug 416553. If you see how to wide it it would be great. Thanks.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 11

[Bernd]

>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

Comment 12

[alexander :surkov (:asurkov)]

(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 

?

Comment 13

[Bernd]

9. border collapsed tables

strict mode:

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

I will write some tests....

Comment 14

[Bernd]

Attachment: 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

Comment 15

[Bernd]

But I might be completely wrong butI just want to open the discussion.

Comment 16

[alexander :surkov (:asurkov)]

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?

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch2

previous patch with Bernd's mochitest without insane table

Comment 18

[alexander :surkov (:asurkov)]

Comment on attachment 303858 [details] [diff] [review]
patch2

I guess I need sr firstly

Comment 19

[Bernd]

re comment 16 thats fine with me

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> re comment 16 thats fine with me
> 

I filed bug 418159 for this.

Comment 21

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

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?

Comment 22

[Bernd]

I will make the index relative to the rowgroup which will obsolete the patch

Comment 23

[alexander :surkov (:asurkov)]

Could you give me details you talk about?

Comment 24

[Bernd]

we hijacked your bug, this conversation is for bug 388700 :-)

Comment 25

[alexander :surkov (:asurkov)]

Would be nice to get this in soon as possible because it's regression.

Comment 26

[alexander :surkov (:asurkov)]

Robert, any change to get this reviewed?

Comment 27

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

Isn't this patch obsolete now that bug 388700 is fixed?

Comment 28

[Bernd]

NO, this is a separate issue which should go into the tree ASAP.

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 303858 [details] [diff] [review]
patch2

a1.9=beltzner

Comment 30

[alexander :surkov (:asurkov)]

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

Comment 31

[Joanmarie Diggs]

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)

Comment 32

[Bernd]

Attachment: 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.

Comment 33

[Bernd]

Attachment: testcase with the first td

btw the first td has a indexInParent of 0.

Comment 34

[Joanmarie Diggs]

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!

Comment 35

[Bernd]

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.

Comment 36

[Joanmarie Diggs]

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!

Comment 37

[Bernd]

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

Comment 38

[alexander :surkov (:asurkov)]

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.

Comment 39

[Joanmarie Diggs]

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.

Comment 40

[Bernd]

>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?

Comment 41

[alexander :surkov (:asurkov)]

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

Comment 42

[alexander :surkov (:asurkov)]

(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

Comment 43

[Joanmarie Diggs]

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

Comment 44

[Bernd]

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?

Comment 45

[Bernd]

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/

Comment 46

[Joanmarie Diggs]

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?

Comment 47

[alexander :surkov (:asurkov)]

Joanie, could you show your accessible tree?

Comment 48

[Marco Zehe (:MarcoZ)]

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.

Comment 49

[Marco Zehe (:MarcoZ)]

Spun off bug 423224 for this.