Closed Bug 1177914 Opened 8 years ago Closed 8 years ago

document.importNode on a ShadowRoot should throw a NotSupportedError exception


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

Not set



Tracking Status
firefox42 --- fixed


(Reporter: sgiles, Assigned: sgiles)


(Blocks 1 open bug)


(Keywords: dev-doc-needed)


(1 file, 2 obsolete files)

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.
Component: Untriaged → DOM
Product: Firefox → Core
Doesn't the patch in bug 1176757 fix this?
Depends on: 1176757
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'.
Attached patch bug-1177914.patch (obsolete) — Splinter Review
Too many coding styles.. tabs -> spaces.
Attachment #8626891 - Attachment is obsolete: true
Attachment #8626891 - Flags: review?(wchen)
Attachment #8626892 - Flags: review?(wchen)
Comment on attachment 8626892 [details] [diff] [review]

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.
Attachment #8626892 - Flags: review?(wchen) → review-
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.
Attachment #8632866 - Flags: review?(wchen)
Attachment #8632866 - Flags: review?(wchen) → review+
Assignee: nobody → sam.e.giles
Flags: in-testsuite+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.