Closed Bug 416742 Opened 12 years ago Closed 12 years ago
Regression: multiple rowgroups interfere with get
Column At Index()
575 bytes, text/html
2.31 KB, text/html
2.15 KB, text/html
9.28 KB, text/html
12.20 KB, patch
|Details | Diff | Splinter Review|
2.44 KB, text/html
2.74 KB, text/html
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?
(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!
the problem is that getColumnAtIndex(3) returns 1 instead of zero.
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.
I can land it only after bug 416553. If you see how to wide it it would be great. Thanks.
>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....
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.
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?
previous patch with Bernd's mochitest without insane table
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
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+
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
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)
btw the first td has a indexInParent of 0.
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!
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.
>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/
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.
Spun off bug 423224 for this.
You need to log in before you can comment on or make changes to this bug.