insertRow generates DOM Exception if TABLE does not possess a TBODY

• Component: Tables
• Status: RESOLVED FIXED

[Omar Kilani]

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.)

[Anders Carlsson]

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.

[Anders Carlsson]

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

[Darin Adler]

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.

[Anders Carlsson]

Created attachment 2693 [details]
Address comments

[Darin Adler]

Comment on attachment 2693 [details]
Address comments

r=me

[Justin Garcia]

Created attachment 2709 [details]
Testcase

insertRow() also fails on <table><form></form></table>	
I would propose removing the !hasChildElements() check altogether.

[Justin Garcia]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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!

[Justin Garcia]

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

[Justin Garcia]

Created attachment 2717 [details]
Testcase for typical cases

Tests that the row inserted is actually inside the table on a few typical
tables.

[Maciej Stachowiak]

Comment on attachment 2693 [details]
Address comments

r=me

[Justin Garcia]

Landing the updated patch