From 1ce6964313f0dcce3e4fd4596ff1a975cb7774a2 Mon Sep 17 00:00:00 2001 From: nguyenyou Date: Tue, 22 Sep 2026 17:09:13 +0700 Subject: [PATCH 1/4] Test: reproduce stale content mounting after moving a dynamic group Adds regression tests for stale content mounting after a nested group move. Co-Authored-By: Codex GPT-6 Astra --- .../NestedGroupActivationOrderSpec.scala | 234 ++++++++++++++++++ 1 file changed, 234 insertions(+) create mode 100644 src/test/scala/com/raquo/laminar/tests/NestedGroupActivationOrderSpec.scala diff --git a/src/test/scala/com/raquo/laminar/tests/NestedGroupActivationOrderSpec.scala b/src/test/scala/com/raquo/laminar/tests/NestedGroupActivationOrderSpec.scala new file mode 100644 index 00000000..a5392ba6 --- /dev/null +++ b/src/test/scala/com/raquo/laminar/tests/NestedGroupActivationOrderSpec.scala @@ -0,0 +1,234 @@ +package com.raquo.laminar.tests + +import com.raquo.laminar.api.L._ +import com.raquo.laminar.inserters.Inserter +import com.raquo.laminar.utils.{EventTracker, UnitSpec} + +/** A moved dynamic inserter must behave like a moved element on its host's next activation: the + * inserter re-renders first, and content it no longer wants is dropped WITHOUT ever mounting. + * These tests pin that a move never makes stale content mount (running its mount hooks) only to + * be unmounted moments later by the inserter's own re-render of a value that changed while it + * was inactive. + */ +class NestedGroupActivationOrderSpec extends UnitSpec { + + // -- Fixture: a `child <--` that builds a FRESH tracked span per rendered value (the common + // `signal.map(render)` idiom). A value that changes while the inserter is inactive is rendered + // on the next activation, replacing the previous span. Each span is tagged with the render + // ordinal so the two generations are distinguishable in the log. + + private class FreshRenderer(tracker: EventTracker) { + val valueVar = Var("v") + private var renderCount = 0 + val inserter: Inserter = child <-- valueVar.signal.map { v => + renderCount += 1 + tracker.createSpan(s"$v$renderCount") + } + } + + it("REFERENCE: a group that was never moved re-renders BEFORE its stale content can mount on remount") { + // Baseline for the tests below: a freshly placed group's inserter re-runs first on remount, so + // the span rendered before the unmount is discarded without a mount, and only the new one mounts. + val tracker = createEventTracker() + val r = new FreshRenderer(tracker) + val items = Var[List[Inserter]](Nil) + val root = div(div("LIST", children <-- items.signal)) + + mount(root) + + withClue("first placement renders and mounts the first span:") { + items.set(List(r.inserter)) + tracker.assertEvents(_.elementCreated("v1"), _.mounted("v1")).clear() + expectNode(div.of(div.of("LIST", sentinel, sentinel, span of "v1", sentinel, sentinel))) + } + + withClue("unmount, then the value changes while inactive (not observed yet):") { + unmount() + tracker.assertEvents(_.unmounted("v1")).clear() + r.valueVar.set("w") + tracker.assertNoEvents.clear() + } + + withClue("remount: the inserter re-renders first, so the stale span is dropped without mounting:") { + mount(root) + tracker.assertEvents(_.elementCreated("w2"), _.mounted("w2")).clear() + expectNode(div.of(div.of("LIST", sentinel, sentinel, span of "w2", sentinel, sentinel))) + } + } + + it("REFERENCE: a plain ELEMENT moved onto an UNMOUNTED element re-renders before its stale content can mount") { + // The plain-element analogy the moved-group tests below must match: the element's own owner + // keeps its inserter ahead of its content, so mounting it re-renders first and the span + // rendered before the move is discarded without mounting. + val tracker = createEventTracker() + val r = new FreshRenderer(tracker) + val items = Var[List[Inserter]](Nil) + val inner = div("INNER", r.inserter) + val host = div("HOST") // stays detached until the last step + val root = div(div("LIST", children <-- items.signal)) + + mount(root) + + withClue("first placement in the mounted LIST:") { + items.set(List(inner)) + tracker.assertEvents(_.elementCreated("v1"), _.mounted("v1")).clear() + } + + withClue("move onto the detached HOST: the content unmounts exactly once:") { + host.amend(inner) + items.set(Nil) + tracker.assertEvents(_.unmounted("v1")).clear() + } + + withClue("the value changes while HOST is detached (not observed yet):") { + r.valueVar.set("w") + tracker.assertNoEvents.clear() + } + + withClue("mounting HOST: only the re-rendered span mounts:") { + root.amend(host) + tracker.assertEvents(_.elementCreated("w2"), _.mounted("w2")).clear() + expectNode( + div.of( + div.of("LIST", sentinel, sentinel), + div.of("HOST", div.of("INNER", sentinel, span of "w2")) + ) + ) + } + } + + it("a group moved onto an UNMOUNTED element does not mount its stale content when that element mounts") { + // Reference: `unmountedEl.amend(div(child <-- signal.map(render)))`, then mounting `unmountedEl` + // after the value changed, re-renders and mounts ONLY the new span. A moved group must do the same. + val tracker = createEventTracker() + val r = new FreshRenderer(tracker) + val items = Var[List[Inserter]](Nil) + val host = div("HOST") // stays detached until the last step + val root = div(div("LIST", children <-- items.signal)) + + mount(root) + + withClue("first placement in the mounted LIST:") { + items.set(List(r.inserter)) + tracker.assertEvents(_.elementCreated("v1"), _.mounted("v1")).clear() + } + + withClue("demote onto the detached HOST: the span relocates and its content unmounts exactly once:") { + host.amend(r.inserter) + items.set(Nil) // no-op removal: the span already left LIST + tracker.assertEvents(_.unmounted("v1")).clear() + expectNode(div.of(div.of("LIST", sentinel, sentinel))) + expectNode(host.ref, div.of("HOST", sentinel, span of "v1", sentinel)) + } + + withClue("the value changes while HOST is detached (not observed yet):") { + r.valueVar.set("w") + tracker.assertNoEvents.clear() + } + + withClue("mounting HOST: the inserter re-renders first; the stale span v1 must NOT mount, only w2 does:") { + root.amend(host) + tracker.assertEvents(_.elementCreated("w2"), _.mounted("w2")).clear() + expectNode( + div.of( + div.of("LIST", sentinel, sentinel), + div.of("HOST", sentinel, span of "w2", sentinel) + ) + ) + } + + withClue("still live on HOST:") { + r.valueVar.set("x") + tracker.assertEvents(_.elementCreated("x3"), _.mounted("x3"), _.unmounted("w2")).clear() + expectNode( + div.of( + div.of("LIST", sentinel, sentinel), + div.of("HOST", sentinel, span of "x3", sentinel) + ) + ) + } + } + + it("a group with content stolen from an UNMOUNTED host into a mounted list does not mount its stale content") { + // Reference: an element with rendered-then-unmounted content whose value changed meanwhile, + // moved into a mounted list, re-renders on the way in and mounts ONLY the new span. + val tracker = createEventTracker() + val r = new FreshRenderer(tracker) + val items2 = Var[List[Inserter]](Nil) + val root1 = div(div("P", r.inserter)) + val root2 = div(div("L2", children <-- items2.signal)) + + withClue("render in P, then unmount P: the span unmounts exactly once:") { + mount(root1) + tracker.assertEvents(_.elementCreated("v1"), _.mounted("v1")).clear() + unmount() + tracker.assertEvents(_.unmounted("v1")).clear() + expectNode(root1.ref, div.of(div.of("P", sentinel, span of "v1"))) + } + + withClue("the value changes while P is unmounted (not observed yet):") { + r.valueVar.set("w") + tracker.assertNoEvents.clear() + } + + withClue("L2 (mounted) steals the group out of the unmounted P: only the re-rendered span mounts:") { + mount(root2) + tracker.clear() + items2.set(List(r.inserter)) + tracker.assertEvents(_.elementCreated("w2"), _.mounted("w2")).clear() + expectNode(div.of(div.of("L2", sentinel, sentinel, span of "w2", sentinel, sentinel))) + expectNode(root1.ref, div.of(div.of("P"))) + } + } + + it("a group stolen between two MOUNTED lists does not mount its stale content on the new host's later remount") { + // The seamless add-first steal itself is silent (no re-mount). But the new host must ALSO keep + // behaving like the reference on every later unmount / remount cycle: re-render first, never + // mount the stale span. + val tracker = createEventTracker() + val r = new FreshRenderer(tracker) + val items1 = Var[List[Inserter]](Nil) + val items2 = Var[List[Inserter]](Nil) + val root = div( + div("L1", children <-- items1.signal), + div("L2", children <-- items2.signal) + ) + + mount(root) + + withClue("first placement in L1:") { + items1.set(List(r.inserter)) + tracker.assertEvents(_.elementCreated("v1"), _.mounted("v1")).clear() + } + + withClue("add-first steal into L2 is seamless:") { + items2.set(List(r.inserter)) + items1.set(Nil) + tracker.assertNoEvents.clear() + expectNode( + div.of( + div.of("L1", sentinel, sentinel), + div.of("L2", sentinel, sentinel, span of "v1", sentinel, sentinel) + ) + ) + } + + withClue("unmount, then the value changes while inactive (not observed yet):") { + unmount() + tracker.assertEvents(_.unmounted("v1")).clear() + r.valueVar.set("w") + tracker.assertNoEvents.clear() + } + + withClue("remount: as in the REFERENCE, the stale span must NOT mount before the inserter re-renders:") { + mount(root) + tracker.assertEvents(_.elementCreated("w2"), _.mounted("w2")).clear() + expectNode( + div.of( + div.of("L1", sentinel, sentinel), + div.of("L2", sentinel, sentinel, span of "w2", sentinel, sentinel) + ) + ) + } + } +} From cab57a6e307e118e2d541f8ccb62ffce63d356de Mon Sep 17 00:00:00 2001 From: nguyenyou Date: Wed, 23 Sep 2026 10:32:30 +0700 Subject: [PATCH 2/4] Test: reproduce stale content stealing from an unrelated list after a group move When a moved group's stale content mounts ahead of the group's re-render, its own bindings run too. Here the stale element's `child.maybe <--` steals a node from an unrelated live list; the group then drops the stale element, taking that node with it for good. Covers all three move paths, plus depth 2, where the stale content is a nested group. Co-Authored-By: Claude Opus 5.5 --- .../NestedGroupActivationOrderSpec.scala | 237 ++++++++++++++++++ 1 file changed, 237 insertions(+) diff --git a/src/test/scala/com/raquo/laminar/tests/NestedGroupActivationOrderSpec.scala b/src/test/scala/com/raquo/laminar/tests/NestedGroupActivationOrderSpec.scala index a5392ba6..8d901e60 100644 --- a/src/test/scala/com/raquo/laminar/tests/NestedGroupActivationOrderSpec.scala +++ b/src/test/scala/com/raquo/laminar/tests/NestedGroupActivationOrderSpec.scala @@ -231,4 +231,241 @@ class NestedGroupActivationOrderSpec extends UnitSpec { ) } } + + // -- Fixture: stale content whose mount has a side effect on an UNRELATED live list -- + + // While mounted, the group's content `c` claims `e` via its own `child.maybe <--` binding. + // While the group is inactive, `c` leaves the group's source and starts claiming `e`, which + // meanwhile lives in an unrelated live list. If stale `c` ever mounts, its binding steals `e` + // from that list, and when the group then drops `c`, `e` leaves the page with it for good. + + private class StaleClaimer(tracker: EventTracker) { + val e: Div = tracker.createDiv("e") + val claimE: Var[Option[HtmlElement]] = Var(None) + val c: Div = tracker.createDiv("c", child.maybe <-- claimE.signal) + val groupItems: Var[List[HtmlElement]] = Var(List(c)) + val group: Inserter = children <-- groupItems.signal + tracker.clear() + + /** Call while the group is inactive: drop `c` from the group, and make `c` claim `e`. */ + def makeContentStale(): Unit = { + Var.set( + groupItems -> Nil, + claimE -> Some(e) + ) + } + } + + it("REFERENCE: a never-moved group's stale content does not mount, so it can't steal from an unrelated list") { + // Baseline for the tests below: `e` stays in L3, untouched, when the group's host remounts. + val tracker = createEventTracker() + val s = new StaleClaimer(tracker) + val showL1 = Var(true) + val items3 = Var[List[HtmlElement]](Nil) + val l1 = div("L1", s.group) + val l3 = div("L3", children <-- items3.signal) + val root = div(child.maybe <-- showL1.signal.map(if (_) Some(l1) else None), l3) + + withClue("mount: the group renders and mounts `c`:") { + mount(root) + tracker.assertEvents(_.mounted("c")).clear() + } + + withClue("hide L1, make `c` stale while inactive, and put `e` in the unrelated live L3:") { + showL1.set(false) + tracker.assertEvents(_.unmounted("c")).clear() + s.makeContentStale() + tracker.assertNoEvents.clear() + items3.set(List(s.e)) + tracker.assertEvents(_.mounted("e")).clear() + } + + withClue("show L1 again: the group re-renders first, stale `c` never mounts, `e` stays in L3:") { + showL1.set(true) + tracker.assertNoEvents.clear() + expectNode(l1.ref, div.of("L1", sentinel, sentinel)) + expectNode(l3.ref, div.of("L3", sentinel, div of "e", sentinel)) + } + } + + it("a group stolen from an UNMOUNTED host into a mounted list does not let stale content steal from an unrelated list") { + val tracker = createEventTracker() + val s = new StaleClaimer(tracker) + val showL1 = Var(true) + val items1 = Var[List[Inserter]](List(s.group)) + val items2 = Var[List[Inserter]](Nil) + val items3 = Var[List[HtmlElement]](Nil) + val l1 = div("L1", children <-- items1.signal) + val l2 = div("L2", children <-- items2.signal) + val l3 = div("L3", children <-- items3.signal) + val root = div(child.maybe <-- showL1.signal.map(if (_) Some(l1) else None), l2, l3) + + withClue("mount: the group renders and mounts `c` in L1:") { + mount(root) + tracker.assertEvents(_.mounted("c")).clear() + } + + withClue("hide L1, make `c` stale while inactive, and put `e` in the unrelated live L3:") { + showL1.set(false) + tracker.assertEvents(_.unmounted("c")).clear() + s.makeContentStale() + tracker.assertNoEvents.clear() + items3.set(List(s.e)) + tracker.assertEvents(_.mounted("e")).clear() + } + + withClue("mounted L2 steals the group out of hidden L1: stale `c` must not mount and steal `e`:") { + items2.set(List(s.group)) + tracker.assertNoEvents.clear() + expectNode(l2.ref, div.of("L2", sentinel, sentinel, sentinel, sentinel)) + expectNode(l3.ref, div.of("L3", sentinel, div of "e", sentinel)) + } + } + + it("a group moved from an UNMOUNTED host onto a mounted element does not let stale content steal from an unrelated list") { + val tracker = createEventTracker() + val s = new StaleClaimer(tracker) + val showL1 = Var(true) + val items1 = Var[List[Inserter]](List(s.group)) + val items3 = Var[List[HtmlElement]](Nil) + val l1 = div("L1", children <-- items1.signal) + val host = div("HOST") + val l3 = div("L3", children <-- items3.signal) + val root = div(child.maybe <-- showL1.signal.map(if (_) Some(l1) else None), host, l3) + + withClue("mount: the group renders and mounts `c` in L1:") { + mount(root) + tracker.assertEvents(_.mounted("c")).clear() + } + + withClue("hide L1, make `c` stale while inactive, and put `e` in the unrelated live L3:") { + showL1.set(false) + tracker.assertEvents(_.unmounted("c")).clear() + s.makeContentStale() + tracker.assertNoEvents.clear() + items3.set(List(s.e)) + tracker.assertEvents(_.mounted("e")).clear() + } + + withClue("move the group onto mounted HOST: stale `c` must not mount and steal `e`:") { + host.amend(s.group) + tracker.assertNoEvents.clear() + expectNode(host.ref, div.of("HOST", sentinel, sentinel)) + expectNode(l3.ref, div.of("L3", sentinel, div of "e", sentinel)) + } + } + + it("a group moved onto an UNMOUNTED element does not let stale content steal from an unrelated list when that element mounts") { + val tracker = createEventTracker() + val s = new StaleClaimer(tracker) + val showL1 = Var(true) + val items1 = Var[List[Inserter]](List(s.group)) + val items3 = Var[List[HtmlElement]](Nil) + val l1 = div("L1", children <-- items1.signal) + val host = div("HOST") // stays detached until the last step + val l3 = div("L3", children <-- items3.signal) + val root = div(child.maybe <-- showL1.signal.map(if (_) Some(l1) else None), l3) + + withClue("mount: the group renders and mounts `c` in L1:") { + mount(root) + tracker.assertEvents(_.mounted("c")).clear() + } + + withClue("hide L1, make `c` stale while inactive, and put `e` in the unrelated live L3:") { + showL1.set(false) + tracker.assertEvents(_.unmounted("c")).clear() + s.makeContentStale() + tracker.assertNoEvents.clear() + items3.set(List(s.e)) + tracker.assertEvents(_.mounted("e")).clear() + } + + withClue("move the group onto the detached HOST (inactive to inactive):") { + host.amend(s.group) + tracker.assertNoEvents.clear() + } + + withClue("mount HOST: stale `c` must not mount and steal `e`:") { + root.amend(host) + tracker.assertNoEvents.clear() + expectNode(host.ref, div.of("HOST", sentinel, sentinel)) + expectNode(l3.ref, div.of("L3", sentinel, div of "e", sentinel)) + } + } + + // -- Depth 2: the stale content is itself a nested group, not an element -- + + // A fix must cover every nesting depth: moving the outer group also moves (and re-registers) + // the inner group, so the inner group must not run ahead of the outer group's re-render either. + + it("REFERENCE depth-2: a never-moved group's stale NESTED group does not activate, so it can't steal from an unrelated list") { + val tracker = createEventTracker() + val e = tracker.createDiv("e") + tracker.clear() + val claimE = Var[Option[HtmlElement]](None) + val inner: Inserter = child.maybe <-- claimE.signal // claims `e` while active + val outerItems = Var[List[Inserter]](List(inner)) + val outer: Inserter = children <-- outerItems.signal + val showL1 = Var(true) + val items3 = Var[List[HtmlElement]](Nil) + val l1 = div("L1", outer) + val l3 = div("L3", children <-- items3.signal) + val root = div(child.maybe <-- showL1.signal.map(if (_) Some(l1) else None), l3) + + withClue("mount, hide L1, make `inner` stale while inactive, and put `e` in the unrelated live L3:") { + mount(root) + showL1.set(false) + Var.set( + outerItems -> Nil, + claimE -> Some(e) + ) + tracker.assertNoEvents.clear() + items3.set(List(e)) + tracker.assertEvents(_.mounted("e")).clear() + } + + withClue("show L1 again: the outer group re-renders first, stale `inner` never activates:") { + showL1.set(true) + tracker.assertNoEvents.clear() + expectNode(l1.ref, div.of("L1", sentinel, sentinel)) + expectNode(l3.ref, div.of("L3", sentinel, div of "e", sentinel)) + } + } + + it("depth-2: a group stolen from an UNMOUNTED host into a mounted list does not let a stale NESTED group steal from an unrelated list") { + val tracker = createEventTracker() + val e = tracker.createDiv("e") + tracker.clear() + val claimE = Var[Option[HtmlElement]](None) + val inner: Inserter = child.maybe <-- claimE.signal // claims `e` while active + val outerItems = Var[List[Inserter]](List(inner)) + val outer: Inserter = children <-- outerItems.signal + val showL1 = Var(true) + val items1 = Var[List[Inserter]](List(outer)) + val items2 = Var[List[Inserter]](Nil) + val items3 = Var[List[HtmlElement]](Nil) + val l1 = div("L1", children <-- items1.signal) + val l2 = div("L2", children <-- items2.signal) + val l3 = div("L3", children <-- items3.signal) + val root = div(child.maybe <-- showL1.signal.map(if (_) Some(l1) else None), l2, l3) + + withClue("mount, hide L1, make `inner` stale while inactive, and put `e` in the unrelated live L3:") { + mount(root) + showL1.set(false) + Var.set( + outerItems -> Nil, + claimE -> Some(e) + ) + tracker.assertNoEvents.clear() + items3.set(List(e)) + tracker.assertEvents(_.mounted("e")).clear() + } + + withClue("mounted L2 steals the outer group out of hidden L1: stale `inner` must not activate and steal `e`:") { + items2.set(List(outer)) + tracker.assertNoEvents.clear() + expectNode(l2.ref, div.of("L2", sentinel, sentinel, sentinel, sentinel)) + expectNode(l3.ref, div.of("L3", sentinel, div of "e", sentinel)) + } + } } From 210e5e843da29d921486e19f6bea9c72727a27e4 Mon Sep 17 00:00:00 2001 From: nguyenyou Date: Wed, 23 Sep 2026 14:28:39 +0700 Subject: [PATCH 3/4] Test: reproduce reclaimed nested groups affecting neighboring items Add two reference tests and three reproducers for same-parent group reclamation after removal and plain placement. Co-Authored-By: Codex GPT-6 Astra --- .../tests/NestedGroupReStealSpec.scala | 191 ++++++++++++++++++ 1 file changed, 191 insertions(+) create mode 100644 src/test/scala/com/raquo/laminar/tests/NestedGroupReStealSpec.scala diff --git a/src/test/scala/com/raquo/laminar/tests/NestedGroupReStealSpec.scala b/src/test/scala/com/raquo/laminar/tests/NestedGroupReStealSpec.scala new file mode 100644 index 00000000..33da08c8 --- /dev/null +++ b/src/test/scala/com/raquo/laminar/tests/NestedGroupReStealSpec.scala @@ -0,0 +1,191 @@ +package com.raquo.laminar.tests + +import com.raquo.laminar.api.L._ +import com.raquo.laminar.inserters.Inserter +import com.raquo.laminar.utils.{EventTracker, UnitSpec} + +/** A dynamic item that a `children <--` list steals back from a plain placement under the SAME + * parent must end up bracketed by its own sentinels, like any other list item, so that its span + * never extends over the list's neighbouring items. + */ +class NestedGroupReStealSpec extends UnitSpec { + + // -- Fixture: L1 and L2 are two `children <--` lists under ONE parent -- + + private class TwoLists { + val items1: Var[List[Inserter]] = Var(Nil) + val items2: Var[List[Inserter]] = Var(Nil) + val parent: Div = div(children <-- items1.signal, children <-- items2.signal) + } + + /** Gets `dyn` into L1 by re-stealing it from a plain placement under L1's own parent. + * + * L2 steals `dyn` and drops it, tearing its group down while L1 still tracks it. Then + * `parent.amend(dyn)` rebuilds the group plainly (without a trailing sentinel), and L1 + * steals it back by re-emitting it, which takes the same-parent branch of + * `DynamicInserter.moveWithinDynamicList`. + * + * `dyn` must render `c` on activation. + */ + private def reStealIntoL1(lists: TwoLists, dyn: Inserter, tracker: EventTracker): Unit = { + withClue("detour: L1 places dyn, L2 steals and drops it, the parent re-applies it:") { + lists.items1.set(List(dyn)) + lists.items2.set(List(dyn)) + lists.items2.set(Nil) + lists.parent.amend(dyn) + tracker.assertEvents( + _.mounted("c"), // L1 places dyn + _.unmounted("c"), // L2 tears dyn's group down + _.mounted("c") // the parent rebuilds dyn's group + ).clear() + } + + withClue("L1 re-emits dyn, stealing it back without re-mounting its content:") { + lists.items1.set(List(dyn)) + tracker.assertNoEvents.clear() + } + } + + it("a group re-stolen from a plain placement under the same parent is bracketed in its new list") { + val tracker = createEventTracker() + val c = tracker.createSpan("c") + tracker.clear() + val lists = new TwoLists + val dyn: Inserter = child <-- Val(c) + + mount(lists.parent) + reStealIntoL1(lists, dyn, tracker) + + withClue("L1 now holds dyn's span, closed by its own trailing sentinel:") { + expectNode( + div.of( + sentinel, sentinel, span of "c", sentinel, sentinel, // L1: [dyn: [c]] + sentinel, sentinel // L2: [] + ) + ) + } + } + + it("REFERENCE: a list's item placed right after a group stays with the list when the group re-renders") { + val tracker = createEventTracker() + val c = tracker.createSpan("c") + val c2 = tracker.createSpan("c2") + tracker.clear() + val lists = new TwoLists + val childVar = Var[HtmlElement](c) + val dyn: Inserter = child <-- childVar.signal + + mount(lists.parent) + + withClue("L1 places dyn directly:") { + lists.items1.set(List(dyn)) + tracker.assertEvents(_.mounted("c")).clear() + } + + withClue("L1 takes c as its own item right after dyn (last write wins):") { + lists.items1.set(List(dyn, c)) + tracker.assertNoEvents.clear() + expectNode( + div.of( + sentinel, sentinel, sentinel, span of "c", sentinel, // L1: [dyn: [], c] + sentinel, sentinel // L2: [] + ) + ) + } + + withClue("dyn renders c2; L1's c stays:") { + childVar.set(c2) + tracker.assertEvents(_.mounted("c2")).clear() + expectNode( + div.of( + sentinel, sentinel, span of "c2", sentinel, span of "c", sentinel, // L1: [dyn: [c2], c] + sentinel, sentinel // L2: [] + ) + ) + } + } + + it("a list's item placed right after a re-stolen group stays with the list when the group re-renders") { + val tracker = createEventTracker() + val c = tracker.createSpan("c") + val c2 = tracker.createSpan("c2") + tracker.clear() + val lists = new TwoLists + val childVar = Var[HtmlElement](c) + val dyn: Inserter = child <-- childVar.signal + + mount(lists.parent) + reStealIntoL1(lists, dyn, tracker) + + withClue("L1 takes c as its own item right after dyn (last write wins):") { + lists.items1.set(List(dyn, c)) + tracker.assertNoEvents.clear() + } + + withClue("dyn renders c2; L1's c stays, as in the REFERENCE:") { + childVar.set(c2) + tracker.assertEvents(_.mounted("c2")).clear() + expectNode( + div.of( + sentinel, sentinel, span of "c2", sentinel, span of "c", sentinel, // L1: [dyn: [c2], c] + sentinel, sentinel // L2: [] + ) + ) + } + } + + it("REFERENCE: moving a group to another list leaves behind a node that its list took from it") { + val tracker = createEventTracker() + val c = tracker.createSpan("c") + tracker.clear() + val lists = new TwoLists + val dyn: Inserter = child <-- Val(c) + + mount(lists.parent) + + withClue("L1 places dyn directly, then takes c from it as its own item:") { + lists.items1.set(List(dyn)) + tracker.assertEvents(_.mounted("c")).clear() + lists.items1.set(List(dyn, c)) + tracker.assertNoEvents.clear() + } + + withClue("L2 takes dyn (now empty); c stays in L1:") { + lists.items2.set(List(dyn)) + tracker.assertNoEvents.clear() + expectNode( + div.of( + sentinel, span of "c", sentinel, // L1: [c] + sentinel, sentinel, sentinel, sentinel // L2: [dyn: []] + ) + ) + } + } + + it("moving a re-stolen group to another list leaves behind a node that its list took from it") { + val tracker = createEventTracker() + val c = tracker.createSpan("c") + tracker.clear() + val lists = new TwoLists + val dyn: Inserter = child <-- Val(c) + + mount(lists.parent) + reStealIntoL1(lists, dyn, tracker) + + withClue("L1 takes c from dyn as its own item:") { + lists.items1.set(List(dyn, c)) + tracker.assertNoEvents.clear() + } + + withClue("L2 takes dyn (now empty); c stays in L1, as in the REFERENCE:") { + lists.items2.set(List(dyn)) + tracker.assertNoEvents.clear() + expectNode( + div.of( + sentinel, span of "c", sentinel, // L1: [c] + sentinel, sentinel, sentinel, sentinel // L2: [dyn: []] + ) + ) + } + } +} From be54d9c19c98c8770a39a5f1757178a8721fcaef Mon Sep 17 00:00:00 2001 From: Nikita Gazarov Date: Thu, 24 Sep 2026 00:59:57 -0700 Subject: [PATCH 4/4] Fix: Simplify NestedGroup moving; fix moving and slotting edge cases. #210, #214 --- notes/Inserters.md | 42 +++++-- .../laminar/inserters/ChildrenInserter.scala | 13 +-- .../raquo/laminar/inserters/Inserter.scala | 107 ++++-------------- .../raquo/laminar/inserters/NestedGroup.scala | 107 +++++++++++++----- .../laminar/tests/InserterMoveSpec.scala | 18 +-- .../NestedGroupActivationOrderSpec.scala | 7 +- .../tests/NestedGroupReStealSpec.scala | 2 +- .../laminar/tests/NestedInsertersSpec.scala | 2 +- .../tests/SlotAttributeStealingSpec.scala | 81 ++++++++++++- .../tests/SlotRetainedContentSpec.scala | 2 +- .../com/raquo/laminar/tests/SlotSpec.scala | 2 +- 11 files changed, 229 insertions(+), 154 deletions(-) diff --git a/notes/Inserters.md b/notes/Inserters.md index 7719ee88..90b1c995 100644 --- a/notes/Inserters.md +++ b/notes/Inserters.md @@ -24,7 +24,7 @@ Everything reachable through the `com.raquo.laminar.inserters` package: `child < ## 1. The north star: the plain-element analogy -The single most important principle, cited throughout the code (e.g. `NestedGroup.moveToParent` scaladoc: _"mirroring how a plain element can be moved between two parents"_): +The single most important principle, cited throughout the code (e.g. `NestedGroup.moveTo` scaladoc: _"mirroring how a plain element can be moved"_): > **A dynamic inserter should behave like a plain element wherever possible.** @@ -32,14 +32,14 @@ A plain Laminar element (`val el = div(...)`) can be referenced by multiple pare Concretely, the analogy dictates: -- **Identity is stable.** The same inserter `val` placed in a new location is _moved_, not rebuilt. (`DynamicInserter.apply` / `addToDynamicList` detect an already-placed group and call `moveToParent` instead of constructing.) +- **Identity is stable.** The same inserter `val` placed in a new location is _moved_, not rebuilt. (`DynamicInserter.apply` / `addToDynamicList` detect an already-placed group and call `NestedGroup.moveTo` instead of constructing.) - **A move carries current content, and only current content.** Moving a `div` relocates the children it currently has — it does not reclaim children that were previously removed or stolen from it. Moving a group relocates exactly the nodes currently in its span. See §5. - **A seamless move does not re-mount.** When an element/group always has an active parent throughout the transition, its subscriptions/owner are _transferred_, not torn down and rebuilt, so no unmount/mount fires. See §6. - **Last write wins for a contested node.** If two hosts both want the same node, whichever acts last owns it — exactly as re-parenting a plain element to B removes it from A. See §4. Two operations must not be conflated (they answer different questions): -1. **Moving a group** (`moveToParent`) — triggered by a re-emission whose payload _is_ the group (a list re-emitting `[G]`, or `G` applied to an element). It relocates the group's span. It says nothing about the group's _inner_ membership. +1. **Moving a group** (`moveTo`) — triggered by a re-emission whose payload _is_ the group (a list re-emitting `[G]`, or `G` applied to an element). It relocates the group's span. It says nothing about the group's _inner_ membership. 2. **Stealing / re-stealing a node** — governed by whichever list re-emits _that node_; last write wins. Blurring these is a recurring source of bugs (see §5). @@ -78,7 +78,7 @@ Therefore any operation that must act on the _current_ span (relocate it, re-slo The two correct DOM-walk helpers are `InsertContext.removeContentMapNodesFromDom` (the destructive walk) and `currentContentInsertersFromDom` (its read-only twin). Any code that iterates `contentMap.forEach` to touch _live_ content is suspect (see §11). ### NestedGroup -The rendering vehicle for a `DynamicInserter` — used both when it is applied plainly and when it is a `children <--` item, to keep one consistent code path (state + subscription management in one place, and moveability between arbitrary contexts). It owns the leading sentinel, the `InsertContext`, a `DynamicOwner` + `TransferableSubscription` pair (the "pilot" that mounts/unmounts the inner inserter with the group), and implements `moveToParent` / `removeFromParent`. +The rendering vehicle for a `DynamicInserter` — used both when it is applied plainly and when it is a `children <--` item, to keep one consistent code path (state + subscription management in one place, and moveability between arbitrary contexts). It owns the leading sentinel, the `InsertContext`, a `DynamicOwner` + `TransferableSubscription` pair (the "pilot" that mounts/unmounts the inner inserter with the group), and implements `moveTo` / `removeFromParent`. ### Stealing When node/group X is tracked by inserter A but gets placed by inserter B, we say B _stole_ X from A. Steals happen because the observables feeding A and B propagate in some order, and the add (into B) can be processed before the remove (from A). Laminar's contract: **inserter code must never fail on the resulting stale state, and must self-correct on the next emission** (`InsertContext` scaladoc, lines 31–51). `removeFromDynamicList` is a no-op on parent mismatch precisely so a stale removal after a steal does nothing (`DynamicInserter.removeFromDynamicList`, `DomApi.removeChild` no-op on parent mismatch). @@ -128,10 +128,16 @@ Last-write-wins must survive a group **move**: relocating a group leaves a sibli Four distinct relocation scenarios, all of which should be **seamless (no re-mount)** because the moved thing always has an active parent throughout: -1. **Reorder within one list** — `moveWithinDynamicList`, same-parent branch: a raw DOM reposition of the span, then a slot re-affirm. No owner transfer needed. -2. **Transfer between two lists** (add-first steal) — `addToDynamicList` sees an already-placed group and calls `moveToParent`: relocate the span, transfer the pilot subscription to the new parent's owner, update `currentParentNode` + `currentSlotName` so future emissions target the new home. -3. **Promote** a plainly-applied dynamic inserter INTO a `children <--` list, and **demote** a list item back onto a plain element (`element.amend(inserter)`) — both routed through `moveToParent` / `apply`. Seamless. The trailing sentinel is added on promote and stickily retained on demote (§2). -4. **Steal-back / re-steal** — a group stolen into a sibling, then re-emitted by its original list. Same-parent layout takes `moveWithinDynamicList`'s raw-reposition branch; cross-parent takes `moveToParent`. +1. **Reorder within one list**. +2. **Transfer between two lists** (add-first steal) — `addToDynamicList` sees an already-placed group. +3. **Promote** a plainly-applied dynamic inserter INTO a `children <--` list, and **demote** a list item back onto a plain element (`element.amend(inserter)`, via `apply`). The trailing sentinel is added on promote and stickily retained on demote (§2). +4. **Steal-back / re-steal** — a group stolen into a sibling (or applied plainly to the list's parent), then re-emitted by its original list. + +In all four, the list (or `apply`) calls `addToDynamicList` / `apply` on an already-placed inserter, just like it would to add a new one – the same way `setParent` both adds and moves a plain element. For a dynamic inserter, all four go through one entry point, `NestedGroup.moveTo`, so that the group always ends up in the same shape and lifecycle order as if it was created at its new place, regardless of how it was placed before: + +- **Shape first.** If the destination is a `children <--` list, `moveTo` ensures the trailing sentinel before moving. Without it, the list would treat the group's content as its own neighbouring items, and the group would treat the list's next items as its own content (`NestedGroupReStealSpec`). +- **Same parent element** → a raw DOM reposition of the span, then a slot re-affirm on its live content (§9). The group and its content keep their Laminar parent and dynamic owner, so there is no lifecycle to update. +- **Different parent element** → `moveToParent`: transfer the pilot subscription, relocate the span, update `currentParentNode` + `currentSlotName` so future emissions target the new home. ### The governing rule for `moveToParent` @@ -141,13 +147,24 @@ This follows directly from the plain-element analogy: a moved `div` takes the ch - **DOM order, not map order.** `children.command <--` builds its DOM out of insertion order, so the map lists nodes differently than the DOM. The move must preserve DOM order (`InserterMoveSpec` "a stolen `children.command <--` span preserves its DOM order"). - **Live membership, not map membership.** A node stolen out of the span by a sibling, or absorbed into the group's own nested `child <--`, has a stale map entry that the move must NOT drag back (the ④/⑤ bug class). -- **Nested spans move as a unit.** The DOM walk jumps by each inserter's `lastNode.nextSibling`, so an inner group (with its own sentinels) is stepped over whole and relocated by its own recursive `moveToParent` (handles depth-2/3, ⑤). +- **Nested spans move as a unit.** The DOM walk jumps by each inserter's `lastNode.nextSibling`, so an inner group (with its own sentinels) is stepped over whole and relocated by its own recursive `moveTo` (handles depth-2/3, ⑤). + +Accordingly, `moveToParent` **snapshots the live span (via `currentContentInsertersFromDom`) BEFORE moving the sentinels** (the walk starts at the leading sentinel, which is about to move), then re-adds each collected inserter in DOM order, then commits the new parent/slot. The move deliberately does **not** touch `contentMap` — stale inner tracking is tolerated and self-corrects on the inner inserter's next emission, matching existing behaviour. + +### Lifecycle first: the pilot transfer precedes the content + +A group has no element of its own: its content's pilot subscriptions are owned directly by the parent element's `DynamicOwner`, alongside the group's own pilot. The group's logical ownership of its content is encoded only by **registration order** in that shared owner: the group's pilot must come first, so that on activation, the inner inserter updates its content _before_ that content mounts. This is what a plain `div(child <-- signal.map(render))` gets for free, and what a never-moved group gets by construction (its content only arrives once it activates). + +So `moveToParent` transfers the pilot subscription **before** snapshotting and moving the content: -Accordingly, `moveToParent` **snapshots the live span (via `currentContentInsertersFromDom`) BEFORE moving the sentinels** (the walk starts at the leading sentinel, which is about to move), then re-adds each collected inserter in DOM order, then commits the new parent/slot, then transfers the subscription. The move deliberately does **not** touch `contentMap` — stale inner tracking is tolerated and self-corrects on the inner inserter's next emission, matching existing behaviour. +- **Inactive → active** (e.g. stolen out of an unmounted host into a mounted list): the inner inserter re-renders while still in the old, inactive parent, so stale content is dropped without ever mounting, and only fresh content is moved (and mounted). This matters beyond wasted events: stale content that mounts runs its own bindings, which can steal nodes from unrelated live lists and take them down with it when it's dropped (`NestedGroupActivationOrderSpec`). +- **Active → inactive**: the inner inserter stops before its content unmounts, mirroring `removeFromParent`. +- **Active → active**: a live transfer, seamless as before. +- **Any move**: the group's pilot re-registers in the new owner ahead of its content's, so later unmount / remount cycles of the new host behave like a never-moved group. This recurses: a nested group is moved by its own `moveTo`, after its outer group has already re-rendered and dropped it if it was stale. ### Re-placing an inserter whose group was already torn down -If the previous host genuinely _removed_ the group (set `nestedGroupOpt = js.undefined`) and then the original list re-emits it, there is no group to move — it must be **placed afresh** (re-inserted + re-mounted), like a plain element that was removed and re-added. `DynamicInserter.moveWithinDynamicList` detects the missing group and falls back to `addToDynamicList`; the list's item count already counted this inserter (it was in the previous map), so the rebuild changes no count. This is the counterpart to "move the live span": when there is no live span, rebuild. Covered by `InserterMoveSpec` section 4d. +If the previous host genuinely _removed_ the group (set `nestedGroupOpt = js.undefined`) and then the original list re-emits it, there is no group to move — it must be **placed afresh** (re-inserted + re-mounted), like a plain element that was removed and re-added. `addToDynamicList` builds a new group when there is none; the list's item count already counted this inserter (it was in the previous map), so the rebuild changes no count. This is the counterpart to "move the live span": when there is no live span, rebuild. Covered by `InserterMoveSpec` section 4d. --- @@ -230,7 +247,8 @@ Distinct from the last-write-wins contest above: when a slot could come from an ### Persistence and live-span re-slotting - A dynamic inserter's slot is **persistent**, not just applied to current content: it is stored on the context (`currentSlotName`) so future emissions are slotted the same way. A `NestedGroup` move resolves the destination list's slot against its own and stores the result, redirecting the inner inserter's future emissions. -- Re-slotting on a move/diff reads the **live DOM span**, never `contentMap` — so a node that has left the span is never re-slotted out from under its new host. `NestedGroup.applySlot` iterates `currentContentInsertersFromDom` and skips departed nodes; `updateChildren`'s same-place branch also re-affirms slot, because a different wrapper may now slot the same node. Covered by `SlotSpec` "re-slotting a moved group re-slots only its live span, leaving a sibling-stolen node's slot with its new host" (a re-steal between two sibling `Slot`s hits `NestedGroup.applySlot` via `moveWithinDynamicList`'s same-parent branch). +- Re-slotting on a move/diff reads the **live DOM span**, never `contentMap` — so a node that has left the span is never re-slotted out from under its new host. `NestedGroup.applySlot` iterates `currentContentInsertersFromDom` and skips departed nodes; `updateChildren`'s same-place branch also re-affirms slot, because a different wrapper may now slot the same node. Covered by `SlotSpec` "re-slotting a moved group re-slots only its live span, leaving a sibling-stolen node's slot with its new host" (a re-steal between two sibling `Slot`s hits `NestedGroup.applySlot` via `NestedGroup.moveTo`'s same-parent branch). +- **A group's content is re-affirmed like a plain item.** Whenever a list places, re-places, or reconciles in place a dynamic inserter item (and whenever such a group is moved, within or across parents), `NestedGroup.applySlot` re-affirms the effective slot on every node of the group's live span, even if the slot name didn't change. So the last-write-wins contest above behaves the same whether an element is a direct list item or sits inside a nested group, and regardless of which move path was taken. There is deliberately no "slot name unchanged" shortcut: it would let a manual `slot :=` on a group's content survive a reconcile that restores it on a plain sibling. Protecting stolen nodes is the live-span walk's job, not the shortcut's. Pinned by `SlotAttributeStealingSpec` "a slotted-list reconcile re-asserts the Slot's slot over a manual override on a nested group's content". --- diff --git a/src/main/scala/com/raquo/laminar/inserters/ChildrenInserter.scala b/src/main/scala/com/raquo/laminar/inserters/ChildrenInserter.scala index eb375811..df72ed42 100644 --- a/src/main/scala/com/raquo/laminar/inserters/ChildrenInserter.scala +++ b/src/main/scala/com/raquo/laminar/inserters/ChildrenInserter.scala @@ -120,15 +120,12 @@ object ChildrenInserter { if (index >= currentItemCount) { // Overflow – we've consumed all previous items: - // Just insert nextInserter at the cursor (or move it there if this inserter it was previously in the list) - if (foundInserterInPrevMap) { - // @Note: DOM update - nextInserter.moveWithinDynamicList(listParentNode, afterRef, slotName) - } else { + // Just insert nextInserter at the cursor + if (!foundInserterInPrevMap) { currentItemCount += 1 - // @Note: DOM update - nextInserter.addToDynamicList(listParentNode, afterRef, slotName) } + // @Note: DOM update + nextInserter.addToDynamicList(listParentNode, afterRef, slotName) } else { if (foundInserterInPrevMap) { if (nextInserter.stableFirstNode == prevItemRef) { @@ -158,7 +155,7 @@ object ChildrenInserter { } else { // Still not in place – this is a MOVE, so we do NOT change the count. // @Note: DOM update - nextInserter.moveWithinDynamicList(listParentNode, afterRef, slotName) + nextInserter.addToDynamicList(listParentNode, afterRef, slotName) } } } else { diff --git a/src/main/scala/com/raquo/laminar/inserters/Inserter.scala b/src/main/scala/com/raquo/laminar/inserters/Inserter.scala index 88892b49..8a740981 100644 --- a/src/main/scala/com/raquo/laminar/inserters/Inserter.scala +++ b/src/main/scala/com/raquo/laminar/inserters/Inserter.scala @@ -2,7 +2,6 @@ package com.raquo.laminar.inserters import com.raquo.airstream.ownership.{Owner, Subscription} import com.raquo.ew -import com.raquo.laminar.domapi.DomApi import com.raquo.laminar.modifiers.Modifier import com.raquo.laminar.nodes.{ChildNode, CommentNode, ParentNode, ReactiveElement} import org.scalajs.dom @@ -52,6 +51,10 @@ trait DiffableInserter extends Inserter { * its nodes: This inserter's nodes are added to parent, their subscriptions * are set up, etc. * + * Like `setParent`, if this inserter is already placed (in this list, or elsewhere), + * this moves it to the new position without re-mounting. Either way, this re-affirms + * the list's slot on its content. + * * Should be paired with [[removeFromDynamicList]]. * * @param afterRef the raw DOM node under `parent` after which this inserter's nodes @@ -80,26 +83,6 @@ trait DiffableInserter extends Inserter { keepNestedItem: dom.Node => Boolean ): Unit - /** No mount / re-mount, just a lateral move. - * - * Note: Note: [[DynamicInserter]] overrides this with a special implementation. - * - * Pre-requisite: you must have called [[addToDynamicList]] - * with the same parent before calling this. - */ - private[laminar] def moveWithinDynamicList( - parent: ReactiveElement.Base, - afterRef: dom.Node, - listSlotName: String | Unit - ): Unit = { - // Re-affirm the list's slot: this is a same-list reorder, so the item stays in its slot. - addToDynamicList( - parent = parent, - afterRef = afterRef, - listSlotName = listSlotName - ) - } - /** Applies to the element(s) currently in this inserter. * * DynamicInserter also records the name on its context, so elements it inserts @@ -227,10 +210,11 @@ class DynamicInserter( // This inserter instance already lives somewhere (applied to another element, or as a // `children <--` list item). Applying it here (e.g. `element.amend(inserter)`) MOVES it // seamlessly (no re-mounting). - group.moveToParent( + group.moveTo( newParent = element, afterRefOpt = afterRefOpt, - newSlotName = slotName // as-is, because a plain `element` parent adds no slot + newSlotName = slotName, // as-is, because a plain `element` parent adds no slot + insertAsListItem = false ) } } @@ -270,7 +254,9 @@ class DynamicInserter( val newSlotName = slotName.orElse(listSlotName) nestedGroupOpt.fold( ifEmpty = { - // First placement of this inserter + // First placement of this inserter, or re-placement after its group was torn down + // (e.g. a list re-emits this inserter after another list stole it and removed it). + // Either way, there is no content to move, so render and mount it afresh. nestedGroupOpt = new NestedGroup( sentinelNode = sentinelNode, insertFn = insertFn @@ -282,18 +268,19 @@ class DynamicInserter( ) } ) { group => - // This inserter instance already lives as a group somewhere else. + // This inserter instance already lives as a group: elsewhere in this list, in another + // list (possibly under the same parent), or applied to a plain element. // Add trailing sentinel for proper tracking inside `children <--`, // then move it seamlessly to its new location. - // Note: The move takes the group's span out of the previous list's walked - // region, so when that list later reconciles it never revisits this - // inserter – it does NOT call `removeFromDynamicList` on it. This new - // list manages the inserter now. - group.ensureTrailingSentinel() - group.moveToParent( + // Note: A move from another place takes the group's span out of that place's walked + // region, so when the previous list later reconciles it never revisits this + // inserter – it does NOT call `removeFromDynamicList` on it. This list manages + // the inserter now. + group.moveTo( newParent = parent, afterRefOpt = afterRef, - newSlotName = newSlotName + newSlotName = newSlotName, + insertAsListItem = true // ensures trailig sentinel ) } } @@ -328,60 +315,4 @@ class DynamicInserter( } } - /** Note: overrides default implementation */ - override private[laminar] def moveWithinDynamicList( - parent: ReactiveElement.Base, - afterRef: dom.Node, - listSlotName: String | Unit - ): Unit = { - nestedGroupOpt.fold( - ifEmpty = { - // This inserter was previously stolen, then the thief removed this inserter, - // and now the list that originally tracked this inserter in its `contentMap` - // is re-emitting it now again. - // The nodes of this inserter were removed from the DOM, so re-insert + re-mount. - // Note: The list's item count already accounted for this inserter (we were in - // its previous contentMap), so this move does not affect `currentItemCount`. - addToDynamicList(parent, afterRef, listSlotName) - } - ) { group => - if (group.leadingSentinel.ref.parentNode != parent.ref) { - // Cross-parent re-steal: another list stole this group, and now the - // previous list re-emits with this inserter again, and steals it back. - // We need a full `moveToParent` here to bring it back, - // transfer subscription ownership, and update the slot. - group.ensureTrailingSentinel() - group.moveToParent( - newParent = parent, - afterRefOpt = afterRef, - newSlotName = slotName.orElse(listSlotName) // innermost `Slot` wins - ) - } else { - // Same DOM parent. Reachable in two cases: - // 1. Reordering within the same dynamic list - // 2. Stealing back an item that was previously stolen into a sibling parent inserter - // (e.g. two `children <--`, possibly in different `Slot`s, under one element) - val lastRef = lastNode // trailing sentinel - var node = stableFirstNode // leading sentinel - var reference = afterRef - var continue = true - while (continue) { - val nextNode = node.nextSibling // capture before `insertAfter` moves `node` away - continue = node ne lastRef - // #Note: raw move – bypasses willSetMount / setMount / slot reconcile - // – slot handled explicitly just below. - DomApi.raw.insertAfter( - parent = parent.ref, - newChild = node, - referenceChild = reference - ) - reference = node - node = nextNode - } - // In case #2 above, we do need to reaffirm the slot. - // In case #1, this is a no-op. - applySlot(parent, listSlotName) - } - } - } } diff --git a/src/main/scala/com/raquo/laminar/inserters/NestedGroup.scala b/src/main/scala/com/raquo/laminar/inserters/NestedGroup.scala index 064a4a78..9d78d9e7 100644 --- a/src/main/scala/com/raquo/laminar/inserters/NestedGroup.scala +++ b/src/main/scala/com/raquo/laminar/inserters/NestedGroup.scala @@ -92,26 +92,86 @@ final class NestedGroup( nestedInsertContext.forceTrailingSentinel() } - /** Move this whole group (its content AND its lifecycle ownership) to a new parent, - * right after `afterRefOpt` (or appended at the end of `newParent` if `afterRefOpt` - * is absent) WITHOUT re-mounting. + /** Place this already-rendered group right after `afterRefOpt` in `newParent` (or append + * it, if `afterRefOpt` is absent) WITHOUT re-mounting, mirroring how a plain element can + * be moved – within its parent, or to a different parent. + * + * This is the single entry point for every re-placement of an existing group. * - * Seamlessly transfers its contents and subscriptions to the new parent, mirroring - * how a plain element can be moved between two parents. + * Whatever the group's previous placement, it ends up in the shape and lifecycle order + * that it would have had if it was created at its new place. * - * Called when the SAME dynamic inserter instance, already placed as a group, - * is placed somewhere else: - * - added to another `children <--` list (see [[DynamicInserter.addToDynamicList]]), or - * - applied to a plain element, e.g. `element.amend(inserter)` (see [[DynamicInserter.apply]]). + * @param newSlotName already resolved against the group's own slot (innermost wins) + * @param insertAsListItem true if the new place is a `children <--` list + */ + private[laminar] def moveTo( + newParent: ReactiveElement.Base, + afterRefOpt: dom.Node | Unit, + newSlotName: String | Unit, + insertAsListItem: Boolean + ): Unit = { + if (insertAsListItem) { + ensureTrailingSentinel() + } + if (newParent eq nestedInsertContext.currentParentNode) { + moveWithinParent(afterRefOpt, newSlotName) + } else { + moveToParent(newParent, afterRefOpt, newSlotName) + } + } + + /** Reposition this group's span within its current parent. The Laminar parent (and so, + * the dynamic owner) of the group and its content does not change, so this is a raw DOM + * move: there is no lifecycle to update. + */ + private def moveWithinParent( + afterRefOpt: dom.Node | Unit, + newSlotName: String | Unit + ): Unit = { + val parentRef = nestedInsertContext.currentParentNode.ref + // Our leading sentinel is in this parent, so `lastChild` is never null. + var reference = afterRefOpt.getOrElse(parentRef.lastChild) + val lastRef = lastNode + var node: dom.Node = leadingSentinel.ref + var continue = true + while (continue) { + val nextNode = node.nextSibling // capture before `insertAfter` moves `node` away + continue = node ne lastRef + DomApi.raw.insertAfter( + parent = parentRef, + newChild = node, + referenceChild = reference + ) + reference = node + node = nextNode + } + // Re-affirm slot, following last-write-wins principle, same as `moveToParent`. + applySlot(newSlotName) + } + + /** Move this whole group (its content AND its lifecycle ownership) to a new parent, + * seamlessly transferring its contents and subscriptions. */ - private[laminar] def moveToParent( + private def moveToParent( newParent: ReactiveElement.Base, afterRefOpt: dom.Node | Unit, newSlotName: String | Unit ): Unit = { + // Transfer lifecycle ownership BEFORE moving the content: + // - Within the new parent's dynamic owner, this group's pilot subscription must precede + // those of its content (as it does when the group is created there), so that + // on activation, the inner inserter updates its content before that content mounts. + // - If this activates the group, the inner inserter updates its content while it's still + // in the old (inactive) parent, so no stale content ever mounts in the new parent. + // - If this deactivates the group, the inner inserter stops before its content unmounts, + // mirroring `removeFromParent`. + // Live transfers (active to active) are seamless, without re-mounting. + nestedPilotSubscription.setOwner(newParent.dynamicOwner) + // Compile a list of inserters matching the actual nodes in the DOM. // We want to move actual de-facto DOM content, without re-stealing anything. + // #Note: this must be done AFTER the pilot transfer above, which can update the content. val contentInserters = nestedInsertContext.currentContentInsertersFromDom // Move the leading and trailing sentinels to the new place. @@ -134,7 +194,7 @@ final class NestedGroup( // Move content nodes (without unnecessary re-mounting) var lastRef: dom.Node = leadingSentinel.ref contentInserters.forEach { inserter => - // Note: this calls `moveToParent` internally if this nested inserter is dynamic. + // Note: this calls `moveTo` internally if this nested inserter is dynamic. inserter.addToDynamicList(newParent, afterRef = lastRef, newSlotName) lastRef = inserter.lastNode } @@ -144,30 +204,15 @@ final class NestedGroup( // to the new destination. nestedInsertContext.setCurrentParentNode(newParent) nestedInsertContext.setCurrentSlotName(newSlotName) - - // Transfer the subscription to the new parent's owner. - // This is seamless, without unnecessary re-mounting. - nestedPilotSubscription.setOwner(newParent.dynamicOwner) } private[laminar] def applySlot(newSlotName: String | Unit): Unit = { - // #Note: this `slotNameChanged` gate is not just a performance optimisation, - // it's needed to cover element stealing edge cases. - val slotNameChanged = - newSlotName.fold( - ifEmpty = nestedInsertContext.currentSlotName.nonEmpty - ) { nsn => - !nestedInsertContext.currentSlotName.contains(nsn) - } - if (slotNameChanged) { - val parent = nestedInsertContext.currentParentNode - // Compile a list of inserters matching the actual nodes in the DOM. - // We want to move apply the slot to actual de-facto DOM content only. - nestedInsertContext.currentContentInsertersFromDom.forEach { inserter => - inserter.applySlot(parent, newSlotName) - } - nestedInsertContext.setCurrentSlotName(newSlotName) + val parent = nestedInsertContext.currentParentNode + // Only the actual de-facto DOM content: nodes stolen out of our span are not ours to slot. + nestedInsertContext.currentContentInsertersFromDom.forEach { inserter => + inserter.applySlot(parent, newSlotName) } + nestedInsertContext.setCurrentSlotName(newSlotName) } /** @param keepItem Content items to leave in the parent's DOM – see diff --git a/src/test/scala/com/raquo/laminar/tests/InserterMoveSpec.scala b/src/test/scala/com/raquo/laminar/tests/InserterMoveSpec.scala index f2a3b7f7..8117c656 100644 --- a/src/test/scala/com/raquo/laminar/tests/InserterMoveSpec.scala +++ b/src/test/scala/com/raquo/laminar/tests/InserterMoveSpec.scala @@ -14,7 +14,7 @@ import com.raquo.laminar.utils.UnitSpec * Sections, in order: * 1. Classic single-node `child <--` moves (pre-#157 path: ChildInserter.switchToChild). * 2. Classic `children <--` plain-element moves (pre-#157 reconcile: updateChildren). - * 3. A dynamic inserter reordered WITHIN one `children <--` list (moveWithinDynamicList). + * 3. A dynamic inserter reordered WITHIN one `children <--` list. * 4. A dynamic inserter moved BETWEEN two `children <--` lists (add-first steal / remove-first / * the #163 two-bindings characterization), including nested and depth-2/3 spans. * 4b. Steal-BACK from a sibling (stale-re-emit re-steal), run against both same-parent and @@ -37,7 +37,7 @@ class InserterMoveSpec extends UnitSpec { // under two separate parent divs (`CrossParent`). This is the move-suite analog of // `SlotStealingSpec.SiblingSlots` (minus the slots). The same-parent layout is the sharp one // for steal-BACK: the two lists no longer differ by DOM `parentNode`, so a DynamicInserter - // re-stolen from a sibling takes `moveWithinDynamicList`'s SAME-parent branch (a raw + // re-stolen from a sibling takes `NestedGroup.moveTo`'s SAME-parent branch (a raw // reposition, no `moveToParent`) rather than the cross-parent transfer. A steal choreography is // written once and registered against both layouts. `expectRoot` wraps each list's content in // that list's own leading + trailing sentinels, then composes the two per the layout. @@ -662,7 +662,7 @@ class InserterMoveSpec extends UnitSpec { it("moving elements within / between classic `children <--` lists never re-mounts them") { // The classic reconcile (ChildrenInserter.updateChildren) routes reorders and steals - // through moveWithinDynamicList, so relocating a plain element must not tear its DOM + // through addToDynamicList, so relocating a plain element must not tear its DOM // span down and re-add it. We pin this via lifecycle events: reorders and an // add-first steal fire NOTHING, while a genuine removal DOES unmount — proving the // moves are real no-ops, not luck. @@ -755,11 +755,11 @@ class InserterMoveSpec extends UnitSpec { } // ---------------------------------------------------------------------------------- - // 3. Dynamic inserter reordered WITHIN one `children <--` list (moveWithinDynamicList) + // 3. Dynamic inserter reordered WITHIN one `children <--` list // ---------------------------------------------------------------------------------- it("reordering never re-mounts items: static and dynamic inserters move without re-running") { - // A move (moveWithinDynamicList) must relocate an item's DOM span WITHOUT tearing it down + // A move must relocate an item's DOM span WITHOUT tearing it down // and re-adding it: the logical parent is unchanged, so hooks/subscriptions must not re-run. // We pin this via lifecycle events – zero mount/unmount for any moved item, covering BOTH the // DYNAMIC item's override (its content span) AND the STATIC items' base-class move (the pure @@ -821,7 +821,7 @@ class InserterMoveSpec extends UnitSpec { .clear() } - withClue("swap the two static neighbours (pure static-inserter moves, base-class moveWithinDynamicList) – no re-mount:") { + withClue("swap the two static neighbours (pure static-inserter moves) – no re-mount:") { itemsVar.set(List(staticB, staticA, dyn)) expectNode(div.of("H", sentinel, span of "B", span of "A", sentinel, span of "d1", sentinel, sentinel)) observeCount shouldBe 2 @@ -830,7 +830,7 @@ class InserterMoveSpec extends UnitSpec { } it("multi-node dynamic span moves forward and backward, of varying length (no re-mount)") { - // Exercises DynamicInserter.moveWithinDynamicList directly: the whole nested span (leading + // Exercises NestedGroup.moveTo's same-parent branch: the whole nested span (leading // sentinel .. content nodes .. trailing sentinel) is relocated as a unit. Each move must relocate // the span WITHOUT re-mounting any of its content or the static neighbour it passes (asserted as // zero lifecycle events per move). We grow / shrink the span between moves so the internal walk @@ -1441,7 +1441,7 @@ class InserterMoveSpec extends UnitSpec { // The stale-re-emit re-steal ("last write wins"), run against BOTH the same-parent and // cross-parent layouts via `TwoLists`. L2 steals the item add-first (L1's contentMap goes stale), // then L1 re-emits WITH the item and steals it back. For same-parent siblings this exercises - // `moveWithinDynamicList`'s same-parent branch: the two lists share the parent ELEMENT — hence the + // `NestedGroup.moveTo`'s same-parent branch: the two lists share the parent ELEMENT — hence the // same mount owner — so a raw reposition (without `moveToParent`'s owner transfer) is correct, and // the item's live subscription must survive the re-steal. Each choreography asserts the re-steal is // seamless (no re-mount / no re-render) and the item stays live at its home afterwards. @@ -2078,7 +2078,7 @@ class InserterMoveSpec extends UnitSpec { // After another host STEALS a dynamic inserter and then GENUINELY removes it, the group is torn // down (`nestedGroupOpt` cleared). But the original list still tracks the inserter in its // `contentMap` (it never re-emitted), so its next re-emission of that inserter routes to - // `moveWithinDynamicList` (with nothing to move). This must NOT fail on the stale tracking — it + // `addToDynamicList` (with nothing to move). This must NOT fail on the stale tracking — it // must place the inserter afresh: re-insert + re-mount, exactly like re-adding a plain element // that had been removed. The list's item count already counted this inserter (it was in the // previous map), so a rebuild changes no count, while a genuinely new sibling still does. diff --git a/src/test/scala/com/raquo/laminar/tests/NestedGroupActivationOrderSpec.scala b/src/test/scala/com/raquo/laminar/tests/NestedGroupActivationOrderSpec.scala index 8d901e60..c42273ff 100644 --- a/src/test/scala/com/raquo/laminar/tests/NestedGroupActivationOrderSpec.scala +++ b/src/test/scala/com/raquo/laminar/tests/NestedGroupActivationOrderSpec.scala @@ -139,7 +139,12 @@ class NestedGroupActivationOrderSpec extends UnitSpec { withClue("still live on HOST:") { r.valueVar.set("x") - tracker.assertEvents(_.elementCreated("x3"), _.mounted("x3"), _.unmounted("w2")).clear() + // #Note: `child <--` swaps unmount the old node before mounting the new one + tracker.assertEvents( + _.elementCreated("x3"), + _.unmounted("w2"), + _.mounted("x3") + ).clear() expectNode( div.of( div.of("LIST", sentinel, sentinel), diff --git a/src/test/scala/com/raquo/laminar/tests/NestedGroupReStealSpec.scala b/src/test/scala/com/raquo/laminar/tests/NestedGroupReStealSpec.scala index 33da08c8..a6269270 100644 --- a/src/test/scala/com/raquo/laminar/tests/NestedGroupReStealSpec.scala +++ b/src/test/scala/com/raquo/laminar/tests/NestedGroupReStealSpec.scala @@ -23,7 +23,7 @@ class NestedGroupReStealSpec extends UnitSpec { * L2 steals `dyn` and drops it, tearing its group down while L1 still tracks it. Then * `parent.amend(dyn)` rebuilds the group plainly (without a trailing sentinel), and L1 * steals it back by re-emitting it, which takes the same-parent branch of - * `DynamicInserter.moveWithinDynamicList`. + * `NestedGroup.moveTo`. * * `dyn` must render `c` on activation. */ diff --git a/src/test/scala/com/raquo/laminar/tests/NestedInsertersSpec.scala b/src/test/scala/com/raquo/laminar/tests/NestedInsertersSpec.scala index 47b69cd3..a791a433 100644 --- a/src/test/scala/com/raquo/laminar/tests/NestedInsertersSpec.scala +++ b/src/test/scala/com/raquo/laminar/tests/NestedInsertersSpec.scala @@ -391,7 +391,7 @@ class NestedInsertersSpec extends UnitSpec { it("nested `children <--` item reordered within its list WHILE EMPTY: the empty span moves as a unit, then populates at its new position") { // An ALREADY-empty span (bare [leading, trailing]) is moved WITHIN one list via - // `moveWithinDynamicList`. The zero-length span must relocate past its neighbour, and the first + // `NestedGroup.moveTo`. The zero-length span must relocate past its neighbour, and the first // population must land at the span's new position. val tracker = createEventTracker() val innerVar = Var[List[Node]](Nil) diff --git a/src/test/scala/com/raquo/laminar/tests/SlotAttributeStealingSpec.scala b/src/test/scala/com/raquo/laminar/tests/SlotAttributeStealingSpec.scala index bd773f5a..140e83ed 100644 --- a/src/test/scala/com/raquo/laminar/tests/SlotAttributeStealingSpec.scala +++ b/src/test/scala/com/raquo/laminar/tests/SlotAttributeStealingSpec.scala @@ -1,7 +1,7 @@ package com.raquo.laminar.tests import com.raquo.laminar.api.L._ -import com.raquo.laminar.inserters.CollectionCommand +import com.raquo.laminar.inserters.{CollectionCommand, Inserter} import com.raquo.laminar.inserters.CollectionCommand.{Append, ReplaceAll} import com.raquo.laminar.nodes.Slot import com.raquo.laminar.utils.UnitSpec @@ -143,6 +143,85 @@ class SlotAttributeStealingSpec extends UnitSpec { } } + it("a slotted-list reconcile re-asserts the Slot's slot over a manual override on a nested group's content") { + // Same as the above, but `e` sits inside a nested group, next to a plain item `f`. + // Every reconcile or move that touches the group is a fresh Slot write for its content, + // exactly like it is for `f`: whether the group stays in place, is reordered, or is moved + // to another list under the same element or a different one. + val tracker = createEventTracker() + val e = tracker.createSpan("E") + val f = tracker.createSpan("F") + tracker.clear() + val nested = children <-- Val(List(e)) + val items1 = Var[List[Inserter]](List(nested, f)) + val items2 = Var[List[Inserter]](Nil) + val items3 = Var[List[Inserter]](Nil) + val host = div(new Slot("prefix")(children <-- items1.signal, children <-- items2.signal)) + val otherHost = div(new Slot("prefix")(children <-- items3.signal)) + + def overrideSlotManually(): Unit = { + e.amend(slot := "manual") + f.amend(slot := "manual") + e.ref.getAttribute("slot") shouldBe "manual" + f.ref.getAttribute("slot") shouldBe "manual" + } + + withClue("both elements get the Slot's slot on mount:") { + mount(div(host, otherHost)) + tracker + .assertEvents( + _.mounted("E"), + _.mounted("F") + ) + .clear() + e.ref.getAttribute("slot") shouldBe "prefix" + f.ref.getAttribute("slot") shouldBe "prefix" + } + + withClue("the list re-emits the same items in place:") { + overrideSlotManually() + items1.set(List(nested, f)) + tracker.assertNoEvents.clear() + e.ref.getAttribute("slot") shouldBe "prefix" + f.ref.getAttribute("slot") shouldBe "prefix" + } + + withClue("the list reorders its items:") { + overrideSlotManually() + items1.set(List(f, nested)) + tracker.assertNoEvents.clear() + e.ref.getAttribute("slot") shouldBe "prefix" + f.ref.getAttribute("slot") shouldBe "prefix" + } + + withClue("a sibling list under the same element steals the items:") { + overrideSlotManually() + items2.set(List(nested, f)) + items1.set(Nil) + tracker.assertNoEvents.clear() + e.ref.getAttribute("slot") shouldBe "prefix" + f.ref.getAttribute("slot") shouldBe "prefix" + } + + withClue("a list under a different element steals the items:") { + overrideSlotManually() + items3.set(List(nested, f)) + items2.set(Nil) + tracker.assertNoEvents.clear() + e.ref.getAttribute("slot") shouldBe "prefix" + f.ref.getAttribute("slot") shouldBe "prefix" + expectNode( + otherHost.ref, + div.of( + sentinel, + sentinel, span.of("E", slot is "prefix"), sentinel, // nested: [e] + span.of("F", slot is "prefix"), + sentinel + ) + ) + } + } + List(true, false).foreach { minimizeDiff => it(s"a slotted `children.command` ReplaceAll(minimizeDiff = $minimizeDiff) re-asserts the Slot's slot over a manual override, even on a node that stays in place") { diff --git a/src/test/scala/com/raquo/laminar/tests/SlotRetainedContentSpec.scala b/src/test/scala/com/raquo/laminar/tests/SlotRetainedContentSpec.scala index 5715bd2a..d395ace3 100644 --- a/src/test/scala/com/raquo/laminar/tests/SlotRetainedContentSpec.scala +++ b/src/test/scala/com/raquo/laminar/tests/SlotRetainedContentSpec.scala @@ -72,7 +72,7 @@ class SlotRetainedContentSpec extends UnitSpec { it("reconciles the slot when a switching item also moves position") { // Same bare<->Slot switch as above, but the item also changes position, so it goes - // through the diff's MOVE branch (`moveWithinDynamicList`) instead of the in-place + // through the diff's MOVE branch (`addToDynamicList`) instead of the in-place // branch. The move must re-affirm the correct slot without re-mounting the node. val tracker = createEventTracker() val a = tracker.createSpan("A") diff --git a/src/test/scala/com/raquo/laminar/tests/SlotSpec.scala b/src/test/scala/com/raquo/laminar/tests/SlotSpec.scala index 6e31590b..ac7c33a9 100644 --- a/src/test/scala/com/raquo/laminar/tests/SlotSpec.scala +++ b/src/test/scala/com/raquo/laminar/tests/SlotSpec.scala @@ -197,7 +197,7 @@ class SlotSpec extends UnitSpec { // A nested group's slot reconcile (`NestedGroup.applySlot`) must re-slot only the inner nodes // still in the group's span. A node a THIRD slot stole out of the group keeps that slot's // attribute – the group must not rewrite it from under its new host. The reconcile is reached - // via a same-parent re-steal between two sibling Slots (moveWithinDynamicList's same-parent + // via a same-parent re-steal between two sibling Slots (NestedGroup.moveTo's same-parent // branch, which runs applySlot); a move between slots (moveToParent) is exercised on the way in. val tracker = createEventTracker() val a = tracker.createSpan("A")