Bug 4571 – rows in footer must be last in rows HTMLCollection regardless of doc. order

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

[Darin Adler]

The rows collection currently returns the rows of a table in document order. But this is incorrect. As with 
the rowIndex function (which has a working implementation that does this correctly), any rows in a table 
footer have to come after all the rows in the table body.

Two of the W3C DOM Level 2 HTML test suite tests are failing because of this issue:

    HTMLCollection07
    HTMLCollection08

[Darin Adler]

Turns out there was a third test affected:

    HTMLTableElement39

[Maks Orlovich]

Actually, rowIndex seems broken wrt to putting <thead> first from looking at 
the code (it only puts tfoot last). Do you (Darin) want a separate BR + 
testcase for that? 
 
 

[Darin Adler]

Yes, lets use a separate bug report for that.

[Darin Adler]

Looks like Maks has a great fix for this in the KDE KHTML tree. We should get that.

[Maks Orlovich]

I haven't committed it yet, but it's at 
http://www.cs.cornell.edu/~maksim/patches/table_rows_order.diff (the diff also 
includes a fix to rowIndex). The patch is only sane because we cache more stuff 
than you do, though.  
 

[Darin Adler]

Anders is looking into this.

[Vicki Murley]

Please don't forget to update the DOM catalog after fixing this (or remind me to).

[Vicki Murley]

<rdar://problem/4288297>

[Vincent Ricard]

Created attachment 16362 [details]
fix HTMLCollection07 and HTMLCollection08

This patch fixes the test cases HTMLCollection07 and HTMLCollection08, and also the bug #8133 (maybe it could be marked as duplicated?).

I extended HTMLCollection and overrided traverseNextItem() (rather than modifiy HTMLCollection::traverseNextItem(), i think is more readable and more maintenable like this.

I copy/pasted the header for the both new files, i guess the copyright are wrong; is there a header sample on the wiki or elsewhere?

I ran the tests, and i didnt see new regression.

[Maciej Stachowiak]

Comment on attachment 16362 [details]
fix HTMLCollection07 and HTMLCollection08

The general concept looks good. However, there are a few issues.

1) I see a lot of code using braces around single-line if or while bodies like the below. In WebKit coding style, we do not use braces in such cases.

+        if (table->firstTBody()) {
+            current = table->firstTBody()->rows()->firstItem();
+        }

2) In general, the logic seems more complicated than necessary. I think just by adding a flag that says whether you haven't started, are currently doing the thead, are currently doing bodies, or are currently doing the tfoot, you could make the logic much simpler. You could do the thead as the first phase, then in phase two do the normal traversal (which will find trs that sit directly in the table, which can happen in XHTML) but refusing to enter thead or tfoot elements, then finally do the tfoot if there is one.

3) It looks like if there is a tfoot, your code will miss tr elements that sit directly in the table or are otherwise misnested (which can happen with XHTML or via direct DOM operations). However, if there isn't a tfoot but there is a tbody, your code will find them.

The general approach is good and it's great that you are doing this fix, but please fix up the coding style issue and consider the correctness issue in #3. It might help to add additional test cases to cover weird edge cases like that.

[Maciej Stachowiak]

Comment on attachment 16362 [details]
fix HTMLCollection07 and HTMLCollection08

r- for above comments. Please address.

[Maciej Stachowiak]

*** Bug 8133 has been marked as a duplicate of this bug. ***

[Ian 'Hixie' Hickson]

HTML5 makes this explicit, fwiw.

[Vincent Ricard]

According to the log of r29101, HTMLCollection07 and HTMLCollection pass. So the bug is still valid?
(r29101 | [e-mail] | 2008-01-03)

[Darin Adler]

Fixed in r29101.