ProcessingInstruction should not be a ContainerNode

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

ProcessingInstruction should not be a ContainerNode

[Adam Klein]

Created attachment 120397 [details]
Patch

[Darin Adler]

Comment on attachment 120397 [details]
Patch

This changes behavior. When you call appendChild on a ProcessingInstruction and pass null for the child, you will get HIERARCHY_REQUEST_ERR now, and before you would get NOT_FOUND_ERR. While unimportant, it would be nice to get this edge case right. Could we add tests covering that behavior?

[Adam Klein]

(In reply to comment #2)
> (From update of attachment 120397 [details])
> This changes behavior. When you call appendChild on a ProcessingInstruction and pass null for the child, you will get HIERARCHY_REQUEST_ERR now, and before you would get NOT_FOUND_ERR. While unimportant, it would be nice to get this edge case right. Could we add tests covering that behavior?

An interesting point. Perhaps Node::appendChild ought to throw NOT_FOUND_ERR if its argument is null, or HIERARCHY_REQUEST_ERR if non-null.  Agreed that it'd be nice to have test coverage in either case for all the various node types.

[WebKit Review Bot]

Comment on attachment 120397 [details]
Patch

Attachment 120397 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10939086

New failing tests:
media/W3C/video/networkState/networkState_during_progress.html
http/tests/inspector/resource-tree/resource-tree-document-url.html

[Ojan Vafai]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 120397 [details] [details])
> > This changes behavior. When you call appendChild on a ProcessingInstruction and pass null for the child, you will get HIERARCHY_REQUEST_ERR now, and before you would get NOT_FOUND_ERR. While unimportant, it would be nice to get this edge case right. Could we add tests covering that behavior?
> 
> An interesting point. Perhaps Node::appendChild ought to throw NOT_FOUND_ERR if its argument is null, or HIERARCHY_REQUEST_ERR if non-null.  Agreed that it'd be nice to have test coverage in either case for all the various node types.

I don't think it's a big deal to change the error type if we're not violating the spec. I highly doubt there is code that depends on throwing a NOT_FOUND_ERR instead of a HIERARCHY_REQUEST_ERR.

We should do what we can to be inline with the new spec or get the spec changed to make the code more simple: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-node-pre-insert.

[Ryosuke Niwa]

According to http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-node-pre-insert:

Step 3: If parent is not a Document, DocumentFragment, or Element node, throw a "HierarchyRequestError" and terminate these steps.
...
Step 4.1: If node is not a DocumentFragment, DocumentType, Element, ProcessingInstruction, or Comment node, throw a "HierarchyRequestError" and terminate these steps.
...
Step 5: Otherwise if parent is a DocumentFragment or Element node and node is not a DocumentFragment, Element, Text, ProcessingInstruction, or Comment node, throw a "HierarchyRequestError" and terminate these steps.

So we should be throwing HierarchyRequestError in both of those cases.

[Ojan Vafai]

Yup, HIERARCHY_REQUEST_ERR seems right. I was a little confused by step 1:
"If child is not null and its parent is not parent, throw a "NotFoundError" exception and terminate these steps."

I now understand that to be talking about parent.insertBefore(newChild, oldChild) and the case where oldChild is not actually a child of parent. For that we should throw NOT_FOUND_ERR, otherwise, ProcessingInstruction should always throw HIERARCHY_REQUEST_ERR for all the DOM insertion methods. 

That said, it might make sense to change the spec here to reorder items 1-3 to the following:
1. If parent is not a Document, DocumentFragment, or Element node, throw a "HierarchyRequestError" and terminate these steps.
2. If node is parent or an ancestor of parent, throw a "HierarchyRequestError" and terminate these steps.
3. If child is not null and its parent is not parent, throw a "NotFoundError" exception and terminate these steps.

It's not a big deal, but it would somewhat simplify implementation.

In either case, I'm fine with this patch going in as long as Adam adds a test verifying the behavior (even in a followup patch if he prefers).

[Ojan Vafai]

http://lists.w3.org/Archives/Public/www-dom/2011OctDec/0271.html

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Spec change: http://dvcs.w3.org/hg/domcore/rev/4bae4cfa591f

Tests: http://w3c-test.org/webapps/DOMCore/tests/submissions/Ms2ger/Node-insertBefore.html

[Adam Klein]

Created attachment 124842 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 124842 [details]
Patch for landing

Clearing flags on attachment: 124842

Committed r106418: <http://trac.webkit.org/changeset/106418>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.