Bug 5264

Summary:

document.createElementNS() should not allow to insert a second <html> element

Product:

WebKit

Reporter:

Richard Vermillion <rvermillion>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez

Priority:

Keywords:

HasReduction

Version:

420+

Hardware:

Mac

OS:

OS X 10.4

Attachments:

Description	Flags
new test case	none
Proposed patch	darin: review-
Different approach	darin: review-
More complete patch	darin: review-
Even better patch	darin: review+

Richard Vermillion

Reported 2005-10-04 14:37:30 PDT

According to the DOM Level 3 specification, the createDocument method of the DOMImplementation interface should create the root element (i.e. the documentElement) of the Document unless the first two arguments are both null. See: http://www.w3.org/TR/DOM-Level-3-Core/core.html#Level-2-Core-DOM-createDocument In Safari, the new Document does not have a root element and it must be created. The following script produces the problem: var d = document.implementation.createDocument("http://www.w3.org/1999/xhtml", "html", null); alert("In Firefox, this is not null: " + d.documentElement); try { d.appendChild(d.createElementNS("http://www.w3.org/1999/xhtml", "html")); } catch (e) { alert("In Firefox, we correctly get a node hierarchy error: " + e); } alert("Now we have: " + d.documentElement);

Attachments
new test case (397 bytes, text/html) 2006-02-07 11:42 PST, Alexey Proskuryakov	no flags	Details
Proposed patch (1.23 KB, patch) 2006-05-14 14:04 PDT, Rob Buis	darin: review-	Details Formatted Diff Diff
Different approach (4.12 KB, patch) 2006-05-15 13:37 PDT, Rob Buis	darin: review-	Details Formatted Diff Diff
More complete patch (8.24 KB, patch) 2006-05-16 02:34 PDT, Rob Buis	darin: review-	Details Formatted Diff Diff
Even better patch (9.08 KB, patch) 2006-05-16 11:47 PDT, Rob Buis	darin: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2006-02-07 11:41:45 PST

The original issue reported here has been fixed as bug 5728. However, the test case still fails, because the code creates two <html> elements. I'm leaving the bug open to track this additional issue.

Alexey Proskuryakov

Comment 2 2006-02-07 11:42:19 PST

Created attachment 6332 [details] new test case

Rob Buis

Comment 3 2006-05-14 14:04:32 PDT

Created attachment 8310 [details] Proposed patch First stab at fixing this. Cheers, Rob.

Darin Adler

Comment 4 2006-05-14 17:28:00 PDT

Comment on attachment 8310 [details] Proposed patch Despite its name, I don't think it's good to reuse the parsing function, childAllowed, for uses other than in the parser. This change alone would leave the code in a very confused state about when childAllowed is called, when checkAddChild is called, and when childTypeAllowed is called. For clarity we should probably rename at least some of the three functions. On the other hand, the code in Document::childAllowed is clearly good to run even when not parsing. If we decide we want that logic all the time, then we should change that logic to be type-based and move it into Document::childTypeAllowed. Since all the code does is check if the new child is an element, and then if its type is DOCUMENT_TYPE_NODE, I think the code could be adapted that way. Also need a layout test, of course, and some documentation about what WinIE and Firefox do in these cases.

Rob Buis

Comment 5 2006-05-15 13:37:06 PDT

Created attachment 8328 [details] Different approach Hi Darin, I think it will be hard to merge some of the functions. For instance HTMLElement::childAllowed really seems to be useful only for parsing, not for dom usage. In the new patch I move the two checks from Document::childallowed to Document::childTypeAllowed. That means for calling childAllowed on Document it behaves as before, for Document::childTypeAllowed it adds the two checks. The changed test results are official dom cases that now pass, except documentnormalizedocument10.xhtml, which also "fails" in firefox. I think that test is broken though, the others probably need updated test results. Firefox behaves correctly, it throws the exception. Win IE does not run the testcase, it says: Line 3 Char 3 Error: Object doesn't support this property or method Cheers, Rob.

Darin Adler

Comment 6 2006-05-15 14:39:29 PDT

Comment on attachment 8328 [details] Different approach Looks great. (In reply to comment #5) > I think it will be hard to merge some of the functions. For instance > HTMLElement::childAllowed > really seems to be useful only for parsing, not for dom usage. Right. Sorry, I didn't mean that we should merge them all. I was requesting that you move the checks in Document, not all of the other parser-specific checks. But I think that childAllowed also (eventually) needs a name that makes it clear it's for parsing only. > The changed test results are official dom cases that now pass, except > documentnormalizedocument10.xhtml, which also "fails" in firefox. I think that > test is broken though, > the others probably need updated test results. > Firefox behaves correctly, it throws the exception. Sounds good. The patch should include the updated results for the DOM test cases. We should add a new test for document type, since we're changing the behavior of that too. r=me on the code change, but please post a patch that includes the test result changes and a change log for LayoutTests.

Rob Buis

Comment 7 2006-05-16 02:34:57 PDT

Created attachment 8349 [details] More complete patch Hi Darin, This patch does include the changed results, and two instead of just one testcase. In fact the old testcase was not suited, having alert() in them. Cheers, Rob.

Darin Adler

Comment 8 2006-05-16 10:12:05 PDT

Comment on attachment 8349 [details] More complete patch Getting better! ChangeLog for LayoutTests needs to mention changes to test results, not just newly added tests. Patch should include expected results for new tests as well as the tests themselves. And as long as I'm looking at this code for the third time, I suggest tightening it a tiny bit: + case ELEMENT_NODE: { + // Documents may contain a maximum of one Element child + Node *c; + for (c = firstChild(); c; c = c->nextSibling()) { + if (c->isElementNode()) + return false; + } + return true; + } + case DOCUMENT_TYPE_NODE: { + // Documents may contain a maximum of one DocumentType child + Node *c; + for (c = firstChild(); c; c = c->nextSibling()) { + if (c->nodeType() == DOCUMENT_TYPE_NODE) + return false; + } + return true; + } Like this: case DOCUMENT_TYPE_NODE: case ELEMENT_NODE: // Documents may contain a maximum of one child of each of these types. // (One Element child and one DocumentType child.) for (Node* c = firstChild(); c; c = c->nextSibling()) if (c->nodeType() == type) return false; return true;

Rob Buis

Comment 9 2006-05-16 11:47:07 PDT

Created attachment 8353 [details] Even better patch I love the tweak to combine the two code sections :) I improved the test cases and have tried to fix all the remaining issues. Cheers, Rob.

Darin Adler

Comment 10 2006-05-16 13:03:20 PDT

Comment on attachment 8353 [details] Even better patch r=me

Darin Adler

Comment 11 2006-05-16 20:25:13 PDT

Looking at createDocumentType2, I see that it adds the same DocumentType element twice. That's not the same thing as adding two DocumentType elements! In fact, it tests a behavior which I'm not sure is correct. If you're appending a child that's already in the document I don't think it should fail, because appendChild removes the child from its old location first.

Darin Adler

Comment 12 2006-05-16 23:30:01 PDT

Committed revision 14430.

Lucas Forschler

Comment 13 2019-02-06 09:04:07 PST

Mass moving XML DOM bugs to the "DOM" Component.

Note You need to log in before you can comment on or make changes to this bug.