If a TABLE does not include a TBODY as a child node, insertRow will generate a DOM Exception. See URL for a test case. This works in all other browsers (IE/Mozilla/Opera.)
Created attachment 2672 [details] Update firstBody accordingly The problem was actually the call to tx.innerHTML = ''; This would remove all the children of the table, but not update the firstBody variable which is supposed to point to the first tbody node. I also found that insertRow didn't work at all when the table element had children that weren't elements. This patch fixes both issues.
*** Bug 3476 has been marked as a duplicate of this bug. ***
Comment on attachment 2672 [details] Update firstBody accordingly I think hasChildElements belongs in either NodeImpl or ElementImpl rather than here. No need to check hasChildNodes() in hasChildElement(). firstChild() will be 0, and the function will quickly return false without the extra check. I think a better approach to tracking firstBody would be to hold a ref to firstBody, then check if its parent is still "this" in childrenChanged, and deref it if so. Also deref in the destructor of course. No need to override removeChild.
Created attachment 2693 [details] Address comments
Comment on attachment 2693 [details] Address comments r=me
Created attachment 2709 [details] Testcase insertRow() also fails on <table><form></form></table> I would propose removing the !hasChildElements() check altogether.
Created attachment 2710 [details] Patch for new testcase Removes the check for !hasChildElements() so that a TBODY is added in cases like <table><form></form></table> and <table><script></script></table>. Also regarding the changes for firstBody, we should make similar changes for 'head' and 'foot' so that if the nodes that they point to are removed, they don't become bogus references. But that should be a different bug.
Comment on attachment 2710 [details] Patch for new testcase The patch looks like a step in the right direction, but I have at least these concerns: 1) There's no test case where we call insertRow in the case where there actually is content. To be truly convinced we don't need the hasChildNodes() check I'd like to see test cases with <tr>, <td>, and other actual body elements, and know they did the right thing (whatever that is). 2) The test case doesn't test what insertRow did, it just checks for success or failure of the call. I think if we're verifying that a <tbody> is added, we should somehow check that. 3) All the test cases fail on Firefox, which worries me. Given these, I'm going to mark this review- until we investigate a bit further.
Looks like the failure in Firefox is simply due to the fact that they don't allow you to call insertRow() with no args. MacIE does, though, and succeeds in all 3 cases.
And changing the test case to pass -1 for the row number causes it to succeed in Firefox, so you can consider my issue (3) not an issue. I think my only objection now is that I'm not entirely convinced that the hasChildNodes check before was completely useless, and I'd like to see more test cases to prove that. But perhaps I'm just being too picky!
Created attachment 2716 [details] Improved Testcase -Fixed the Firefox issue -Now tests that the inserted row is actually inside the table -Now tests whether or not the new table has a tbody
Created attachment 2717 [details] Testcase for typical cases Tests that the row inserted is actually inside the table on a few typical tables.
Landing the updated patch