Bug 4586 – rowIndex calculation doesn't consider head to come before body.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Maks Orlovich]

From looking at the code (and testing on konq), the rowIndex routine only 
places the footer below the bodies, but fails to place the header before them, 
which is wrong. Will attach a testcase. For completeness, I'll also attach a 
second one with invalid multiple heads/foots, but IE and FFox don't agree on 
that, so it probably doesn't matter much

[Maks Orlovich]

Created attachment 3517 [details]
normal testcase

This should output:
foot:2
body:1
head:0

but instead outputs:
foot:2
body:0
head:1

[Maks Orlovich]

Created attachment 3518 [details]
Testcase of invalid table

This one has 2 footers and 2 headers (which IIRC is invalid), and doesn't show
any sort of consistent behavior between other browsers. WinIE just orders the
extra tfoot/thead in document order (which is what khtml currently matches if
one ignores the thead issue), Firefox gives them -1 for row order. Either
behavior would be trivial to implement. I'll probably fix this in konq in a
week or so and post a diff for adoption.

[Maks Orlovich]

I committed the following to KDE SVN to fix this: 
 
 
[maksim@treetop khtml]$ ~/kde3/kdesdk/scripts/svnlastchange 
html/html_tableimpl.cpp 
------------------------------------------------------------------------ 
r453378 | orlovich | 2005-08-25 20:58:28 -0400 (Thu, 25 Aug 2005) | 3 lines 
 
Fix rowIndex to make sure the head goes first. Noticed from reading the code. 
Testcase upcoming. 
 
------------------------------------------------------------------------ 
Index: html/html_tableimpl.cpp 
=================================================================== 
--- html/html_tableimpl.cpp     (revision 453377) 
+++ html/html_tableimpl.cpp     (revision 453378) 
@@ -707,10 +707,22 @@ long HTMLTableRowElementImpl::rowIndex() 
     if ( !table || table->id() != ID_TABLE ) 
        return -1; 
 
+    HTMLTableSectionElementImpl *head = static_cast<HTMLTableElementImpl 
*>(table)->tHead(); 
     HTMLTableSectionElementImpl *foot = static_cast<HTMLTableElementImpl 
*>(table)->tFoot(); 
+ 
+    if ( head ) { 
+        const NodeImpl *row = head->firstChild(); 
+        while ( row ) { 
+            if ( row == this ) 
+                return rIndex; 
+            rIndex++; 
+            row = row->nextSibling(); 
+        } 
+    } 
+ 
     NodeImpl *node = table->firstChild(); 
     while ( node ) { 
-        if ( node != foot && (node->id() == ID_THEAD || node->id() == 
ID_TFOOT || node->id() == ID_TBODY) ) { 
+        if ( node != foot && node != head && (node->id() == ID_THEAD || 
node->id() == ID_TFOOT || node->id() == ID_TBODY) ) { 
            HTMLTableSectionElementImpl* section = 
static_cast<HTMLTableSectionElementImpl *>(node); 
            const NodeImpl *row = section->firstChild(); 
            while ( row ) { 
 

[Oliver Hunt]

nice testcase

[Eric Seidel (no email)]

Created attachment 5314 [details]
Completely blind application of Maksim's fix (untested)

[Maks Orlovich]

Thanks,  but:  
+        if (node != foot && node != foot && (node->hasTagName(theadTag) ||  
node->hasTagName(tfootTag) || node->hasTagName(tbodyTag))) {  
  
should probably have node != head && node != foot... Will reattach tweaked. 
 
  

[Maks Orlovich]

Created attachment 5318 [details]
An obvious fix to MacDome's adoptation of my patch (even more untested)

Coincidentally, the current testcase in our SVN is nicer:
http://websvn.kde.org/trunk/tests/khtmltests/regression/tests/dom/rowindex.html?rev=453382&view=markup

[Darin Adler]

Comment on attachment 5318 [details]
An obvious fix to MacDome's adoptation of my patch (even more untested)

Patch looks fine. I'd like to see it landed and have the test landed at the
same time.

[Darin Adler]

Comment on attachment 5318 [details]
An obvious fix to MacDome's adoptation of my patch (even more untested)

Will this patch fix bug 4571 too?

[Maks Orlovich]

 (In reply to comment #9) 
> (From update of attachment 5318 [details] [edit]) 
> Will this patch fix bug 4571 too? 
 
No.  
 
 
 

[Darin Adler]

I would land this now, but there's no Change Log or layout test at this point, so it's a bit of work.

[Darin Adler]

I made a layout test, wrote a change log, and updated the fix to match Firefox's behavior. Working on 
landing it now.