From e03faa12a86fced56d076740e95fc7557db5eac4 Mon Sep 17 00:00:00 2001
From: Mason Freed <masonf@chromium.org>
Date: Wed, 26 Jul 2023 17:29:49 -0700
Subject: [PATCH] Fix cloning of templates with DOM Parts, and improve testing

This CL improves the testing of template cloning with Parts, testing
these four cases:

  1. Main document parsing
  2. Template (content fragment) parsing
  3. Template/fragment cloning
  4. Declarative Shadow DOM parsing and cloning

This CL fixes the behavior for #3 above, but leaves #4 broken. The
following changes in behavior are made:

1. Part::MoveToRoot() can be used to change the root(), including
   to set it to nullptr. This happens when a Node tree is removed
   from the DOM, and it contains Parts that refer to the old root.
2. IsDocumentPartRoot() is now virtual, because during a tree move,
   the root() for a Part can be made nullptr even when it's a
   ChildNodePart.
3. Part::disconnected_ is added to keep track of whether the
   Part has been disconnected, since root() can now be nullptr.
4. (This is a bug fix) When using ChildNodePart::setNextSibling(),
   the new sibling node wasn't having its Part registered with
   NodeRareData, which caused a CHECK failure when trying to
   subsequently clone that Part. This is caught in the new test
   which clones declaratively-built templates containing Parts.

Bug: 1453291
Change-Id: Ic1c1475431cf6bd658f191db78003204412ef78f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4713668
Reviewed-by: David Baron <dbaron@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1175782}
---
 .../basic-dom-part-objects.tentative.html     | 162 +++++++++++++-----
 1 file changed, 115 insertions(+), 47 deletions(-)

diff --git a/dom/parts/basic-dom-part-objects.tentative.html b/dom/parts/basic-dom-part-objects.tentative.html
index ee35298e5dabbb..3dda4e492c2198 100644
--- a/dom/parts/basic-dom-part-objects.tentative.html
+++ b/dom/parts/basic-dom-part-objects.tentative.html
@@ -30,6 +30,8 @@
         <span id=sub>B-sub2</span>
       </span><span id=c>C</span></div>
   </div>
+  <span id=direct_child_1></span>
+  <span id=direct_child_2></span>
 </template>
 
 <script>{
@@ -40,13 +42,15 @@
 }
 [false,true].forEach(useTemplate => {
   const doc = useTemplate ? template.content : document;
-  let target,wrapper;
+  let target,wrapper,directChildren;
   if (useTemplate) {
     target = doc.querySelector('#target1');
+    directChildren = [doc.querySelector('#direct_child_1'),doc.querySelector('#direct_child_2')];
   } else {
     wrapper = document.body.appendChild(document.createElement('div'));
     wrapper.appendChild(template.content.cloneNode(true));
     target = wrapper.querySelector('#target1');
+    directChildren = [doc.documentElement,doc.documentElement];
   }
   const a = target.querySelector('#a');
   const b = target.querySelector('#b');
@@ -251,6 +255,25 @@
     assert_array_equals(target.children,[a,b,c]);
   }, `ChildNodePart children manipulation (${description})`);
 
+  test((t) => {
+    const root = doc.getPartRoot();
+    // Make sure no crashes occur for invalid parts.
+    const cornerCasePartsInvalid = [
+      addCleanup(t,new ChildNodePart(root,target, target.children[2],{metadata: ['different parents']})),
+      addCleanup(t,new ChildNodePart(root,target.children[0], target,{metadata: ['different parents']})),
+      addCleanup(t,new ChildNodePart(root,target, doc,{metadata: ['different parents']})),
+    ];
+    const cornerCasePartsValid = [];
+    const maybeValid = addCleanup(t,new ChildNodePart(root,directChildren[0], directChildren[1],{metadata: ['direct parent of the root container']}));
+    if (directChildren[0] === directChildren[1]) {
+      cornerCasePartsInvalid.push(maybeValid);
+    } else {
+      cornerCasePartsValid.push(maybeValid);
+    }
+    assert_array_equals(root.getParts(),cornerCasePartsValid);
+    assert_equals(root.clone().getParts().length,cornerCasePartsValid.length);
+  }, `Corner case ChildNodePart construction and cloning (${description})`);
+
   wrapper?.remove(); // Cleanup
 });
 }</script>
@@ -258,11 +281,12 @@
 <div>
   <!-- Note - the test will remove this chunk of DOM once the test completes -->
   <div id=target2>
-    Declarative syntax - The template below (with id=declarative) should have
-    IDENTICAL STRUCTURE. There are three cases to test:
+    Declarative syntax - The *two* templates below should have IDENTICAL STRUCTURE
+    to this one. There are four cases to test:
       1. Main document parsing (this chunk)
-      2. Template parsing (the template below)
-      3. Template/fragment cloning (the clone of the template below)
+      2. Template parsing (the template below with id=declarative)
+      3. Template/fragment cloning (a clone of the template with id=declarative)
+      4. Declarative Shadow DOM parsing (template with id=declarative_shadow_dom and shadowrootmode attribute)
     <h1 id="name">
       <?child-node-part fullname?>
         First
@@ -277,7 +301,7 @@ <h1 id="name">
     <child-node-part test PI without leading ?>
     <!--?child-node-partfoobar?-->
     <?child-node-partfoobar?>
-</div>
+  </div>
 </div>
 <template id=declarative>
   <div>
@@ -296,61 +320,105 @@ <h1 id="name">
       <child-node-part test PI without leading ?>
       <!--?child-node-partfoobar?-->
       <?child-node-partfoobar?>
-</div>
+    </div>
   </div>
 </template>
+<div id=declarative_shadow_dom>
+  <template shadowrootmode="open">
+    <div>
+      <div id=target3>Declarative syntax
+        <h1 id="name">
+          <?child-node-part fullname?>
+            First
+            <!--?child-node-part middle?--> <?node-part middle-node?>Middle <?/child-node-part middle?>
+            Last
+          <!-- ?/child-node-part foobar? -->
+        </h1>
+        Email: <?node-part email-link?><a id="link"></a>
+
+        Here are some invalid parts that should not get parsed:
+        <!--child-node-part test comment without leading ?-->
+        <child-node-part test PI without leading ?>
+        <!--?child-node-partfoobar?-->
+        <?child-node-partfoobar?>
+      </div>
+    </div>
+  </template>
+</div>
 
 <script> {
 const template = document.getElementById('declarative');
-['Main Document','Template','Clone'].forEach(testCase => {
-  let doc,target,cleanup;
-  switch (testCase) {
-    case 'Main Document':
-      doc = document;
-      target = doc.querySelector('#target2');
-      cleanup = [target.parentElement];
-      break;
-    case 'Template':
-      doc = template.content;
-      target = doc.querySelector('#target3');
-      cleanup = [];
-      break;
-    case 'Clone':
-      doc = document;
-      wrapper = document.body.appendChild(document.createElement('div'));
-      wrapper.appendChild(template.content.cloneNode(true));
-      target = wrapper.querySelector('#target3');
-      cleanup = [wrapper];
-      break;
-  }
-  assert_true(!!(doc && target && target.parentElement));
-
+['Main Document','Template','Clone','PartClone','DeclarativeShadowDOM'].forEach(testCase => {
   function assertIsComment(node,commentText) {
     assert_true(node instanceof Comment);
     assert_equals(node.textContent,commentText);
   }
 
   test((t) => {
+    let doc,target,wrapper,cleanup;
+    let expectDOMParts = true;
+    switch (testCase) {
+      case 'Main Document':
+        doc = document;
+        target = doc.querySelector('#target2');
+        cleanup = [target.parentElement];
+        break;
+      case 'Template':
+        doc = template.content;
+        target = doc.querySelector('#target3');
+        cleanup = [];
+        break;
+      case 'Clone':
+        doc = document;
+        wrapper = document.body.appendChild(document.createElement('div'));
+        wrapper.appendChild(template.content.cloneNode(true));
+        target = wrapper.querySelector('#target3');
+        // A "normal" tree clone should not keep DOM Parts:
+        expectDOMParts = false;
+        cleanup = [wrapper];
+        break;
+      case 'PartClone':
+        doc = document;
+        wrapper = document.body.appendChild(document.createElement('div'));
+        wrapper.appendChild(template.content.getPartRoot().clone().rootContainer);
+        target = wrapper.querySelector('#target3');
+        cleanup = [wrapper];
+        break;
+      case 'DeclarativeShadowDOM': {
+        const host = document.getElementById('declarative_shadow_dom');
+        doc = host.shadowRoot;
+        target = doc.querySelector('#target3'); // Shadow isolated
+        cleanup = [host];
+        break;
+      }
+      default:
+        assert_unreached('Invalid test case');
+    }
+    assert_true(!!(doc && target && target.parentElement));
+    t.add_cleanup(() => cleanup.forEach(el => el.remove())); // Cleanup
+
     const root = doc.getPartRoot();
     assert_true(root instanceof DocumentPartRoot);
-    const expectedRootParts = [{type:'ChildNodePart',metadata:['fullname']},{type:'NodePart',metadata:['email-link']}];
-    assertEqualParts(root.getParts(),expectedRootParts,0,'declarative root should have two parts');
-    assert_equals(root.getParts()[1].node,target.querySelector('#link'));
-    const childPart1 = root.getParts()[0];
-    assertIsComment(childPart1.previousSibling,'?child-node-part fullname?');
-    assertIsComment(childPart1.nextSibling,' ?/child-node-part foobar? ');
-    const expectedChild1Parts = [{type:'ChildNodePart',metadata:['middle']}];
-    assertEqualParts(childPart1.getParts(),expectedChild1Parts,0,'First level childpart should just have one child part');
-    const childPart2 = childPart1.getParts()[0];
-    assertIsComment(childPart2.previousSibling,'?child-node-part middle?');
-    assertIsComment(childPart2.nextSibling,'?/child-node-part middle?');
-    const expectedChild2Parts = [{type:'NodePart',metadata:['middle-node']}];
-    assertEqualParts(childPart2.getParts(),expectedChild2Parts,0,'Second level childpart should have just the node part');
-    assert_true(childPart2.getParts()[0].node instanceof Text);
-    assert_equals(childPart2.getParts()[0].node.textContent,'Middle ');
+    if (expectDOMParts) {
+      const expectedRootParts = [{type:'ChildNodePart',metadata:['fullname']},{type:'NodePart',metadata:['email-link']}];
+      assertEqualParts(root.getParts(),expectedRootParts,0,'declarative root should have two parts');
+      assert_equals(root.getParts()[1].node,target.querySelector('#link'));
+      const childPart1 = root.getParts()[0];
+      assertIsComment(childPart1.previousSibling,'?child-node-part fullname?');
+      assertIsComment(childPart1.nextSibling,' ?/child-node-part foobar? ');
+      const expectedChild1Parts = [{type:'ChildNodePart',metadata:['middle']}];
+      assertEqualParts(childPart1.getParts(),expectedChild1Parts,0,'First level childpart should just have one child part');
+      const childPart2 = childPart1.getParts()[0];
+      assertIsComment(childPart2.previousSibling,'?child-node-part middle?');
+      assertIsComment(childPart2.nextSibling,'?/child-node-part middle?');
+      const expectedChild2Parts = [{type:'NodePart',metadata:['middle-node']}];
+      assertEqualParts(childPart2.getParts(),expectedChild2Parts,0,'Second level childpart should have just the node part');
+      assert_true(childPart2.getParts()[0].node instanceof Text);
+      assert_equals(childPart2.getParts()[0].node.textContent,'Middle ');
+    } else {
+      assertEqualParts(root.getParts(),[],[]);
+    }
   }, `Basic declarative DOM Parts (${testCase})`);
-
-  cleanup.forEach(el => el.remove()); // Cleanup
 });
 
 }</script>