phoenixframework / phoenix_live_view

Rich, real-time user experiences with server-rendered HTML
https://hex.pm/packages/phoenix_live_view
MIT License
6.18k stars 931 forks source link

`phx-remove` not triggered for nested HTML #1719

Closed mveytsman closed 2 years ago

mveytsman commented 2 years ago

Here's an example of a failing test

 test("worked in deeply nested html", done => {
      let view = setupView(`<div>
        <div id="modal" phx-remove='[["push", {"event": "removed"}], ["toggle", {"to": "#modal"}]]'>modal</div>
      </div>
      `)

      view.pushEvent = (eventType, sourceEl, targetCtx, event, data) => {
        expect(event).toEqual("removed")
        done()
      }

      let updatedHtml = `<div id="foo"></div>`
      view.update({s: [updatedHtml]}, [])
      expect(view.el.innerHTML).toBe(updatedHtml)
    })

The issue is that morphdom calls onBeforeNodeDiscarded and onNodeDiscarded depth-first on a node and all it's children if it's being discarded, so if we have a phx-remove on a child of a node being discarded, we won't process the commands, as by the time we get to the childs onBeforeNodeDiscarded it's already gone from the DOM.

I have a draft PR for this at #1718.

josevalim commented 2 years ago

AFAIK this is by design. Otherwise we run into issues when doing page navigations or when removing a modal, where we would animate everything inside, while we should not. Maybe what we could do is to find the first child with phx-remove and animate it, but that can still lead to undesired animations. What is the use case?

mveytsman commented 2 years ago

I think that makes sense!

I do think triggering the first child might be good, otherwise we wouldn't trigger the phx_remove here.

<%= if @show do %>
  <div>
    parent
    <div phx-remove={some_transition()}>child</div>
  </div>
<% end %>

It is unintuitive that phx-remove works differently than a disconnect hook, which will trigger on children. If the goal is for JS commands to do what people are currently doing with hooks it's worth thinking about this.

chrismccord commented 2 years ago

phi-remove will work agains the first parent that has a phx-remove, so it would trigger in your scenario, but if the child div had children with phx-remove's, they would not be fired, so I believe we are reasonably well covered here. Let me know if you are seeing otherwise. Thanks!