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

RESOLVED FIXED in Firefox 42

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: sgiles, Assigned: sgiles)

Tracking

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

Trunk
mozilla42
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 4 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.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 1

2 years ago
Obsoletes: https://bugzilla.mozilla.org/show_bug.cgi?id=1172680
(Assignee)

Comment 2

2 years ago
Created attachment 8625348 [details] [diff] [review]
bug1176757_shadowRoot_cloneNode_test.diff

rev1 - failing test case
(Assignee)

Updated

2 years ago
Component: Untriaged → DOM
Product: Firefox → Core
(Assignee)

Updated

2 years ago
Blocks: 811542
Keywords: dev-doc-needed
(Assignee)

Comment 3

2 years ago
Created attachment 8625390 [details] [diff] [review]
Throw a DataCloneError when attempting to invoke 'cloneNode' on a `ShadowRoot`
Attachment #8625390 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

2 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

2 years ago
Created attachment 8625514 [details] [diff] [review]
Throw a DataCloneError when attempting to invoke 'cloneNode' on a `ShadowRoot`, and test
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

2 years ago
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 :).
Attachment #8625514 - Attachment is obsolete: true
Attachment #8626340 - Flags: review?(wchen)
(Assignee)

Comment 9

2 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

2 years ago
Created attachment 8626414 [details] [diff] [review]
Throw a DataCloneError when attempting to invoke 'cloneNode' on a `ShadowRoot`, and test - with suggested edits

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)
Blocks: 1177914
https://hg.mozilla.org/integration/mozilla-inbound/rev/17f5cab3e712
Flags: needinfo?(wchen)
https://hg.mozilla.org/mozilla-central/rev/17f5cab3e712
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Duplicate of this bug: 1172680
You need to log in before you can comment on or make changes to this bug.