1177914 - document.importNode on a ShadowRoot should throw a NotSupportedError exception

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[sgiles]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.130 Safari/537.36

Steps to reproduce:

```
var shadowRoot = document.createElement('a').createShadowRoot();

// Should throw NotSupportedError, however, instead it returns a DocumentFragment (the ShadowRoot)
var x = document.importNode(shadowRoot);
```


Actual results:

Calling importNode on a document returns a DocumentFragment.


Expected results:

A NotSupportedError should've been thrown.

https://dom.spec.whatwg.org/#dom-document-importnode

Comment 1

[Boris Zbarsky [:bzbarsky]]

Doesn't the patch in bug 1176757 fix this?

Comment 2

[sgiles]

The patch there will cause a DataCloneError, the spec here says it should be a NotSupportedError when using `document.importNode`.  Although, whether the spec should be a DataCloneError or a NotSupportedError is questionable.  I'm in favour of 'NotSupportedError'.

Comment 3

[sgiles]

Attachment: Throw a NotSupportedError when running document.importNode on a ShadowRoot and test

Comment 4

[sgiles]

Attachment: bug-1177914.patch

Too many coding styles.. tabs -> spaces.

Comment 5

[William Chen [:wchen]]

Comment on attachment 8626892 [details] [diff] [review]
bug-1177914.patch

Review of attachment 8626892 [details] [diff] [review]:
-----------------------------------------------------------------

While this does throw a NotSupportedError, it also outputs a warning that was probably meant to catch unexpected node types. For the shadow root case, it's not unexpected because it's explicitly mentioned by the spec. We should keep the default case of the switch statement and just return NS_ERROR_DOM_NOT_SUPPORTED_ERR from the check for the shadow root. As a side note, it looks like importing a document nodes warns as well, nsIDOMNode::DOCUMENT_NODE should probably have a case in the switch statement that just returns the error without the warning (similar to what we do in nsIDocument::AdoptNode), we could probably do that here too and just add "shadow root or document" to the patch description.

Comment 6

[sgiles]

Attachment: Bug 1177914 - throw a NotSupportedError when running document.importNode on a ShadowRoot or Document

Comment 7

[sgiles]

Comment on attachment 8632866 [details] [diff] [review]
Bug 1177914 - throw a NotSupportedError when running document.importNode on a ShadowRoot or Document

I'm unsure about the order of the 'case' statements. I've kept the statements that require branching at the top.

Sorry it took so long :), I've been pretty busy.

Comment 8

[William Chen [:wchen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/246aabca6f2c

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/246aabca6f2c