[FIX] portal: correctly mount portal content in target created by mount

Previously, blah blah blah

src/runtime/blockdom/block_compiler.ts

@@ -517,9 +517,12 @@ function createBlockClass(template: HTMLElement, ctx: BlockCtx): BlockClass {
       return this.el!;
     }
 
-    moveBefore(other: Block | null, afterNode: Node | null) {
-      const target = other ? other.el! : afterNode;
-      nodeInsertBefore.call(this.parentEl, this.el!, target);
+    moveBeforeDOMNode(node: Node | null) {
+      nodeInsertBefore.call(this.parentEl, this.el!, node);
+    }
+
+    moveBeforeVNode(other: Block | null, afterNode: Node | null) {
+      nodeInsertBefore.call(this.parentEl, this.el!, other ? other.el! : afterNode);
     }
 
     toString() {

src/runtime/blockdom/event_catcher.ts

@@ -56,8 +56,17 @@ export function createCatcher(eventsSpec: EventsSpec): Catcher {
       }
     }
 
-    moveBefore(other: VCatcher | null, afterNode: Node | null) {
-      this.child.moveBefore(other ? other.child : null, afterNode);
+    moveBeforeDOMNode(node: Node | null) {
+      this.child.moveBeforeDOMNode(node);
+      this.parentEl!.insertBefore(this.afterNode!, node);
+    }
+
+    moveBeforeVNode(other: VCatcher | null, afterNode: Node | null) {
+      if (other) {
+        // check this with @ged-odoo for use in foreach
+        afterNode = other.firstNode() || afterNode;
+      }
+      this.child.moveBeforeVNode(other ? other.child : null, afterNode);
       this.parentEl!.insertBefore(this.afterNode!, afterNode);
     }
 

src/runtime/blockdom/html.ts

@@ -29,14 +29,18 @@ class VHtml {
     }
   }
 
-  moveBefore(other: VHtml | null, afterNode: Node | null) {
-    const target = other ? other.content[0] : afterNode;
+  moveBeforeDOMNode(node: Node | null) {
     const parent = this.parentEl;
     for (let elem of this.content) {
-      nodeInsertBefore.call(parent, elem, target);
+      nodeInsertBefore.call(parent, elem, node);
     }
   }
 
+  moveBeforeVNode(other: VHtml | null, afterNode: Node | null) {
+    const target = other ? other.content[0] : afterNode;
+    this.moveBeforeDOMNode(target);
+  }
+
   patch(other: VHtml) {
     if (this === other) {
       return;

src/runtime/blockdom/index.ts

@@ -10,7 +10,8 @@ export { createCatcher } from "./event_catcher";
 
 export interface VNode<T = any> {
   mount(parent: HTMLElement, afterNode: Node | null): void;
-  moveBefore(other: T | null, afterNode: Node | null): void;
+  moveBeforeDOMNode(node: Node | null): void;
+  moveBeforeVNode(other: T | null, afterNode: Node | null): void;
   patch(other: T, withBeforeRemove: boolean): void;
   beforeRemove(): void;
   remove(): void;

src/runtime/blockdom/list.ts

@@ -38,14 +38,22 @@ class VList {
     this.parentEl = parent;
   }
 
-  moveBefore(other: VList | null, afterNode: Node | null) {
+  moveBeforeDOMNode(node: Node | null) {
+    const children = this.children;
+    for (let i = 0, l = children.length; i < l; i++) {
+      children[i].moveBeforeDOMNode(node);
+    }
+    this.parentEl!.insertBefore(this.anchor!, node);
+  }
+
+  moveBeforeVNode(other: VList | null, afterNode: Node | null) {
     if (other) {
       const next = other!.children[0];
       afterNode = (next ? next.firstNode() : other!.anchor) || null;
     }
     const children = this.children;
     for (let i = 0, l = children.length; i < l; i++) {
-      children[i].moveBefore(null, afterNode);
+      children[i].moveBeforeVNode(null, afterNode);
     }
     this.parentEl!.insertBefore(this.anchor!, afterNode);
   }
@@ -66,7 +74,7 @@ class VList {
       patch: cPatch,
       remove: cRemove,
       beforeRemove,
-      moveBefore: cMoveBefore,
+      moveBeforeVNode: cMoveBefore,
       firstNode: cFirstNode,
     } = proto;
 

src/runtime/blockdom/multi.ts

@@ -38,7 +38,22 @@ export class VMulti {
     this.parentEl = parent;
   }
 
-  moveBefore(other: VMulti | null, afterNode: Node | null) {
+  moveBeforeDOMNode(node: Node | null) {
+    const children = this.children;
+    const parent = this.parentEl;
+    const anchors = this.anchors;
+    for (let i = 0, l = children.length; i < l; i++) {
+      let child = children[i];
+      if (child) {
+        child.moveBeforeDOMNode(node);
+      } else {
+        const anchor = anchors![i];
+        nodeInsertBefore.call(parent, anchor, node);
+      }
+    }
+  }
+
+  moveBeforeVNode(other: VMulti | null, afterNode: Node | null) {
     if (other) {
       const next = other!.children[0];
       afterNode = (next ? next.firstNode() : other!.anchors![0]) || null;
@@ -49,7 +64,7 @@ export class VMulti {
     for (let i = 0, l = children.length; i < l; i++) {
       let child = children[i];
       if (child) {
-        child.moveBefore(null, afterNode);
+        child.moveBeforeVNode(null, afterNode);
       } else {
         const anchor = anchors![i];
         nodeInsertBefore.call(parent, anchor, afterNode);

src/runtime/blockdom/text.ts

@@ -23,9 +23,12 @@ abstract class VSimpleNode {
     this.el = node;
   }
 
-  moveBefore(other: VText | null, afterNode: Node | null) {
-    const target = other ? other.el! : afterNode;
-    nodeInsertBefore.call(this.parentEl, this.el!, target);
+  moveBeforeDOMNode(node: Node | null) {
+    nodeInsertBefore.call(this.parentEl, this.el!, node);
+  }
+
+  moveBeforeVNode(other: VText | null, afterNode: Node | null) {
+    nodeInsertBefore.call(this.parentEl, this.el!, other ? other.el! : afterNode);
   }
 
   beforeRemove() {}

src/runtime/blockdom/toggler.ts

@@ -20,8 +20,12 @@ class VToggler {
     this.child.mount(parent, afterNode);
   }
 
-  moveBefore(other: VToggler | null, afterNode: Node | null) {
-    this.child.moveBefore(other ? other.child : null, afterNode);
+  moveBeforeDOMNode(node: Node | null) {
+    this.child.moveBeforeDOMNode(node);
+  }
+
+  moveBeforeVNode(other: VToggler | null, afterNode: Node | null) {
+    this.moveBeforeDOMNode((other && other.firstNode()) || afterNode);
   }
 
   patch(other: VToggler, withBeforeRemove: boolean) {

src/runtime/component_node.ts

@@ -306,8 +306,12 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
     this.fiber = null;
   }
 
-  moveBefore(other: ComponentNode | null, afterNode: Node | null) {
-    this.bdom!.moveBefore(other ? other.bdom : null, afterNode);
+  moveBeforeDOMNode(node: Node | null): void {
+    this.bdom!.moveBeforeDOMNode(node);
+  }
+
+  moveBeforeVNode(other: ComponentNode<P, E> | null, afterNode: Node | null) {
+    this.bdom!.moveBeforeVNode(other ? other.bdom : null, afterNode);
   }
 
   patch() {

src/runtime/portal.ts

@@ -21,20 +21,16 @@ class VPortal extends VText implements Partial<VNode<VPortal>> {
     this.target = document.querySelector(this.selector) as any;
     if (this.target) {
       this.content!.mount(this.target!, null);
-    } else {
-      this.content!.mount(parent, anchor);
     }
   }
 
   beforeRemove() {
-    this.content!.beforeRemove();
-  }
-  remove() {
-    if (this.content) {
-      super.remove();
+    // this.target not being null means content is mounted
+    if (this.target) {
+      this.content!.beforeRemove();
       this.content!.remove();
-      this.content = null;
     }
+    this.content = null;
   }
 
   patch(other: VPortal) {
@@ -73,9 +69,9 @@ export class Portal extends Component {
     onMounted(() => {
       const portal: VPortal = node.bdom;
       if (!portal.target) {
-        const target: HTMLElement = document.querySelector(this.props.target);
-        if (target) {
-          portal.content!.moveBefore(target, null);
+        portal.target = document.querySelector(this.props.target);
+        if (portal.target) {
+          portal.content!.mount(portal.target, null);
         } else {
           throw new OwlError("invalid portal target");
         }
@@ -84,7 +80,7 @@ export class Portal extends Component {
 
     onWillUnmount(() => {
       const portal: VPortal = node.bdom;
-      portal.remove();
+      portal.beforeRemove();
     });
   }
 }

tests/components/__snapshots__/t_foreach.test.ts.snap

@@ -149,6 +149,58 @@ exports[`list of components list of sub components inside other nodes 2`] = `
 }"
 `;
 
+exports[`list of components order is correct when slots are not of same type 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { capture, markRaw } = helpers;
+  const comp1 = app.createComponent(\`Child\`, true, true, false, true);
+  
+  let block2 = createBlock(\`<div>A</div>\`);
+  
+  function slot1(ctx, node, key = \\"\\") {
+    let b2;
+    if (!ctx['state'].active) {
+      b2 = block2();
+    }
+    return multi([b2]);
+  }
+  
+  function slot2(ctx, node, key = \\"\\") {
+    return text(\`B\`);
+  }
+  
+  function slot3(ctx, node, key = \\"\\") {
+    return text(\`C\`);
+  }
+  
+  return function template(ctx, node, key = \\"\\") {
+    const ctx1 = capture(ctx);
+    return comp1({slots: markRaw({'a': {__render: slot1, __ctx: ctx1, active: !ctx['state'].active}, 'b': {__render: slot2, __ctx: ctx1, active: true}, 'c': {__render: slot3, __ctx: ctx1, active: ctx['state'].active}})}, key + \`__1\`, node, this, null);
+  }
+}"
+`;
+
+exports[`list of components order is correct when slots are not of same type 2`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { prepareList, callSlot, withKey } = helpers;
+  
+  return function template(ctx, node, key = \\"\\") {
+    ctx = Object.create(ctx);
+    const [k_block1, v_block1, l_block1, c_block1] = prepareList(ctx['slotNames']);;
+    for (let i1 = 0; i1 < l_block1; i1++) {
+      ctx[\`slotName\`] = v_block1[i1];
+      const key1 = ctx['slotName'];
+      const slot1 = (ctx['slotName']);
+      c_block1[i1] = withKey(toggler(slot1, callSlot(ctx, node, key1 + \`__1__\${key1}\`, slot1, true, {})), key1);
+    }
+    return list(c_block1);
+  }
+}"
+`;
+
 exports[`list of components reconciliation alg works for t-foreach in t-foreach 1`] = `
 "function anonymous(app, bdom, helpers
 ) {

tests/components/t_foreach.test.ts

@@ -359,4 +359,35 @@ describe("list of components", () => {
     console.info = consoleInfo;
     expect(mockConsoleWarn).toBeCalledTimes(1);
   });
+
+  test("order is correct when slots are not of same type", async () => {
+    class Child extends Component {
+      static template = xml`
+          <t t-slot="{{ slotName }}" t-foreach="slotNames" t-as="slotName" t-key="slotName"/>
+      `;
+      get slotNames() {
+        return Object.entries(this.props.slots)
+          .filter((entry: any) => entry[1].active)
+          .map((entry) => entry[0]);
+      }
+    }
+
+    class Parent extends Component {
+      static template = xml`
+        <Child>
+          <t t-set-slot="a" active="!state.active"><div t-if="!state.active">A</div></t>
+          <t t-set-slot="b" active="true">B</t>
+          <t t-set-slot="c" active="state.active">C</t>
+        </Child>
+      `;
+      static components = { Child };
+      state = useState({ active: false });
+    }
+
+    const parent = await mount(Parent, fixture);
+    expect(fixture.textContent).toBe("AB");
+    parent.state.active = true;
+    await nextTick();
+    expect(fixture.textContent).toBe("BC");
+  });
 });

tests/misc/portal.test.ts

@@ -892,7 +892,7 @@ describe("Portal", () => {
     }
     await mount(Parent, fixture);
     expect(fixture.innerHTML).toBe(
-      '<span>child</span><div class="portal"></div><span>portal</span>'
+      '<span>child</span><div class="portal"><span>portal</span></div>'
     );
   });