[FIX] blockdom: correctly reorder children in heterogeneous t-foreach

Currently, the `moveBefore` method on VNodes assumes that the `other`
VNode it receives is of the same type, and that the entire VNode tree
below that other VNode has the exact same structure. While this is
correct in most cases, it breaks down when there is a VToggler somewhere
in the VNode tree, as the structure below a VToggler can be very
different from the structure below another VToggler that was created
from the same compiled code. For example, two iterations of a t-foreach
that contains a <t t-component="..."/> may spawn different components,
and different components obviously have different structures.

One way to fix this is to remove the assumption that the structure of
the `this` block tree in moveBefore is the same as the structure
of the `other` block tree, and instead, always give the concrete DOM
node before which we want to move the current VNode instead of giving it
a VNode and an afterNode as a fallback. One problem with this solution
is that it degrades performance in the "standard" case, where a
t-foreach contains no VToggler anywhere in its block tree, as retrieving
the first concrete DOM node requires calling firstNode() which
recursively traverses the entire tree.

To avoid this performance penalty in the standard case, we opt to only
go down this route whenever we encounter a VToggler when calling
`moveBefore`. This requires that we maintain two separate methods, one
to move a VNode before another VNode of assumed similar structure, which
is basically the current implementation of `moveBefore` for all VNode
types except VToggler, and one implementation that moves a VNode before
a concrete DOM node. This method needs to be implemented for all VNode
types, as all VNode types can be descendants of a VToggler. This method
will only be called from one place: the `moveBeforeVNode` method of the
toggler, which is the point where we realize that the assumption of
identical structure breaks down.

Co-authored-by: Bruno Boi <boi@odoo.com>

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

@@ -75,7 +75,7 @@ export class Portal extends Component {
       if (!portal.target) {
         const target: HTMLElement = document.querySelector(this.props.target);
         if (target) {
-          portal.content!.moveBefore(target, null);
+          portal.content!.moveBeforeVNode(target, null);
         } else {
           throw new OwlError("invalid portal target");
         }

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");
+  });
 });