From 6f23b18cab86945f4d459ebad0013716f1b2a920 Mon Sep 17 00:00:00 2001
From: Samuel Degueldre <sad@odoo.com>
Date: Thu, 20 Oct 2022 09:00:39 +0200
Subject: [PATCH] [FIX] portal: correctly move portal content when target is
 after it

Previously, when trying to mount a portal into a target that would be
mounted in the same render as the portal itself, the portal content
could not be mounted correctly. In a previous attempt to fix it, a
mistake was made while writing the test causing us to incorrectly
believe in now worked, when in fact, it would just move the portal
content to the end of its parent but without changing it.

This commit fixes the issue by making the `moveBeforeDOMNode` method of
VNodes accept a second optional parameter which is the parent in which
the element should be moved, defaulting to the current parent. This
results in identical behaviour when a parent is not specified, but when
it is, the VNode is "reparented" to the passed parent, which is what is
now done by the Portal
---
 src/runtime/blockdom/block_compiler.ts | 5 +++--
 src/runtime/blockdom/event_catcher.ts  | 7 ++++---
 src/runtime/blockdom/html.ts           | 4 ++--
 src/runtime/blockdom/index.ts          | 2 +-
 src/runtime/blockdom/list.ts           | 7 ++++---
 src/runtime/blockdom/multi.ts          | 6 +++---
 src/runtime/blockdom/text.ts           | 5 +++--
 src/runtime/blockdom/toggler.ts        | 4 ++--
 src/runtime/component_node.ts          | 4 ++--
 src/runtime/portal.ts                  | 2 +-
 tests/misc/portal.test.ts              | 2 +-
 11 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/src/runtime/blockdom/block_compiler.ts b/src/runtime/blockdom/block_compiler.ts
index b04fc804..9857bdfa 100644
--- a/src/runtime/blockdom/block_compiler.ts
+++ b/src/runtime/blockdom/block_compiler.ts
@@ -517,8 +517,9 @@ function createBlockClass(template: HTMLElement, ctx: BlockCtx): BlockClass {
       return this.el!;
     }
 
-    moveBeforeDOMNode(node: Node | null) {
-      nodeInsertBefore.call(this.parentEl, this.el!, node);
+    moveBeforeDOMNode(node: Node | null, parent = this.parentEl) {
+      this.parentEl = parent;
+      nodeInsertBefore.call(parent, this.el!, node);
     }
 
     moveBeforeVNode(other: Block | null, afterNode: Node | null) {
diff --git a/src/runtime/blockdom/event_catcher.ts b/src/runtime/blockdom/event_catcher.ts
index 7bd4da58..bd59d577 100644
--- a/src/runtime/blockdom/event_catcher.ts
+++ b/src/runtime/blockdom/event_catcher.ts
@@ -56,9 +56,10 @@ export function createCatcher(eventsSpec: EventsSpec): Catcher {
       }
     }
 
-    moveBeforeDOMNode(node: Node | null) {
-      this.child.moveBeforeDOMNode(node);
-      this.parentEl!.insertBefore(this.afterNode!, node);
+    moveBeforeDOMNode(node: Node | null, parent = this.parentEl) {
+      this.parentEl = parent;
+      this.child.moveBeforeDOMNode(node, parent);
+      parent!.insertBefore(this.afterNode!, node);
     }
 
     moveBeforeVNode(other: VCatcher | null, afterNode: Node | null) {
diff --git a/src/runtime/blockdom/html.ts b/src/runtime/blockdom/html.ts
index fe88fa40..1b33df75 100644
--- a/src/runtime/blockdom/html.ts
+++ b/src/runtime/blockdom/html.ts
@@ -29,8 +29,8 @@ class VHtml {
     }
   }
 
-  moveBeforeDOMNode(node: Node | null) {
-    const parent = this.parentEl;
+  moveBeforeDOMNode(node: Node | null, parent = this.parentEl) {
+    this.parentEl = parent;
     for (let elem of this.content) {
       nodeInsertBefore.call(parent, elem, node);
     }
diff --git a/src/runtime/blockdom/index.ts b/src/runtime/blockdom/index.ts
index 165fa5d0..99218f12 100644
--- a/src/runtime/blockdom/index.ts
+++ b/src/runtime/blockdom/index.ts
@@ -10,7 +10,7 @@ export { createCatcher } from "./event_catcher";
 
 export interface VNode<T = any> {
   mount(parent: HTMLElement, afterNode: Node | null): void;
-  moveBeforeDOMNode(node: Node | null): void;
+  moveBeforeDOMNode(node: Node | null, parent?: HTMLElement): void;
   moveBeforeVNode(other: T | null, afterNode: Node | null): void;
   patch(other: T, withBeforeRemove: boolean): void;
   beforeRemove(): void;
diff --git a/src/runtime/blockdom/list.ts b/src/runtime/blockdom/list.ts
index 0b7c257a..d3db6722 100644
--- a/src/runtime/blockdom/list.ts
+++ b/src/runtime/blockdom/list.ts
@@ -38,12 +38,13 @@ class VList {
     this.parentEl = parent;
   }
 
-  moveBeforeDOMNode(node: Node | null) {
+  moveBeforeDOMNode(node: Node | null, parent = this.parentEl) {
+    this.parentEl = parent;
     const children = this.children;
     for (let i = 0, l = children.length; i < l; i++) {
-      children[i].moveBeforeDOMNode(node);
+      children[i].moveBeforeDOMNode(node, parent);
     }
-    this.parentEl!.insertBefore(this.anchor!, node);
+    parent!.insertBefore(this.anchor!, node);
   }
 
   moveBeforeVNode(other: VList | null, afterNode: Node | null) {
diff --git a/src/runtime/blockdom/multi.ts b/src/runtime/blockdom/multi.ts
index b375020f..9fad5fff 100644
--- a/src/runtime/blockdom/multi.ts
+++ b/src/runtime/blockdom/multi.ts
@@ -38,14 +38,14 @@ export class VMulti {
     this.parentEl = parent;
   }
 
-  moveBeforeDOMNode(node: Node | null) {
+  moveBeforeDOMNode(node: Node | null, parent = this.parentEl) {
+    this.parentEl = parent;
     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);
+        child.moveBeforeDOMNode(node, parent);
       } else {
         const anchor = anchors![i];
         nodeInsertBefore.call(parent, anchor, node);
diff --git a/src/runtime/blockdom/text.ts b/src/runtime/blockdom/text.ts
index 687eb21e..c01da3d3 100644
--- a/src/runtime/blockdom/text.ts
+++ b/src/runtime/blockdom/text.ts
@@ -23,8 +23,9 @@ abstract class VSimpleNode {
     this.el = node;
   }
 
-  moveBeforeDOMNode(node: Node | null) {
-    nodeInsertBefore.call(this.parentEl, this.el!, node);
+  moveBeforeDOMNode(node: Node | null, parent = this.parentEl) {
+    this.parentEl = parent;
+    nodeInsertBefore.call(parent, this.el!, node);
   }
 
   moveBeforeVNode(other: VText | null, afterNode: Node | null) {
diff --git a/src/runtime/blockdom/toggler.ts b/src/runtime/blockdom/toggler.ts
index a734813f..22415439 100644
--- a/src/runtime/blockdom/toggler.ts
+++ b/src/runtime/blockdom/toggler.ts
@@ -20,8 +20,8 @@ class VToggler {
     this.child.mount(parent, afterNode);
   }
 
-  moveBeforeDOMNode(node: Node | null) {
-    this.child.moveBeforeDOMNode(node);
+  moveBeforeDOMNode(node: Node | null, parent?: HTMLElement) {
+    this.child.moveBeforeDOMNode(node, parent);
   }
 
   moveBeforeVNode(other: VToggler | null, afterNode: Node | null) {
diff --git a/src/runtime/component_node.ts b/src/runtime/component_node.ts
index f27867fc..de925700 100644
--- a/src/runtime/component_node.ts
+++ b/src/runtime/component_node.ts
@@ -306,8 +306,8 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
     this.fiber = null;
   }
 
-  moveBeforeDOMNode(node: Node | null): void {
-    this.bdom!.moveBeforeDOMNode(node);
+  moveBeforeDOMNode(node: Node | null, parent?: HTMLElement): void {
+    this.bdom!.moveBeforeDOMNode(node, parent);
   }
 
   moveBeforeVNode(other: ComponentNode<P, E> | null, afterNode: Node | null) {
diff --git a/src/runtime/portal.ts b/src/runtime/portal.ts
index f85f3d62..37ba8a32 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!.moveBeforeVNode(target, null);
+          portal.content!.moveBeforeDOMNode(target.firstChild, target);
         } else {
           throw new OwlError("invalid portal target");
         }
diff --git a/tests/misc/portal.test.ts b/tests/misc/portal.test.ts
index 33fdbe45..b5c230b4 100644
--- a/tests/misc/portal.test.ts
+++ b/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>'
     );
   });