From 8d25bddda497e1fb7c2a0fbf93c14df7072be476 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Mon, 14 Sep 2020 21:17:52 +0200 Subject: [PATCH] [FIX] component: subtle issue with unmounted children Owl has to manage a lot of interesting situations. One of them is when a rendering is initiated, which creates a sub component, but then another rendering starts, which invalidate the previous one, and will create another sub component. Since the first sub component was not ever in the DOM, we cannot rely on the vdom patching process to remove it, so we have to do it manually. Sadly, this is actually a very tricky situation, since there are other subtle situations where the code that remove an unmounted widget could be executed, in particular when the parent component is unmounted, then remounted, then modified to trigger yet another rendering. In this commit, we handle this case more carefully by making sure that the destroyed subcomponent properly configures its pvnode so the patch process happens as expected. joint work with the framework team, and in particular LPE for his work on finding a testcase! closes #724, #731 --- src/component/component.ts | 25 +++++++- src/qweb/base_directives.ts | 8 +-- src/qweb/qweb.ts | 4 +- tests/component/component.test.ts | 16 ++--- tests/component/un_mounting.test.ts | 99 +++++++++++++++++++++++++++++ tests/vdom.test.ts | 8 +-- 6 files changed, 137 insertions(+), 23 deletions(-) diff --git a/src/component/component.ts b/src/component/component.ts index 84a8dd3b..bd2bcd2a 100644 --- a/src/component/component.ts +++ b/src/component/component.ts @@ -656,9 +656,28 @@ export class Component { // destroyed right now, because they are not in the DOM, and thus we won't // be notified later on (when patching), that they are removed from the DOM for (let childKey in __owl__.children) { - let child = __owl__.children[childKey]; - if (!child.__owl__.isMounted && child.__owl__.parentLastFiberId < fiber.id) { - child.destroy(); + const child = __owl__.children[childKey]; + const childOwl = child.__owl__; + if (!childOwl.isMounted && childOwl.parentLastFiberId < fiber.id) { + // we only do here a "soft" destroy, meaning that we leave the child + // dom node alone, without removing it. Most of the time, it does not + // matter, because the child component is already unmounted. However, + // if some of its parent have been unmounted, the child could actually + // still be attached to its parent, and this may be important if we + // want to remount the parent, because the vdom need to match the + // actual DOM + child.__destroy(childOwl.parent); + if (childOwl.pvnode) { + // we remove the key here to make sure that the patching algorithm + // is able to make the difference between this pvnode and an eventual + // other instance of the same component + delete childOwl.pvnode.key; + // Since the component has been unmounted, we do not want to actually + // call a remove hook. This is pretty important, since the t-component + // directive actually disabled it, so the vdom algorithm will just + // not remove the child elm if we don't remove the hook. + delete childOwl.pvnode.data!.hook!.remove; + } } } if (!vnode) { diff --git a/src/qweb/base_directives.ts b/src/qweb/base_directives.ts index feff7b2c..95c9a3cd 100644 --- a/src/qweb/base_directives.ts +++ b/src/qweb/base_directives.ts @@ -275,16 +275,12 @@ QWeb.addDirective({ const parentNode = ctx.parentNode ? `c${ctx.parentNode}` : "result"; const extra = `Object.assign({}, extra, {parentNode: ${parentNode}, parent: ${parentComponent}, key: ${key}})`; if (ctx.parentNode) { - ctx.addLine( - `this.constructor.subTemplates['${subId}'].call(this, scope, ${extra});` - ); + ctx.addLine(`this.constructor.subTemplates['${subId}'].call(this, scope, ${extra});`); } else { // this is a t-call with no parentnode, we need to extract the result ctx.rootContext.shouldDefineResult = true; ctx.addLine(`result = []`); - ctx.addLine( - `this.constructor.subTemplates['${subId}'].call(this, scope, ${extra});` - ); + ctx.addLine(`this.constructor.subTemplates['${subId}'].call(this, scope, ${extra});`); ctx.addLine(`result = result[0]`); } diff --git a/src/qweb/qweb.ts b/src/qweb/qweb.ts index 618807ec..d9e6b067 100644 --- a/src/qweb/qweb.ts +++ b/src/qweb/qweb.ts @@ -216,8 +216,8 @@ export class QWeb extends EventBus { // id, and a (global) mapping from an id to the compiled function. This is // necessary to ensure that global templates can be called with more than one // QWeb instance. - subTemplates: {[key: string]: number} = {}; - static subTemplates: {[id: number]: Function} = {}; + subTemplates: { [key: string]: number } = {}; + static subTemplates: { [id: number]: Function } = {}; isUpdating: boolean = false; translateFn?: QWebConfig["translateFn"]; diff --git a/tests/component/component.test.ts b/tests/component/component.test.ts index 3500c2b7..e731c1c3 100644 --- a/tests/component/component.test.ts +++ b/tests/component/component.test.ts @@ -3021,7 +3021,7 @@ describe("random stuff/miscellaneous", () => { static template = xml`
__ -
` + `; } class Child extends Component { static components = { Custom }; @@ -3029,7 +3029,7 @@ describe("random stuff/miscellaneous", () => { ` + subKey="props.subKey"/>`; } class Parent extends Component { @@ -3045,24 +3045,24 @@ describe("random stuff/miscellaneous", () => { } const parent = new Parent(null); await parent.mount(fixture); - expect(fixture.textContent!.trim()).toBe('1__1'); + expect(fixture.textContent!.trim()).toBe("1__1"); // First step: change the Custom's instance Object.assign(parent.childProps, { - subKey: 2, + subKey: 2, }); parent.render(); await nextTick(); - expect(fixture.textContent!.trim()).toBe('1__2'); + expect(fixture.textContent!.trim()).toBe("1__2"); // Second step, change both Child's and Custom's instance Object.assign(parent.childProps, { - key: 2, - subKey: 3, + key: 2, + subKey: 3, }); parent.render(); await nextTick(); - expect(fixture.textContent!.trim()).toBe('2__3'); + expect(fixture.textContent!.trim()).toBe("2__3"); }); }); diff --git a/tests/component/un_mounting.test.ts b/tests/component/un_mounting.test.ts index 3c6730c8..10411a1e 100644 --- a/tests/component/un_mounting.test.ts +++ b/tests/component/un_mounting.test.ts @@ -509,4 +509,103 @@ describe("unmounting and remounting", () => { expect(error).toBeDefined(); expect(error.message).toBe("Cannot mount a destroyed component"); }); + + test("destroying a sub-component cleans itself from parent's vnode", async () => { + class C1 extends Component { + static template = xml`
`; + } + class P extends Component { + static components = { C1 }; + static template = xml`
`; + state = { + a: "first", + }; + } + const parent = new P(); + await parent.mount(fixture); + expect(fixture.textContent).toBe("first"); + parent.unmount(); + parent.state.a = ""; + parent.mount(fixture); + parent.state.a = "fixed"; + await parent.render(); + expect(fixture.textContent).toBe("fixed"); + }); + + test("destroying a sub-component cleans itself from parent's vnode, part 2", async () => { + class C1 extends Component { + static template = xml`
`; + } + class P extends Component { + static components = { C1 }; + static template = xml`
some text
`; + state = { + a: "first", + }; + } + const parent = new P(); + await parent.mount(fixture); + expect(fixture.textContent).toBe("firstsome text"); + parent.unmount(); + parent.state.a = ""; + parent.mount(fixture); + parent.state.a = "fixed"; + await parent.render(); + expect(fixture.textContent).toBe("fixedsome text"); + }); + + test("destroying a sub-component cleans itself from parent's vnode, part 3", async () => { + class C1 extends Component { + static template = xml`
`; + } + + class C2 extends Component { + static template = xml``; + static components = { C1 }; + } + + class P extends Component { + static components = { C2 }; + static template = xml`
`; + state = { + a: "first", + }; + } + const parent = new P(); + await parent.mount(fixture); + expect(fixture.textContent).toBe("first"); + parent.unmount(); + parent.state.a = ""; + parent.mount(fixture); + parent.state.a = "fixed"; + await parent.render(); + expect(fixture.textContent).toBe("fixed"); + }); + + test("destroying a sub-component cleans itself from parent's vnode, part 4", async () => { + class C1 extends Component { + static template = xml`
`; + } + + class C2 extends Component { + static template = xml``; + static components = { C1 }; + } + class P extends Component { + static components = { C2 }; + static template = xml`
some text
`; + state = { + a: "first", + }; + } + const parent = new P(); + await parent.mount(fixture); + expect(fixture.textContent).toBe("firstsome text"); + parent.unmount(); + parent.state.a = ""; + parent.mount(fixture); + parent.state.a = "fixed"; + await parent.render(); + expect(fixture.textContent).toBe("fixedsome text"); + }); }); diff --git a/tests/vdom.test.ts b/tests/vdom.test.ts index e1de7dff..d4a12517 100644 --- a/tests/vdom.test.ts +++ b/tests/vdom.test.ts @@ -1107,9 +1107,9 @@ describe("html to vdom", function () { }); test("svg", function () { - const nodeList = htmlToVDOM(``); - expect(nodeList).toHaveLength(1); - elm = patch(vnode0, nodeList[0]).elm; - expect(elm).toBeInstanceOf(SVGSVGElement); + const nodeList = htmlToVDOM(``); + expect(nodeList).toHaveLength(1); + elm = patch(vnode0, nodeList[0]).elm; + expect(elm).toBeInstanceOf(SVGSVGElement); }); });