fix phx-remove and phx-click-loading in sticky LVs (#3659)

Fixes #3656
Fixes #3658

Author: SteffenDE

assets/js/phoenix_live_view/dom_patch.js

@@ -433,7 +433,7 @@ export default class DOMPatch {
   transitionPendingRemoves(){
     let {pendingRemoves, liveSocket} = this
     if(pendingRemoves.length > 0){
-      liveSocket.transitionRemoves(pendingRemoves, false, () => {
+      liveSocket.transitionRemoves(pendingRemoves, () => {
         pendingRemoves.forEach(el => {
           let child = DOM.firstPhxChild(el)
           if(child){ liveSocket.destroyViewByEl(child) }

assets/js/phoenix_live_view/live_socket.js

@@ -393,22 +393,26 @@ export default class LiveSocket {
   }
 
   replaceMain(href, flash, callback = null, linkRef = this.setPendingLink(href)){
-    let liveReferer = this.currentLocation.href
+    const liveReferer = this.currentLocation.href
     this.outgoingMainEl = this.outgoingMainEl || this.main.el
-    let removeEls = DOM.all(this.outgoingMainEl, `[${this.binding("remove")}]`)
-    let newMainEl = DOM.cloneNode(this.outgoingMainEl, "")
+
+    const stickies = DOM.findPhxSticky(document) || []
+    const removeEls = DOM.all(this.outgoingMainEl, `[${this.binding("remove")}]`)
+      .filter(el => !DOM.isChildOfAny(el, stickies))
+
+    const newMainEl = DOM.cloneNode(this.outgoingMainEl, "")
     this.main.showLoader(this.loaderTimeout)
     this.main.destroy()
 
     this.main = this.newRootView(newMainEl, flash, liveReferer)
     this.main.setRedirect(href)
-    this.transitionRemoves(removeEls, true)
+    this.transitionRemoves(removeEls)
     this.main.join((joinCount, onDone) => {
       if(joinCount === 1 && this.commitPendingLink(linkRef)){
         this.requestDOMUpdate(() => {
           // remove phx-remove els right before we replace the main element
           removeEls.forEach(el => el.remove())
-          DOM.findPhxSticky(document).forEach(el => newMainEl.appendChild(el))
+          stickies.forEach(el => newMainEl.appendChild(el))
           this.outgoingMainEl.replaceWith(newMainEl)
           this.outgoingMainEl = null
           callback && callback(linkRef)
@@ -418,12 +422,8 @@ export default class LiveSocket {
     })
   }
 
-  transitionRemoves(elements, skipSticky, callback){
+  transitionRemoves(elements, callback){
     let removeAttr = this.binding("remove")
-    if(skipSticky){
-      const stickies = DOM.findPhxSticky(document) || []
-      elements = elements.filter(el => !DOM.isChildOfAny(el, stickies))
-    }
     let silenceEvents = (e) => {
       e.preventDefault()
       e.stopImmediatePropagation()
@@ -800,7 +800,8 @@ export default class LiveSocket {
   }
 
   historyRedirect(e, href, linkState, flash, targetEl){
-    if(targetEl && e.isTrusted && e.type !== "popstate"){ targetEl.classList.add("phx-click-loading") }
+    const clickLoading = targetEl && e.isTrusted && e.type !== "popstate"
+    if(clickLoading){ targetEl.classList.add("phx-click-loading") }
     if(!this.isConnected() || !this.main.isMain()){ return Browser.redirect(href, flash) }
 
     // convert to full href if only path prefix
@@ -829,6 +830,9 @@ export default class LiveSocket {
           DOM.dispatchEvent(window, "phx:navigate", {detail: {href, patch: false, pop: false, direction: "forward"}})
           this.registerNewLocation(window.location)
         }
+        // explicitly undo click-loading class
+        // (in case it originated in a sticky live view, otherwise it would be removed anyway)
+        if(clickLoading){ targetEl.classList.remove("phx-click-loading") }
         done()
       })
     })

test/e2e/support/issues/issue_3656.ex

@@ -0,0 +1,56 @@
+defmodule Phoenix.LiveViewTest.E2E.Issue3656Live do
+  # https://github.com/phoenixframework/phoenix_live_view/issues/3656
+
+  use Phoenix.LiveView
+
+  alias Phoenix.LiveView.JS
+
+  def mount(_params, _session, socket) do
+    {:ok, socket}
+  end
+
+  def render(assigns) do
+    ~H"""
+    <style>
+      * { font-size: 1.1em }
+      nav { margin-top: 1em }
+      nav a { padding: 8px 16px; border: 1px solid black; text-decoration: none }
+      nav a:visited { color: inherit }
+      nav a.active { border: 3px solid green }
+      nav a.phx-click-loading { animation: pulsate 2s infinite }
+      @keyframes pulsate {
+        0% {
+          background-color: white;
+        }
+        50% {
+          background-color: red;
+        }
+        100% {
+          background-color: white;
+        }
+      }
+    </style>
+
+    {live_render(@socket, Phoenix.LiveViewTest.E2E.Issue3656Live.Sticky,
+      id: "sticky",
+      sticky: true
+    )}
+    """
+  end
+end
+
+defmodule Phoenix.LiveViewTest.E2E.Issue3656Live.Sticky do
+  use Phoenix.LiveView
+
+  def mount(:not_mounted_at_router, _session, socket) do
+    {:ok, socket, layout: false}
+  end
+
+  def render(assigns) do
+    ~H"""
+    <nav>
+      <.link navigate="/issues/3656?navigated=true">Link 1</.link>
+    </nav>
+    """
+  end
+end

test/e2e/support/issues/issue_3658.ex

@@ -0,0 +1,38 @@
+defmodule Phoenix.LiveViewTest.E2E.Issue3658Live do
+  # https://github.com/phoenixframework/phoenix_live_view/issues/3658
+
+  use Phoenix.LiveView
+
+  alias Phoenix.LiveView.JS
+
+  def mount(_params, _session, socket) do
+    {:ok, socket}
+  end
+
+  def render(assigns) do
+    ~H"""
+    <.link navigate="/issues/3658?navigated=true">Link 1</.link>
+
+    {live_render(@socket, Phoenix.LiveViewTest.E2E.Issue3658Live.Sticky,
+      id: "sticky",
+      sticky: true
+    )}
+    """
+  end
+end
+
+defmodule Phoenix.LiveViewTest.E2E.Issue3658Live.Sticky do
+  use Phoenix.LiveView
+
+  def mount(:not_mounted_at_router, _session, socket) do
+    {:ok, socket, layout: false}
+  end
+
+  def render(assigns) do
+    ~H"""
+    <div>
+      <div id="foo" phx-remove={Phoenix.LiveView.JS.dispatch("my-event")}>Hi</div>
+    </div>
+    """
+  end
+end

test/e2e/test_helper.exs

@@ -160,6 +160,8 @@ defmodule Phoenix.LiveViewTest.E2E.Router do
       live "/3496/b", Issue3496.BLive
       live "/3529", Issue3529Live
       live "/3651", Issue3651Live
+      live "/3656", Issue3656Live
+      live "/3658", Issue3658Live
     end
   end
 

test/e2e/tests/issues/3656.spec.js

@@ -0,0 +1,21 @@
+const {test, expect} = require("../../test-fixtures")
+const {syncLV, attributeMutations} = require("../../utils")
+
+// https://github.com/phoenixframework/phoenix_live_view/issues/3656
+test("phx-click-loading is removed from links in sticky LiveViews", async ({page}) => {
+  await page.goto("/issues/3656")
+  await syncLV(page)
+
+  let changes = attributeMutations(page, "nav a")
+
+  const link = page.getByRole("link", {name: "Link 1"})
+  await link.click()
+
+  await syncLV(page)
+  await expect(link).not.toHaveClass("phx-click-loading")
+
+  expect(await changes()).toEqual(expect.arrayContaining([
+    {attr: "class", oldValue: null, newValue: "phx-click-loading"},
+    {attr: "class", oldValue: "phx-click-loading", newValue: ""},
+  ]))
+})

test/e2e/tests/issues/3658.spec.js

@@ -0,0 +1,15 @@
+const {test, expect} = require("../../test-fixtures")
+const {syncLV} = require("../../utils")
+
+// https://github.com/phoenixframework/phoenix_live_view/issues/3658
+test("phx-remove elements inside sticky LiveViews are not removed when navigating", async ({page}) => {
+  await page.goto("/issues/3658")
+  await syncLV(page)
+
+  await expect(page.locator("#foo")).toBeVisible()
+  await page.getByRole("link", {name: "Link 1"}).click()
+
+  await syncLV(page)
+  // the bug would remove the element
+  await expect(page.locator("#foo")).toBeVisible()
+})