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

Re-rendering a live view form causes checkbox problems #539

Closed drapergeek closed 4 years ago

drapergeek commented 4 years ago

Environment

Actual behavior

When the phx-change event is fired and the live view changes the state of an attribute on the socket, when the form is re-rendered, the checkboxes are not correctly checked. Sometimes it will change the state of other checkboxes on the form.

Expected behavior

The form should not change beyond the user's clicks.

Here is a small app that shows this problem in action: https://github.com/drapergeek/broken_live_view

I tried to eliminate everything that wasn't absolutely necessary to verify the issue. I've tried a few different things such as changing whether or not the submit button is rendered. I've also tried just outputting the value of the attribute on the page, this still causes the issue. If I don't change anything within the html rendering on the event, it seems to work fine.

If there's anything else I can answer, I'm happy to help.

snewcomer commented 4 years ago

@drapergeek Thank you for the issue! On an input[checked] element attributes are only the defaults and properties are the current values. This is a legitimate bug. I'm going to have to dig around and see where we can look at the checked property during our diff and bring that over. At present, there is nothing "visible" to tell us that this should be checked during diffing (except via the checked property)

drapergeek commented 4 years ago

Thanks for the help @snewcomer, I spent a lot of time trying to make sure it was a bug and not my mistake. Let me know if I can be of assistance.

snewcomer commented 4 years ago

In the meantime, if you get the checked attribute to change on the input, that should work if you have something with a need for an immediate fix.

<%= checkbox form, 
          :jimmy,
          name: ...
          checked: @value %>
snewcomer commented 4 years ago

I got a potential solution here...

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

I might need to think about this more. For simplicity sake, I would recommend we modify the checked attribute (<input checked>) in LV and as a result, we can handle that properly. This is possible b/c we deal in strings and our diffing algorithm can detect and handle this case properly.

However, this isn't how checkboxes work necessarily. Look up a checkbox rendered on any random page and inspect it. When you toggle on and off, you don't see any visual changes on the element but if you query it and ask input.checked, it will evaluate to true or false. Thus this information gets hidden from us b/c we deal in strings...

chrismccord commented 4 years ago

As Scott alluded to, you need to maintain the form state in the LiveView once the value is checked. Here is code based on your example app that works properly:

  def render(assigns) do
    ~L"""
    <%= form_for :departure, "/", [phx_change: "check_enabled"], fn form -> %>
      <label>
        <%= tag(:input, name: "departure[students][]", type: "checkbox", value: "timmy", 
                        checked: "timmy" in @ids) %>
        Timmy
      </label>

      <label>
        <%= tag(:input, name: "departure[students][]", type: "checkbox", value: "jimmy", 
                        checked: "jimmy" in @ids) %>
        Jimmy
      </label>
      <%= tag(:input, type: "submit", disabled: @disabled) %>
    <% end %>
    """
  end

  def mount(_, socket) do
    {:ok, assign(socket, ids: [], disabled: true)}
  end

  def handle_event(
        "check_enabled",
        %{"departure" => %{"students" => student_ids}},
        socket
      ) do
    {:noreply, assign(socket, disabled: false, ids: student_ids)}
  end

  def handle_event("check_enabled", _params, socket) do
    {:noreply, assign(socket, disabled: true, ids: [])}
  end

Anytime the client state such as form data is sent, you need to assign that state and render the form inputs accordingly. Closing as this is operating as expected, but feel free to send a doc PR if you think the docs could be clearer around this. Thanks!