sgiles Assignee
	Description • 9 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

	sgiles Assignee
	Comment 1 • 9 years ago

Obsoletes: https://bugzilla.mozilla.org/show_bug.cgi?id=1172680

	sgiles Assignee
	Comment 2 • 9 years ago

rev1 - failing test case

	sgiles Assignee
	Comment 4 • 9 years ago

Should I merge the patches?

	Wilson Page [:wilsonpage]
	Comment 5 • 9 years ago

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 :)

	William Chen [:wchen]
	Comment 7 • 9 years ago

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.

	sgiles Assignee
	Comment 8 • 9 years ago

I've updated the patch with your suggested edits :).

	sgiles Assignee
	Comment 9 • 9 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.

	William Chen [:wchen]
	Comment 10 • 9 years ago

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.

	sgiles Assignee
	Comment 11 • 9 years ago

Removes rogue white space. Oops.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 9 years ago

William, are you planning to drive this into the tree?

	William Chen [:wchen]
	Comment 13 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/17f5cab3e712

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 14 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/17f5cab3e712