Bug 137755 - cloneNode(true) does not clone nested template elements' contents
Summary: cloneNode(true) does not clone nested template elements' contents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.9
: P2 Major
Assignee: Ryosuke Niwa
URL: http://jsfiddle.net/3s73zgjn/2/
Keywords:
Depends on:
Blocks: 137619
  Show dependency treegraph
 
Reported: 2014-10-15 15:04 PDT by Evan You
Modified: 2014-12-16 22:55 PST (History)
8 users (show)

See Also:


Attachments
Fixes the bug (24.34 KB, patch)
2014-12-14 03:09 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Reverted an irrelevant change (22.24 KB, patch)
2014-12-14 03:14 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (22.38 KB, patch)
2014-12-15 12:41 PST, Ryosuke Niwa
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-02 for mac-mountainlion (207.60 KB, application/zip)
2014-12-15 13:43 PST, WebKit Commit Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Evan You 2014-10-15 15:04:53 PDT
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.
Comment 1 Alexey Proskuryakov 2014-10-16 00:16:38 PDT
See also: bug 137619.
Comment 2 Ryosuke Niwa 2014-12-14 00:00:29 PST

*** This bug has been marked as a duplicate of bug 137619 ***
Comment 3 Ryosuke Niwa 2014-12-14 00:05:11 PST
Actually, these bugs aren't duplicates of each other.
Comment 4 Ryosuke Niwa 2014-12-14 03:09:39 PST
Created attachment 243267 [details]
Fixes the bug
Comment 5 Ryosuke Niwa 2014-12-14 03:14:59 PST
Created attachment 243268 [details]
Reverted an irrelevant change
Comment 6 Darin Adler 2014-12-15 09:22:15 PST
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?
Comment 7 Ryosuke Niwa 2014-12-15 12:33:12 PST
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.
Comment 8 Ryosuke Niwa 2014-12-15 12:41:36 PST
Created attachment 243304 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2014-12-15 13:43:47 PST
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
Comment 10 WebKit Commit Bot 2014-12-15 13:43:50 PST
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
Comment 11 Ryosuke Niwa 2014-12-15 13:53:56 PST
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.
Comment 12 Ryosuke Niwa 2014-12-15 14:18:53 PST
Committed r177314: <http://trac.webkit.org/changeset/177314>
Comment 13 Darin Adler 2014-12-16 22:55:20 PST
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.