4586 2005-08-22 08:59:19 -0700 rowIndex calculation doesn't consider head to come before body. 2006-01-13 09:52:04 -0800 1 1 1 Unclassified WebKit DOM 420+ Other Linux RESOLVED FIXED P2 Normal --- 1 maksim darin oliver oldest_to_newest 17131 0 maksim 2005-08-22 08:59:19 -0700 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 17132 1 3517 maksim 2005-08-22 09:00:42 -0700 Created attachment 3517 normal testcase This should output: foot:2 body:1 head:0 but instead outputs: foot:2 body:0 head:1 17133 2 3518 maksim 2005-08-22 09:05:17 -0700 Created attachment 3518 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. 17482 3 maksim 2005-08-25 18:22:19 -0700 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 ) { 18990 4 oliver 2005-09-07 04:30:44 -0700 nice testcase 26843 5 5314 eric 2005-12-27 15:15:17 -0800 Created attachment 5314 Completely blind application of Maksim's fix (untested) 26887 6 maksim 2005-12-27 16:25:09 -0800 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. 26890 7 5318 maksim 2005-12-27 16:29:08 -0800 Created attachment 5318 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 27277 8 5318 darin 2005-12-30 16:18:48 -0800 Comment on attachment 5318 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. 27530 9 5318 darin 2006-01-02 18:30:31 -0800 Comment on attachment 5318 An obvious fix to MacDome's adoptation of my patch (even more untested) Will this patch fix bug 4571 too? 27735 10 maksim 2006-01-04 11:54:28 -0800 (In reply to comment #9) > (From update of attachment 5318 [edit]) > Will this patch fix bug 4571 too? No. 28296 11 darin 2006-01-12 07:52:51 -0800 I would land this now, but there's no Change Log or layout test at this point, so it's a bit of work. 28369 12 darin 2006-01-13 09:49:33 -0800 I made a layout test, wrote a change log, and updated the fix to match Firefox's behavior. Working on landing it now. 3517 2005-08-22 09:00:42 -0700 2005-08-22 09:00:42 -0700 normal testcase rowindex.html text/html 391 maksim PHRhYmxlPgogICAgPHRmb290Pjx0ciBpZD0iZm9vdCI+PC90Zm9vdD4KICAgIDx0Ym9keT48dHIg aWQ9ImJvZHkiPjwvdGJvZHk+CiAgICA8dGhlYWQ+PHRyIGlkPSJoZWFkIj48L3RoZWFkPgo8L3Rh YmxlPgoKPGRpdiBpZD0ibG9nIj4KPC9kaXY+Cgo8c2NyaXB0PgoKZnVuY3Rpb24gZG9JdCgpCnsK ICAgIHJvd3MgPSBkb2N1bWVudC5nZXRFbGVtZW50c0J5VGFnTmFtZSgidHIiKTsKICAgIGZvciAo aSA9IDA7IGkgPCByb3dzLmxlbmd0aDsgKytpKQogICAgICAgIGRvY3VtZW50LmdldEVsZW1lbnRC eUlkKCJsb2ciKS5pbm5lckhUTUwgKz0gcm93c1tpXS5pZCArICI6IiArIHJvd3NbaV0ucm93SW5k ZXggKyAiPGJyPiIKfQoKPC9zY3JpcHQ+Cgo8Ym9keSBvbmxvYWQ9ImRvSXQoKSI+Cg== 3518 2005-08-22 09:05:17 -0700 2005-08-22 09:05:17 -0700 Testcase of invalid table rowindex2.html text/html 496 maksim PHRhYmxlPgogICAgPHRmb290Pjx0ciBpZD0iZm9vdCI+PC90Zm9vdD4KICAgIDx0Zm9vdD48dHIg aWQ9ImZvb3QyIj48L3Rmb290PgogICAgPHRib2R5Pjx0ciBpZD0iYm9keSI+PC90Ym9keT4KICAg IDx0Ym9keT48dHIgaWQ9ImJvZHkyIj48L3Rib2R5PgogICAgPHRoZWFkPjx0ciBpZD0iaGVhZCI+ PC90aGVhZD4KICAgIDx0aGVhZD48dHIgaWQ9ImhlYWQyIj48L3RoZWFkPgo8L3RhYmxlPgoKPGRp diBpZD0ibG9nIj4KPC9kaXY+Cgo8c2NyaXB0PgoKZnVuY3Rpb24gZG9JdCgpCnsKICAgIHJvd3Mg PSBkb2N1bWVudC5nZXRFbGVtZW50c0J5VGFnTmFtZSgidHIiKTsKICAgIGZvciAoaSA9IDA7IGkg PCByb3dzLmxlbmd0aDsgKytpKQogICAgICAgIGRvY3VtZW50LmdldEVsZW1lbnRCeUlkKCJsb2ci KS5pbm5lckhUTUwgKz0gcm93c1tpXS5pZCArICI6IiArIHJvd3NbaV0ucm93SW5kZXggKyAiPGJy PiIKfQoKPC9zY3JpcHQ+Cgo8Ym9keSBvbmxvYWQ9ImRvSXQoKSI+Cg== 5314 2005-12-27 15:15:17 -0800 2005-12-27 16:29:42 -0800 Completely blind application of Maksim's fix (untested) sad.patch text/plain 1407 eric SW5kZXg6IGh0bWxfdGFibGVpbXBsLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09ClJDUyBmaWxlOiAvY3ZzL3Jvb3Qv V2ViQ29yZS9raHRtbC9odG1sL2h0bWxfdGFibGVpbXBsLmNwcCx2CnJldHJpZXZpbmcgcmV2aXNp b24gMS43MgpkaWZmIC11IC1yMS43MiBodG1sX3RhYmxlaW1wbC5jcHAKLS0tIGh0bWxfdGFibGVp bXBsLmNwcAkyMyBEZWMgMjAwNSAxODo0NDoxNyAtMDAwMAkxLjcyCisrKyBodG1sX3RhYmxlaW1w bC5jcHAJMjcgRGVjIDIwMDUgMjM6MTU6NTUgLTAwMDAKQEAgLTg4MiwxMCArODgyLDIzIEBACiAg ICAgaWYgKCF0YWJsZSB8fCAhdGFibGUtPmhhc1RhZ05hbWUodGFibGVUYWcpKQogCXJldHVybiAt MTsKIAorICAgIEhUTUxUYWJsZVNlY3Rpb25FbGVtZW50SW1wbCAqaGVhZCA9IHN0YXRpY19jYXN0 PEhUTUxUYWJsZUVsZW1lbnRJbXBsKj4odGFibGUpLT50SGVhZCgpOwogICAgIEhUTUxUYWJsZVNl Y3Rpb25FbGVtZW50SW1wbCAqZm9vdCA9IHN0YXRpY19jYXN0PEhUTUxUYWJsZUVsZW1lbnRJbXBs ICo+KHRhYmxlKS0+dEZvb3QoKTsKKworICAgIC8vIG1ha2UgPGhlYWQ+IGNvbWUgYmVmb3JlIDxi b2R5PiA6IGh0dHA6Ly9idWd6aWxsYS5vcGVuZGFyd2luLm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDU4 NgorICAgIGlmIChoZWFkKSB7CisgICAgICAgY29uc3QgTm9kZUltcGwgKnJvdyA9IGhlYWQtPmZp cnN0Q2hpbGQoKTsKKyAgICAgICB3aGlsZSAocm93KSB7CisgICAgICAgICAgIGlmIChyb3cgPT0g dGhpcykKKyAgICAgICAgICAgICAgIHJldHVybiBySW5kZXg7CisgICAgICAgICAgIHJJbmRleCsr OworICAgICAgICAgICByb3cgPSByb3ctPm5leHRTaWJsaW5nKCk7CisgICAgICAgfQorICAgIH0K KyAgICAKICAgICBOb2RlSW1wbCAqbm9kZSA9IHRhYmxlLT5maXJzdENoaWxkKCk7CiAgICAgd2hp bGUgKG5vZGUpIHsKLSAgICAgICAgaWYgKG5vZGUgIT0gZm9vdCAmJiAobm9kZS0+aGFzVGFnTmFt ZSh0aGVhZFRhZykgfHwgbm9kZS0+aGFzVGFnTmFtZSh0Zm9vdFRhZykgfHwgbm9kZS0+aGFzVGFn TmFtZSh0Ym9keVRhZykpKSB7CisgICAgICAgIGlmIChub2RlICE9IGZvb3QgJiYgbm9kZSAhPSBm b290ICYmIChub2RlLT5oYXNUYWdOYW1lKHRoZWFkVGFnKSB8fCBub2RlLT5oYXNUYWdOYW1lKHRm b290VGFnKSB8fCBub2RlLT5oYXNUYWdOYW1lKHRib2R5VGFnKSkpIHsKIAkgICAgSFRNTFRhYmxl U2VjdGlvbkVsZW1lbnRJbXBsKiBzZWN0aW9uID0gc3RhdGljX2Nhc3Q8SFRNTFRhYmxlU2VjdGlv bkVsZW1lbnRJbXBsICo+KG5vZGUpOwogCSAgICBjb25zdCBOb2RlSW1wbCAqcm93ID0gc2VjdGlv bi0+Zmlyc3RDaGlsZCgpOwogCSAgICB3aGlsZSAoIHJvdyApIHsK 5318 2005-12-27 16:29:08 -0800 2005-12-30 16:18:48 -0800 An obvious fix to MacDome's adoptation of my patch (even more untested) mc.patch text/plain 1407 maksim SW5kZXg6IGh0bWxfdGFibGVpbXBsLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09ClJDUyBmaWxlOiAvY3ZzL3Jvb3Qv V2ViQ29yZS9raHRtbC9odG1sL2h0bWxfdGFibGVpbXBsLmNwcCx2CnJldHJpZXZpbmcgcmV2aXNp b24gMS43MgpkaWZmIC11IC1yMS43MiBodG1sX3RhYmxlaW1wbC5jcHAKLS0tIGh0bWxfdGFibGVp bXBsLmNwcAkyMyBEZWMgMjAwNSAxODo0NDoxNyAtMDAwMAkxLjcyCisrKyBodG1sX3RhYmxlaW1w bC5jcHAJMjcgRGVjIDIwMDUgMjM6MTU6NTUgLTAwMDAKQEAgLTg4MiwxMCArODgyLDIzIEBACiAg ICAgaWYgKCF0YWJsZSB8fCAhdGFibGUtPmhhc1RhZ05hbWUodGFibGVUYWcpKQogCXJldHVybiAt MTsKIAorICAgIEhUTUxUYWJsZVNlY3Rpb25FbGVtZW50SW1wbCAqaGVhZCA9IHN0YXRpY19jYXN0 PEhUTUxUYWJsZUVsZW1lbnRJbXBsKj4odGFibGUpLT50SGVhZCgpOwogICAgIEhUTUxUYWJsZVNl Y3Rpb25FbGVtZW50SW1wbCAqZm9vdCA9IHN0YXRpY19jYXN0PEhUTUxUYWJsZUVsZW1lbnRJbXBs ICo+KHRhYmxlKS0+dEZvb3QoKTsKKworICAgIC8vIG1ha2UgPGhlYWQ+IGNvbWUgYmVmb3JlIDxi b2R5PiA6IGh0dHA6Ly9idWd6aWxsYS5vcGVuZGFyd2luLm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDU4 NgorICAgIGlmIChoZWFkKSB7CisgICAgICAgY29uc3QgTm9kZUltcGwgKnJvdyA9IGhlYWQtPmZp cnN0Q2hpbGQoKTsKKyAgICAgICB3aGlsZSAocm93KSB7CisgICAgICAgICAgIGlmIChyb3cgPT0g dGhpcykKKyAgICAgICAgICAgICAgIHJldHVybiBySW5kZXg7CisgICAgICAgICAgIHJJbmRleCsr OworICAgICAgICAgICByb3cgPSByb3ctPm5leHRTaWJsaW5nKCk7CisgICAgICAgfQorICAgIH0K KyAgICAKICAgICBOb2RlSW1wbCAqbm9kZSA9IHRhYmxlLT5maXJzdENoaWxkKCk7CiAgICAgd2hp bGUgKG5vZGUpIHsKLSAgICAgICAgaWYgKG5vZGUgIT0gZm9vdCAmJiAobm9kZS0+aGFzVGFnTmFt ZSh0aGVhZFRhZykgfHwgbm9kZS0+aGFzVGFnTmFtZSh0Zm9vdFRhZykgfHwgbm9kZS0+aGFzVGFn TmFtZSh0Ym9keVRhZykpKSB7CisgICAgICAgIGlmIChub2RlICE9IGZvb3QgJiYgbm9kZSAhPSBo ZWFkICYmIChub2RlLT5oYXNUYWdOYW1lKHRoZWFkVGFnKSB8fCBub2RlLT5oYXNUYWdOYW1lKHRm b290VGFnKSB8fCBub2RlLT5oYXNUYWdOYW1lKHRib2R5VGFnKSkpIHsKIAkgICAgSFRNTFRhYmxl U2VjdGlvbkVsZW1lbnRJbXBsKiBzZWN0aW9uID0gc3RhdGljX2Nhc3Q8SFRNTFRhYmxlU2VjdGlv bkVsZW1lbnRJbXBsICo+KG5vZGUpOwogCSAgICBjb25zdCBOb2RlSW1wbCAqcm93ID0gc2VjdGlv bi0+Zmlyc3RDaGlsZCgpOwogCSAgICB3aGlsZSAoIHJvdyApIHsK