From 30d699483657d4012bcf18e9d80cf11174553267 Mon Sep 17 00:00:00 2001 From: Samuel Degueldre Date: Wed, 19 Oct 2022 09:14:20 +0200 Subject: [PATCH] [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 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 --- src/runtime/blockdom/block_compiler.ts | 9 ++-- src/runtime/blockdom/event_catcher.ts | 13 ++++- src/runtime/blockdom/html.ts | 10 ++-- src/runtime/blockdom/index.ts | 3 +- src/runtime/blockdom/list.ts | 14 +++-- src/runtime/blockdom/multi.ts | 19 ++++++- src/runtime/blockdom/text.ts | 9 ++-- src/runtime/blockdom/toggler.ts | 8 ++- src/runtime/component_node.ts | 8 ++- src/runtime/portal.ts | 2 +- .../__snapshots__/t_foreach.test.ts.snap | 52 +++++++++++++++++++ tests/components/t_foreach.test.ts | 31 +++++++++++ 12 files changed, 156 insertions(+), 22 deletions(-) diff --git a/src/runtime/blockdom/block_compiler.ts b/src/runtime/blockdom/block_compiler.ts index 0730ee8a..b04fc804 100644 --- a/src/runtime/blockdom/block_compiler.ts +++ b/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() { diff --git a/src/runtime/blockdom/event_catcher.ts b/src/runtime/blockdom/event_catcher.ts index c3cc0b2b..7bd4da58 100644 --- a/src/runtime/blockdom/event_catcher.ts +++ b/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); } diff --git a/src/runtime/blockdom/html.ts b/src/runtime/blockdom/html.ts index 0c01eccf..fe88fa40 100644 --- a/src/runtime/blockdom/html.ts +++ b/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; diff --git a/src/runtime/blockdom/index.ts b/src/runtime/blockdom/index.ts index 0bc65806..165fa5d0 100644 --- a/src/runtime/blockdom/index.ts +++ b/src/runtime/blockdom/index.ts @@ -10,7 +10,8 @@ export { createCatcher } from "./event_catcher"; export interface VNode { 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; diff --git a/src/runtime/blockdom/list.ts b/src/runtime/blockdom/list.ts index 08fd5749..0b7c257a 100644 --- a/src/runtime/blockdom/list.ts +++ b/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; diff --git a/src/runtime/blockdom/multi.ts b/src/runtime/blockdom/multi.ts index c9278208..b375020f 100644 --- a/src/runtime/blockdom/multi.ts +++ b/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); diff --git a/src/runtime/blockdom/text.ts b/src/runtime/blockdom/text.ts index 25da7baf..687eb21e 100644 --- a/src/runtime/blockdom/text.ts +++ b/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() {} diff --git a/src/runtime/blockdom/toggler.ts b/src/runtime/blockdom/toggler.ts index d31c84ad..a734813f 100644 --- a/src/runtime/blockdom/toggler.ts +++ b/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) { diff --git a/src/runtime/component_node.ts b/src/runtime/component_node.ts index e6636001..f27867fc 100644 --- a/src/runtime/component_node.ts +++ b/src/runtime/component_node.ts @@ -306,8 +306,12 @@ export class ComponentNode

implements VNode | null, afterNode: Node | null) { + this.bdom!.moveBeforeVNode(other ? other.bdom : null, afterNode); } patch() { diff --git a/src/runtime/portal.ts b/src/runtime/portal.ts index e06b1009..f85f3d62 100644 --- a/src/runtime/portal.ts +++ b/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"); } diff --git a/tests/components/__snapshots__/t_foreach.test.ts.snap b/tests/components/__snapshots__/t_foreach.test.ts.snap index 665eeaec..1799a234 100644 --- a/tests/components/__snapshots__/t_foreach.test.ts.snap +++ b/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(\`

A
\`); + + 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 ) { diff --git a/tests/components/t_foreach.test.ts b/tests/components/t_foreach.test.ts index f809fc97..4c783b66 100644 --- a/tests/components/t_foreach.test.ts +++ b/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` + + `; + 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` + +
A
 + B + C + + `; + 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"); + }); });