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 930 forks source link

Unexpected DOM patching when using append and forms together #499

Closed andykent closed 4 years ago

andykent commented 4 years ago

Environment

Actual behavior

When using phx-update="append" and the elements contain forms that append to the list the diffing behaviour leads to very unexpected results. I've included a reproduction of one variant of this where form elements get duplicated but have also seen it in another project where the form elements value gets blanked out depending on it's focus state. My best guess is that it's to do with the interaction between the append DOM diffing / updating based on ID and the logic to avoid patching focused form elements but I could be totally wrong. Hopefully this isn't a case of user error :/

Expected behavior

Elements can be appended and patched without leading to an inconsistent DOM.

Demo

Kapture 2019-11-23 at 0 30 45

Code

defmodule TestLive do
  use Phoenix.LiveView

  def render(assigns) do
    ~L"""
      <div>
        <div id="list" phx-update="append">
          <%= for {v, id} <- @values do %>
            <div id="item-<%= id %>">
              <hr />
              <form id="form-<%= id %>" phx-submit="submit">
                <input type="hidden" name="id" value="<%= id %>" />
                <textarea name="text"><%= v %></textarea>
                <button type="submit">Submit</button>
              </form>
              <p>Block <%= id %>'s value is <%= v %>
            </div>
          <% end %>
        </div>
      </div>
    """
  end

  def mount(_session, socket) do
    {:ok, assign(socket, %{values: [{"", 0}], current_id: 0}), temporary_assigns: [values: []]}
  end

  def handle_event("submit", %{"text" => text, "id" => id}, socket) do
    id = String.to_integer(id)
    current_id = socket.assigns[:current_id]
    value = {text, id}

    {values, next_id} =
      if text != "" && current_id == id do
        # submitting a block so append a new blank one
        next_id = current_id + 1
        {[value, {"", next_id}], next_id}
      else
        # editing a block
        {[value], current_id}
      end

    socket =
      socket
      |> assign(:values, values)
      |> assign(:current_id, next_id)

    {:noreply, socket}
  end
end

edit: I tweaked the demo to make things a little clearer

davydog187 commented 4 years ago

I'm noticing similar behavior when rendering a list of forms

Screen Shot 2019-11-24 at 4 42 01 PM

It seems like the DOM is being incorrectly patched for the first element in my list

snewcomer commented 4 years ago

@andykent This seems like a solvable problem! Thanks for the issue. It might take me 1-2 hrs just to get an reproduction going. Would you mind putting this into a github repo if you have something easily accessible?

davydog187 commented 4 years ago

I'm not sure if this demonstrates the same problem, but I started to put together an example that just renders a list of forms, and it is doing that incorrectly. Instead of having N number of forms, its being patched into a single form element.

Example: https://github.com/davydog187/nested_forms

Screen Shot 2019-11-26 at 8 06 36 AM
andykent commented 4 years ago

@snewcomer great to hear. I will try to find some time to put a repo together probably won't be able to get to it until the weekend now though.

In the meantime the code I posted above is a complete working example, if you have an existing live_view app around you can drop it into app_web/live/test_live.ex add a live "/test", TestLive route to your router and you should be good to go.

andykent commented 4 years ago

@snewcomer I forked the live_view_examples repo and pushed a branch that includes the code above... https://github.com/andykent/phoenix_live_view_example/tree/forms-test

If you pull that branch, start the server and then visit http://localhost:4000/test you should be able to reproduce the issue.

Thanks.

snewcomer commented 4 years ago

@andykent Amazing reproduction! Thank you. I have reproduced this as a bug in [morphdom](https://github.com/patrick-steele-idem/morphdom). I'll look into it soon.

snewcomer commented 4 years ago

Thank you all for the issue! Found it. Because form input elements are mapped as properties to their parent form according to it's name, if <input name="id" is used, accessing form.id was actually returning the input element. So getAttribute is safer here.

https://github.com/patrick-steele-idem/morphdom/pull/174

Will merge and bump here in the next few days!

andykent commented 4 years ago

Oh wow, great find @snewcomer I never imagined it would be a naming issue. Just confirming for anyone else hitting this issue that you can work around it by renaming the "id" field to something else. This workaround is working on the repo above.