From cb38d795f9df7081d367abf470edb6b3f73ab232 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Fri, 9 Oct 2020 13:33:17 +0200
Subject: [PATCH] [FIX] slots: properly handle named t-slots inside named
 t-slots

Previous code naively handled nested t-set-slots: if a second named
slots was found, it overrode the first.

In this commit, we use a set to make sure that we only use the first
found t-set-slot node. Also, we ignore set-slots defined in a sub
components, because these slots are only relevant to the sub component
itself.

Note that it works as expected because document.querySelectorAll
performs a search depth first, so we will always use the named slots
closer to the parent element, in term of depth.

closes #682
---
 src/component/directive.ts    | 42 +++++++++++++++-------
 tests/component/slots.test.ts | 65 +++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+), 13 deletions(-)

diff --git a/src/component/directive.ts b/src/component/directive.ts
index c4707b39..38555219 100644
--- a/src/component/directive.ts
+++ b/src/component/directive.ts
@@ -400,7 +400,6 @@ QWeb.addDirective({
 
     if (hasSlots) {
       const clone = <Element>node.cloneNode(true);
-      const slotNodes = Array.from(clone.querySelectorAll("[t-set-slot]"));
 
       // The next code is a fallback for compatibility reason. It accepts t-set
       // elements that are direct children with a non empty body as nodes defining
@@ -410,26 +409,43 @@ QWeb.addDirective({
       // code using slots. This will be removed in v2.0 someday. Meanwhile,
       // please use t-set-slot everywhere you need to set the content of a
       // slot.
-      for (let el of clone.children) {
-        if (el.getAttribute("t-set") && el.hasChildNodes()) {
-          slotNodes.push(el);
+      for (let node of clone.children) {
+        if (node.hasAttribute("t-set") && node.hasChildNodes()) {
+          node.setAttribute("t-set-slot", node.getAttribute("t-set")!);
+          node.removeAttribute("t-set");
         }
       }
+      const slotNodes = Array.from(clone.querySelectorAll("[t-set-slot]"));
+      const slotNames = new Set<string>();
       const slotId = QWeb.nextSlotId++;
       ctx.addLine(`w${componentID}.__owl__.slotId = ${slotId};`);
       if (slotNodes.length) {
         for (let i = 0, length = slotNodes.length; i < length; i++) {
           const slotNode = slotNodes[i];
-          slotNode.parentElement!.removeChild(slotNode);
-          let key = slotNode.getAttribute("t-set-slot")!;
-          slotNode.removeAttribute("t-set-slot");
-
-          // here again, this code should be removed when we stop supporting
-          // using t-set to define the content of named slots.
-          if (!key) {
-            key = slotNode.getAttribute("t-set")!;
-            slotNode.removeAttribute("t-set");
+          // check if this is defined in a sub component (in which case it should
+          // be ignored)
+          let el = slotNode.parentElement;
+          let isInSubComponent = false;
+          while (el !== clone) {
+            if (
+              el!.hasAttribute("t-component") ||
+              el!.tagName[0] === el!.tagName[0].toUpperCase()
+            ) {
+              isInSubComponent = true;
+              break;
+            }
           }
+          if (isInSubComponent) {
+            continue;
+          }
+          let key = slotNode.getAttribute("t-set-slot")!;
+          if (slotNames.has(key)) {
+            continue;
+          }
+          slotNames.add(key);
+          slotNode.removeAttribute("t-set-slot");
+          slotNode.parentElement!.removeChild(slotNode);
+
           const slotFn = qweb._compile(`slot_${key}_template`, slotNode, ctx);
           QWeb.slots[`${slotId}_${key}`] = slotFn;
         }
diff --git a/tests/component/slots.test.ts b/tests/component/slots.test.ts
index cd5fa3cd..ff1950b5 100644
--- a/tests/component/slots.test.ts
+++ b/tests/component/slots.test.ts
@@ -987,4 +987,69 @@ describe("t-slot directive", () => {
 
     expect(fixture.innerHTML).toBe("<div><span><p>sokka</p></span></div>");
   });
+
+  test("named slot inside slot", async () => {
+    class Child extends Component {
+      static template = xml`
+        <div>
+          <t t-slot="brol"/>
+          <t t-slot="default"/>
+        </div>`;
+    }
+    class Parent extends Component {
+      static template = xml`
+        <div>
+          <Child>
+            <t t-set-slot="brol">
+              <p>A<t t-esc="value"/></p>
+            </t>
+            <Child>
+              <t t-set-slot="brol">
+                <p>B<t t-esc="value"/></p>
+              </t>
+            </Child>
+          </Child>
+        </div>`;
+      static components = { Child };
+      value = "blip";
+    }
+    const parent = new Parent();
+    await parent.mount(fixture);
+
+    expect(fixture.innerHTML).toBe("<div><div><p>Ablip</p><div><p>Bblip</p></div></div></div>");
+  });
+
+  test("named slots inside slot, again", async () => {
+    class Child extends Component {
+      static template = xml`
+        <child>
+          <t t-slot="brol1">default1</t>
+          <t t-slot="brol2">default2</t>
+          <t t-slot="default"/>
+        </child>`;
+    }
+    class Parent extends Component {
+      static template = xml`
+        <div>
+          <Child>
+            <t t-set-slot="brol1">
+              <p>A<t t-esc="value"/></p>
+            </t>
+            <Child>
+              <t t-set-slot="brol2">
+                <p>B<t t-esc="value"/></p>
+              </t>
+            </Child>
+          </Child>
+        </div>`;
+      static components = { Child };
+      value = "blip";
+    }
+    const parent = new Parent();
+    await parent.mount(fixture);
+
+    expect(fixture.innerHTML).toBe(
+      "<div><child><p>Ablip</p>default2<child>default1<p>Bblip</p></child></child></div>"
+    );
+  });
 });