From ab29b896eba8c9602f283013f9d0a3fa16f603c9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Fri, 23 Sep 2022 11:22:42 +0200
Subject: [PATCH] [FIX] portal: make it work in all cases

Before this commit, the portal wouldn't work when its target is created
after the portal content, since it wouldn't be able to mount the dom at
the correct location.

With this commit, we work around the issue by mounting the portal
content at the portal location, then when the Portal component is
mounted, moving it to its correct location.

The big downside with that approach is that the portal content is
(sometimes) rendered and mounted at a location, THEN mounted in another
location. I think that it is most of the time not an issue, but one
could argue that it is inconsistent: some specific code could work at
one point, then fail in a different very similar situation (for example,
iframes don't support very well being moved around).  On the flip side,
having the portal work as expected is very useful, and may be worth the
tradeoff.

closes #1250
---
 src/runtime/portal.ts                        | 69 +++++++++---------
 tests/misc/__snapshots__/portal.test.ts.snap | 76 ++++++++++++++++++++
 tests/misc/portal.test.ts                    | 44 +++++++++++-
 3 files changed, 156 insertions(+), 33 deletions(-)

diff --git a/src/runtime/portal.ts b/src/runtime/portal.ts
index 4723a8d7..e06b1009 100644
--- a/src/runtime/portal.ts
+++ b/src/runtime/portal.ts
@@ -1,4 +1,4 @@
-import { onWillUnmount } from "./lifecycle_hooks";
+import { onMounted, onWillUnmount } from "./lifecycle_hooks";
 import { BDom, text, VNode } from "./blockdom";
 import { Component } from "./component";
 import { OwlError } from "./error_handling";
@@ -6,60 +6,55 @@ import { OwlError } from "./error_handling";
 const VText: any = text("").constructor;
 
 class VPortal extends VText implements Partial<VNode<VPortal>> {
-  //   selector: string;
-  realBDom: BDom | null;
+  content: BDom | null;
+  selector: string;
   target: HTMLElement | null = null;
 
-  constructor(selector: string, realBDom: BDom) {
+  constructor(selector: string, content: BDom) {
     super("");
     this.selector = selector;
-    this.realBDom = realBDom;
+    this.content = content;
   }
+
   mount(parent: HTMLElement, anchor: ChildNode) {
     super.mount(parent, anchor);
     this.target = document.querySelector(this.selector) as any;
-    if (!this.target) {
-      let el: any = this.el;
-      while (el && el.parentElement instanceof HTMLElement) {
-        el = el.parentElement;
-      }
-      this.target = el && el.querySelector(this.selector);
-      if (!this.target) {
-        throw new OwlError("invalid portal target");
-      }
+    if (this.target) {
+      this.content!.mount(this.target!, null);
+    } else {
+      this.content!.mount(parent, anchor);
     }
-    this.realBDom!.mount(this.target!, null);
   }
 
   beforeRemove() {
-    this.realBDom!.beforeRemove();
+    this.content!.beforeRemove();
   }
   remove() {
-    if (this.realBDom) {
+    if (this.content) {
       super.remove();
-      this.realBDom!.remove();
-      this.realBDom = null;
+      this.content!.remove();
+      this.content = null;
     }
   }
 
   patch(other: VPortal) {
     super.patch(other);
-    if (this.realBDom) {
-      this.realBDom.patch(other.realBDom!, true);
+    if (this.content) {
+      this.content.patch(other.content!, true);
     } else {
-      this.realBDom = other.realBDom;
-      this.realBDom!.mount(this.target!, null);
+      this.content = other.content;
+      this.content!.mount(this.target!, null);
     }
   }
 }
 
 /**
- * <t t-slot="default"/>
+ * kind of similar to <t t-slot="default"/>, but it wraps it around a VPortal
  */
 export function portalTemplate(app: any, bdom: any, helpers: any) {
   let { callSlot } = helpers;
-  return function template(ctx: any, node: any, key = "") {
-    return callSlot(ctx, node, key, "default", false, null);
+  return function template(ctx: any, node: any, key = ""): any {
+    return new VPortal(ctx.props.target, callSlot(ctx, node, key, "default", false, null));
   };
 }
 
@@ -73,13 +68,23 @@ export class Portal extends Component {
   };
 
   setup() {
-    const node = this.__owl__;
-    const renderFn = node.renderFn;
-    node.renderFn = () => new VPortal(this.props.target, renderFn());
-    onWillUnmount(() => {
-      if (node.bdom) {
-        node.bdom.remove();
+    const node: any = this.__owl__;
+
+    onMounted(() => {
+      const portal: VPortal = node.bdom;
+      if (!portal.target) {
+        const target: HTMLElement = document.querySelector(this.props.target);
+        if (target) {
+          portal.content!.moveBefore(target, null);
+        } else {
+          throw new OwlError("invalid portal target");
+        }
       }
     });
+
+    onWillUnmount(() => {
+      const portal: VPortal = node.bdom;
+      portal.remove();
+    });
   }
 }
diff --git a/tests/misc/__snapshots__/portal.test.ts.snap b/tests/misc/__snapshots__/portal.test.ts.snap
index dca22dd3..fd61e0f5 100644
--- a/tests/misc/__snapshots__/portal.test.ts.snap
+++ b/tests/misc/__snapshots__/portal.test.ts.snap
@@ -155,6 +155,44 @@ exports[`Portal Add and remove portals with t-foreach inside div 1`] = `
 }"
 `;
 
+exports[`Portal Child and Portal 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  const comp1 = app.createComponent(\`Child\`, true, false, false, true);
+  
+  let block3 = createBlock(\`<div class=\\"portal\\"/>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = comp1({}, key + \`__1\`, node, this, null);
+    const b3 = block3();
+    return multi([b2, b3]);
+  }
+}"
+`;
+
+exports[`Portal Child and Portal 2`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  const Portal = app.Portal;
+  const comp1 = app.createComponent(null, false, true, false, false);
+  
+  let block2 = createBlock(\`<span>child</span>\`);
+  let block3 = createBlock(\`<span>portal</span>\`);
+  
+  function slot1(ctx, node, key = \\"\\") {
+    return block3();
+  }
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = block2();
+    const b4 = comp1({target: '.portal',slots: {'default': {__render: slot1, __ctx: ctx}}}, key + \`__1\`, node, ctx, Portal);
+    return multi([b2, b4]);
+  }
+}"
+`;
+
 exports[`Portal Portal composed with t-slot 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
@@ -515,6 +553,44 @@ exports[`Portal lifecycle hooks of portal sub component are properly called 2`]
 }"
 `;
 
+exports[`Portal portal and Child 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  const comp1 = app.createComponent(\`Child\`, true, false, false, true);
+  
+  let block2 = createBlock(\`<div class=\\"portal\\"/>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = block2();
+    const b3 = comp1({}, key + \`__1\`, node, this, null);
+    return multi([b2, b3]);
+  }
+}"
+`;
+
+exports[`Portal portal and Child 2`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  const Portal = app.Portal;
+  const comp1 = app.createComponent(null, false, true, false, false);
+  
+  let block2 = createBlock(\`<span>child</span>\`);
+  let block3 = createBlock(\`<span>portal</span>\`);
+  
+  function slot1(ctx, node, key = \\"\\") {
+    return block3();
+  }
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = block2();
+    const b4 = comp1({target: '.portal',slots: {'default': {__render: slot1, __ctx: ctx}}}, key + \`__1\`, node, ctx, Portal);
+    return multi([b2, b4]);
+  }
+}"
+`;
+
 exports[`Portal portal could have dynamically no content 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
diff --git a/tests/misc/portal.test.ts b/tests/misc/portal.test.ts
index 103aa669..33fdbe45 100644
--- a/tests/misc/portal.test.ts
+++ b/tests/misc/portal.test.ts
@@ -276,7 +276,7 @@ describe("Portal", () => {
 
     expect(error!).toBeDefined();
     expect(error!.message).toBe("invalid portal target");
-    expect(fixture.innerHTML).toBe(`<div></div>`);
+    expect(fixture.innerHTML).toBe(``);
     expect(mockConsoleWarn).toBeCalledTimes(1);
   });
 
@@ -874,6 +874,48 @@ describe("Portal", () => {
     await nextTick();
     expect(fixture.innerHTML).toBe('<div id="outside"></div><div></div>');
   });
+
+  test("Child and Portal", async () => {
+    class Child extends Component {
+      static template = xml`
+        <span>child</span>
+        <t t-portal="'.portal'"><span>portal</span></t>`;
+    }
+    class Parent extends Component {
+      static template = xml`
+        <t>
+          <Child/>
+          <div class="portal"></div>
+        </t>`;
+
+      static components = { Child };
+    }
+    await mount(Parent, fixture);
+    expect(fixture.innerHTML).toBe(
+      '<span>child</span><div class="portal"></div><span>portal</span>'
+    );
+  });
+
+  test("portal and Child", async () => {
+    class Child extends Component {
+      static template = xml`
+        <span>child</span>
+        <t t-portal="'.portal'"><span>portal</span></t>`;
+    }
+    class Parent extends Component {
+      static template = xml`
+        <t>
+          <div class="portal"></div>
+          <Child/>
+        </t>`;
+
+      static components = { Child };
+    }
+    await mount(Parent, fixture);
+    expect(fixture.innerHTML).toBe(
+      '<div class="portal"><span>portal</span></div><span>child</span>'
+    );
+  });
 });
 
 describe("Portal: UI/UX", () => {