Bug 3298

Summary: insertRow generates DOM Exception if TABLE does not possess a TBODY
Product: WebKit Reporter: Omar Kilani <omar>
Component: TablesAssignee: Anders Carlsson <andersca>
Severity: Normal CC: justin.garcia, stuartmorgan
Priority: P2    
Version: 412   
Hardware: PC   
OS: OS X 10.4   
URL: http://aurore.net/safari/insertrow.html
Description Flags
Update firstBody accordingly
darin: review-
Address comments
mjs: review+
Patch for new testcase
mjs: review+
Improved Testcase
Testcase for typical cases none

Description Omar Kilani 2005-06-07 06:37:19 PDT
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
Comment 1 Anders Carlsson 2005-06-27 11:06:24 PDT
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.
Comment 2 Anders Carlsson 2005-06-27 11:09:03 PDT
*** Bug 3476 has been marked as a duplicate of this bug. ***
Comment 3 Darin Adler 2005-06-27 17:17:24 PDT
Comment on attachment 2672 [details]
Update firstBody accordingly

I think hasChildElements belongs in either NodeImpl or ElementImpl rather than

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
Comment 4 Anders Carlsson 2005-06-28 15:45:07 PDT
Created attachment 2693 [details]
Address comments
Comment 5 Darin Adler 2005-06-29 08:13:34 PDT
Comment on attachment 2693 [details]
Address comments

Comment 6 Justin Garcia 2005-06-29 19:42:56 PDT
Created attachment 2709 [details]

insertRow() also fails on <table><form></form></table>	
I would propose removing the !hasChildElements() check altogether.
Comment 7 Justin Garcia 2005-06-29 19:55:20 PDT
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 8 Darin Adler 2005-06-29 21:35:50 PDT
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

    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.
Comment 9 Darin Adler 2005-06-29 21:49:47 PDT
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.
Comment 10 Darin Adler 2005-06-29 21:51:45 PDT
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!
Comment 11 Justin Garcia 2005-06-30 09:44:53 PDT
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
Comment 12 Justin Garcia 2005-06-30 09:50:22 PDT
Created attachment 2717 [details]
Testcase for typical cases

Tests that the row inserted is actually inside the table on a few typical
Comment 13 Maciej Stachowiak 2005-07-13 23:55:43 PDT
Comment on attachment 2693 [details]
Address comments

Comment 14 Justin Garcia 2005-07-14 13:38:38 PDT
Landing the updated patch