shadowRoot.cloneNode() returns a DocumentFragment, should throw DataCloneError

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: sgiles, Assigned: sgiles)

Tracking

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

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

4 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.124 Safari/537.36

Steps to reproduce:


```
var element = document.createElement("a");

// Should throw, not return.
var docFragment = element.createShadowRoot().cloneNode();
```


Actual results:

A `DocumentFragment` is returned.


Expected results:

When calling `cloneNode()` on a `ShadowRoot` a `DataCloneError` should've been thrown. [1]


[1] http://www.w3.org/TR/shadow-dom/#methods
Assignee

Comment 2

4 years ago
rev1 - failing test case
Assignee

Updated

4 years ago
Component: Untriaged → DOM
Product: Firefox → Core
Assignee

Updated

4 years ago
Assignee

Comment 4

4 years ago
Should I merge the patches?
Comment on attachment 8625390 [details] [diff] [review]
Throw a DataCloneError when attempting to invoke 'cloneNode' on a `ShadowRoot`

William should be good to review this :)
Attachment #8625390 - Flags: review?(bzbarsky) → review?(wchen)
Assignee

Comment 6

4 years ago
Attachment #8625348 - Attachment is obsolete: true
Attachment #8625390 - Attachment is obsolete: true
Attachment #8625390 - Flags: review?(wchen)
Attachment #8625514 - Flags: review?(wchen)
Comment on attachment 8625514 [details] [diff] [review]
Throw a DataCloneError when attempting to invoke 'cloneNode' on a `ShadowRoot`, and test

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

This change will also affect Document.importNode, causing it to throw. I've filed a bug against the spec here: https://github.com/w3c/webcomponents/issues/125. For now, it's probably OK to let importNode propagate the DataCloneError, but we may want to change this depending how the spec bug gets resolved. It looks like Chrome is explicitly checking for a ShadowRoot when adopting and importing.

::: dom/base/ShadowRoot.cpp
@@ +711,5 @@
>      RemoveDistributedNode(aChild);
>    }
>  }
>  
> +nsresult ShadowRoot::Clone(mozilla::dom::NodeInfo *aNodeInfo, nsINode **aResult) const

nit: nsresult should be on its own line.

@@ +713,5 @@
>  }
>  
> +nsresult ShadowRoot::Clone(mozilla::dom::NodeInfo *aNodeInfo, nsINode **aResult) const
> +{
> +  return NS_ERROR_DOM_DATA_CLONE_ERR;

lets set *aResult = nullptr; before returning.
Attachment #8625514 - Flags: review?(wchen) → review+
Assignee

Comment 8

4 years ago
I've updated the patch with your suggested edits :).
Attachment #8625514 - Attachment is obsolete: true
Attachment #8626340 - Flags: review?(wchen)
Assignee

Comment 9

4 years ago
(In reply to sam.e.giles@gmail.com from comment #8)
> Created attachment 8626340 [details] [diff] [review]
> Throw a DataCloneError when attempting to invoke 'cloneNode' on a
> `ShadowRoot`, and test  - with suggested edits
> 
> I've updated the patch with your suggested edits :).

I've also moved the test case into `dom/tests/mochitest/webcomponents` as I recently discovered this directory and it seems like this test should live there.
Comment on attachment 8626340 [details] [diff] [review]
Throw a DataCloneError when attempting to invoke 'cloneNode' on a `ShadowRoot`, and test  - with suggested edits

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

Thanks for working on this Sam. You generally don't need to re-request review on r+ed patches when the review comments are very minor issues, as long as it's fixed and doesn't result in any significant changes.

::: dom/base/ShadowRoot.cpp
@@ +714,5 @@
>  
> +nsresult 
> +ShadowRoot::Clone(mozilla::dom::NodeInfo *aNodeInfo, nsINode **aResult) const
> +{
> +  *aResult = nullptr; 

Some trailing whitespace sneaked in here, should be removed.
Attachment #8626340 - Flags: review?(wchen) → review+
Assignee

Comment 11

4 years ago
Removes rogue white space. Oops.
Attachment #8626340 - Attachment is obsolete: true
Attachment #8626414 - Flags: review+
William, are you planning to drive this into the tree?
Assignee: nobody → sam.e.giles
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(wchen)
https://hg.mozilla.org/mozilla-central/rev/17f5cab3e712
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Duplicate of this bug: 1172680
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.