document.importNode on a ShadowRoot should throw a NotSupportedError exception

RESOLVED FIXED in Firefox 42

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sgiles, Assigned: sgiles)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla42
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
Blocks: 811542
Component: Untriaged → DOM
Product: Firefox → Core
Doesn't the patch in bug 1176757 fix this?
Depends on: 1176757
(Assignee)

Comment 2

2 years ago
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'.
(Assignee)

Comment 3

2 years ago
Created attachment 8626891 [details] [diff] [review]
Throw a NotSupportedError when running document.importNode on a ShadowRoot and test
Attachment #8626891 - Flags: review?(wchen)
(Assignee)

Comment 4

2 years ago
Created attachment 8626892 [details] [diff] [review]
bug-1177914.patch

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]
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.
Attachment #8626892 - Flags: review?(wchen) → review-
(Assignee)

Comment 6

2 years ago
Created attachment 8632866 [details] [diff] [review]
Bug 1177914 - throw a NotSupportedError when running document.importNode on a ShadowRoot or Document
Attachment #8626892 - Attachment is obsolete: true
(Assignee)

Comment 7

2 years ago
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)

Updated

2 years ago
Attachment #8632866 - Flags: review?(wchen) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/246aabca6f2c
Assignee: nobody → sam.e.giles
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/246aabca6f2c
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.