Bug 137755 – cloneNode(true) does not clone nested template elements' contents

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Evan You]

When an element or documentFragment that contains <template> elements is cloned via cloneNode(true), the nested <template> inside the cloned element will lose all the original content (possibly shallow-cloned). See the jsfiddle in the provided URL for an example.

[Alexey Proskuryakov]

See also: bug 137619.

[Ryosuke Niwa]

*** This bug has been marked as a duplicate of bug 137619 ***

[Ryosuke Niwa]

Actually, these bugs aren't duplicates of each other.

[Ryosuke Niwa]

Created attachment 243267 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 243268 [details]
Reverted an irrelevant change

[Darin Adler]

Comment on attachment 243268 [details]
Reverted an irrelevant change

View in context: https://bugs.webkit.org/attachment.cgi?id=243268&action=review

> Source/WebCore/dom/Node.h:205
> +    enum class CloneNodeType {

I don’t like the name “clone node type”, because it’s not clear that it’s the type of cloning operation. I think it should be named CloningOperation.

> Source/WebCore/dom/Node.h:208
> +        CloneSelf,
> +        CloneContent,
> +        CloneChildNodes,

I’m not too fond of these three names, because it’s not at all clear that each one in succession clones more. For example, all three clone self. And CloneChildNodes also clones “content”, whatever that is. I’m not so happy with “content” as the name for shadow nodes either.

Also, it seems that including "Clone" as a prefix in each enum value is not so great, since it’s already in the name of the enum class at every site we use this.

> Source/WebCore/dom/ShadowRoot.cpp:136
> +    return nullptr; // ShadowRoots should never be cloned.

Does this also need an ASSERT_NOT_REACHED some day?

[Ryosuke Niwa]

Thanks for the review!

(In reply to comment #6)
> Comment on attachment 243268 [details]
> Reverted an irrelevant change
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243268&action=review
> 
> > Source/WebCore/dom/Node.h:205
> > +    enum class CloneNodeType {
> 
> I don’t like the name “clone node type”, because it’s not clear that it’s
> the type of cloning operation. I think it should be named CloningOperation.

Done.

> > Source/WebCore/dom/Node.h:208
> > +        CloneSelf,
> > +        CloneContent,
> > +        CloneChildNodes,
> 
> I’m not too fond of these three names, because it’s not at all clear that
> each one in succession clones more. For example, all three clone self. And
> CloneChildNodes also clones “content”, whatever that is. I’m not so happy
> with “content” as the name for shadow nodes either.
> 
> Also, it seems that including "Clone" as a prefix in each enum value is not
> so great, since it’s already in the name of the enum class at every site we
> use this.

Renamed them to OnlySelf, SelfWithTemplateContent, and Everything respectively.

> > Source/WebCore/dom/ShadowRoot.cpp:136
> > +    return nullptr; // ShadowRoots should never be cloned.
> 
> Does this also need an ASSERT_NOT_REACHED some day?

I don't think so.

[Ryosuke Niwa]

Created attachment 243304 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 243304 [details]
Patch for landing

Rejecting attachment 243304 [details] from commit-queue.

Number of test failures exceeded the failure limit.
Full output: http://webkit-queues.appspot.com/results/5781854196072448

[WebKit Commit Bot]

Created attachment 243306 [details]
Archive of layout-test-results from webkit-cq-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Ryosuke Niwa]

Comment on attachment 243304 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=243304&action=review

> Source/WebCore/dom/Node.h:211
> +    RefPtr<Node> cloneNode(bool deep) { return cloneNodeInternal(deep ? CloningOperation::OnlySelf : CloningOperation::Everything); }

Oops, this is wrong.

[Ryosuke Niwa]

Committed r177314: <http://trac.webkit.org/changeset/177314>

[Darin Adler]

Comment on attachment 243268 [details]
Reverted an irrelevant change

View in context: https://bugs.webkit.org/attachment.cgi?id=243268&action=review

>>> Source/WebCore/dom/Node.h:208
>>> +        CloneChildNodes,
>> 
>> I’m not too fond of these three names, because it’s not at all clear that each one in succession clones more. For example, all three clone self. And CloneChildNodes also clones “content”, whatever that is. I’m not so happy with “content” as the name for shadow nodes either.
>> 
>> Also, it seems that including "Clone" as a prefix in each enum value is not so great, since it’s already in the name of the enum class at every site we use this.
> 
> Renamed them to OnlySelf, SelfWithTemplateContent, and Everything respectively.

I like those names.