Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Radio input checked state regression between v1.0.2..v1.0.3 #3684

Open
hattmarris opened this issue Feb 19, 2025 · 7 comments · May be fixed by #3690
Open

Radio input checked state regression between v1.0.2..v1.0.3 #3684

hattmarris opened this issue Feb 19, 2025 · 7 comments · May be fixed by #3690
Labels

Comments

@hattmarris
Copy link

hattmarris commented Feb 19, 2025

Environment

  • Elixir version (elixir -v): 1.18.2
  • Phoenix version (mix deps): 1.7.19
  • Phoenix LiveView version (mix deps): 1.0.4
  • Operating system: Linux Ubuntu 24.04
  • Browsers you attempted to reproduce this bug on (the more the merrier): Chrome / Safari / Brave
  • Does the problem persist after removing "assets/node_modules" and trying again? Yes/no: yes

Actual behavior

We are encountering a bug with radio type inputs checked state that seems to be a regression in version 1.0.3. The back end state bound to the checked attribute logs correctly, but the DOM patching does not correctly update the input checked state toggling between two radios.

I've isolated to have occurred in changes between v1.0.2..v1.0.3 (exists in >= 1.0.3 only). I can't replicate the error in 1.0.2 or lower, so I'm going to start combing through those changes to see which commit may be related. Will report back with any more info.

Here's the bug (v1.0.3 and v1.0.4):

Screencast.from.2025-02-19.14-17-26.webm

Expected behavior

Here's expected (<= v1.0.2):

Screencast.from.2025-02-19.14-20-07.webm

Update for @SteffenDE the bug rode in on this commit:

ad9beca

@hattmarris
Copy link
Author

Calling this out again because I'm not sure my "edit" to the issue will send a DM notification (if it did already, apologies for any annoyance 🙃) --

Update for @SteffenDE the bug rode in on this commit:

ad9beca

@SteffenDE
Copy link
Collaborator

Thank you, I’ll investigate!

@SteffenDE
Copy link
Collaborator

@hattmarris can you provide some details on how your radio setup looks like? If you can provide something to reproduce, that would be best.

Here's an example radio setup that works as expected, so you can see what the differences are and maybe adapt it to show your problem:

Application.put_env(:sample, Example.Endpoint,
  http: [ip: {127, 0, 0, 1}, port: 5001],
  server: true,
  live_view: [signing_salt: "aaaaaaaa"],
  secret_key_base: String.duplicate("a", 64)
)

Mix.install([
  {:plug_cowboy, "~> 2.5"},
  {:jason, "~> 1.0"},
  {:phoenix, "~> 1.7"},
  # please test your issue using the latest version of LV from GitHub!
  {:phoenix_live_view,
   github: "phoenixframework/phoenix_live_view", branch: "main", override: true},
  {:ecto, "~> 3.11"},
  {:phoenix_ecto, "~> 4.5"}
])

# build the LiveView JavaScript assets (this needs mix and npm available in your path!)
path = Phoenix.LiveView.__info__(:compile)[:source] |> Path.dirname() |> Path.join("../")
System.cmd("mix", ["deps.get"], cd: path, into: IO.binstream())
System.cmd("npm", ["install"], cd: Path.join(path, "./assets"), into: IO.binstream())
System.cmd("mix", ["assets.build"], cd: path, into: IO.binstream())

defmodule Example.ErrorView do
  def render(template, _), do: Phoenix.Controller.status_message_from_template(template)
end

defmodule Example.HomeLive do
  use Phoenix.LiveView, layout: {__MODULE__, :live}

  defp changeset(params) do
    data = %{}
    types = %{items: {:array, :string}, name: :string, radio: :string, select: :string}

    {data, types}
    |> Ecto.Changeset.cast(params, Map.keys(types))
    |> Ecto.Changeset.validate_required(:items)
    |> Ecto.Changeset.validate_length(:items, min: 1)
    |> Ecto.Changeset.validate_required(:radio)
    |> Ecto.Changeset.validate_required(:select)
  end

  def mount(_params, _session, socket) do
    {:ok, assign(socket, form: to_form(changeset(%{}), as: :foo), payload: nil)}
  end

  def handle_event("validate", %{"foo" => params}, socket) do
    changeset = changeset(params)

    {:noreply,
     assign(socket, form: to_form(changeset, as: :foo, action: :validate), payload: params)}
  end

  def handle_event("submit", %{"foo" => params}, socket) do
    changeset = changeset(params)

    {:noreply,
     assign(socket, form: to_form(changeset, as: :foo, action: :submit), payload: params)}
  end

  def render("live.html", assigns) do
    ~H"""
    <script src="/assets/phoenix/phoenix.js">
    </script>
    <script src="/assets/phoenix_live_view/phoenix_live_view.js">
    </script>
    <%!-- uncomment to use enable tailwind --%>
    <script src="https://cdn.tailwindcss.com">
    </script>
    <script>
      let liveSocket = new window.LiveView.LiveSocket("/live", window.Phoenix.Socket)
      liveSocket.connect()
    </script>
    {@inner_content}
    """
  end

  def render(assigns) do
    ~H"""
    <.form
      for={@form}
      id="foo"
      phx-change="validate"
      phx-submit="submit"
      class="max-w-lg p-8 flex flex-col gap-4"
    >
      <.input type="text" field={@form[:name]} />

      <hr />

      <fieldset>
        <input type="hidden" name={@form[:radio].name} value="" />
        <legend>Radio example:</legend>

        <div>
          <input
            type="radio"
            id="huey"
            name={@form[:radio].name}
            value="huey"
            checked={@form[:radio].value === "huey"}
          />
          <label for="huey">Huey</label>
        </div>

        <div>
          <input
            type="radio"
            id="dewey"
            name={@form[:radio].name}
            value="dewey"
            checked={@form[:radio].value === "dewey"}
          />
          <label for="dewey">Dewey</label>
        </div>

        <div>
          <input
            type="radio"
            id="louie"
            name={@form[:radio].name}
            value="louie"
            checked={@form[:radio].value === "louie"}
          />
          <label for="louie">Louie</label>
        </div>

        <%= if used_input?(@form[:radio]) do %>
          <.error :for={msg <- @form[:radio].errors}>{inspect(msg)}</.error>
        <% end %>
      </fieldset>

      <hr />

      <p>Select multiple example:</p>

      <input type="hidden" value="" name={@form[:items].name <> "[]"} />
      <.input field={@form[:items]} type="select" multiple options={["foo", "bar", "baz"]} />

      <p>Select example:</p>

      <.input
        field={@form[:select]}
        type="select"
        required
        prompt="Choose an item"
        options={["foo", "bar", "baz"]}
      />

      <button type="submit">Submit</button>

      <hr />

      <div class="flex flex-col gap-4">
        <p>Payload:</p>
        <pre><%= inspect(@payload) %></pre>
        <p>Selected items: {inspect(@form[:items].value)}</p>
        <p>Selected radio: {inspect(@form[:radio].value)}</p>
        <p>Used input @form[:name]: {used_input?(@form[:name])}</p>
        <p>Used input @form[:radio]: {used_input?(@form[:radio])}</p>
        <p>Used input @form[:items]: {used_input?(@form[:items])}</p>
        <p>Form errors: {inspect(@form.errors)}</p>
      </div>
    </.form>
    """
  end

  @doc """
  Renders an input with label and error messages.

  A `Phoenix.HTML.FormField` may be passed as argument,
  which is used to retrieve the input name, id, and values.
  Otherwise all attributes may be passed explicitly.

  ## Types

  This function accepts all HTML input types, considering that:

    * You may also set `type="select"` to render a `<select>` tag

    * `type="checkbox"` is used exclusively to render boolean values

    * For live file uploads, see `Phoenix.Component.live_file_input/1`

  See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input
  for more information. Unsupported types, such as hidden and radio,
  are best written directly in your templates.

  ## Examples

      <.input field={@form[:email]} type="email" />
      <.input name="my-input" errors={["oh no!"]} />
  """
  attr :id, :any, default: nil
  attr :name, :any
  attr :label, :string, default: nil
  attr :value, :any

  attr :type, :string,
    default: "text",
    values: ~w(checkbox color date datetime-local email file month number password
               range search select tel text textarea time url week)

  attr :field, Phoenix.HTML.FormField,
    doc: "a form field struct retrieved from the form, for example: @form[:email]"

  attr :errors, :list, default: []
  attr :checked, :boolean, doc: "the checked flag for checkbox inputs"
  attr :prompt, :string, default: nil, doc: "the prompt for select inputs"
  attr :options, :list, doc: "the options to pass to Phoenix.HTML.Form.options_for_select/2"
  attr :multiple, :boolean, default: false, doc: "the multiple flag for select inputs"

  attr :rest, :global,
    include: ~w(accept autocomplete capture cols disabled form list max maxlength min minlength
                multiple pattern placeholder readonly required rows size step)

  slot :inner_block

  def input(%{field: %Phoenix.HTML.FormField{} = field} = assigns) do
    errors = if Phoenix.Component.used_input?(field), do: field.errors, else: []

    assigns
    |> assign(field: nil, id: assigns.id || field.id)
    |> assign(:errors, errors)
    |> assign_new(:name, fn -> if assigns.multiple, do: field.name <> "[]", else: field.name end)
    |> assign_new(:value, fn -> field.value end)
    |> input()
  end

  def input(%{type: "checkbox"} = assigns) do
    assigns =
      assign_new(assigns, :checked, fn ->
        Phoenix.HTML.Form.normalize_value("checkbox", assigns[:value])
      end)

    ~H"""
    <div>
      <label class="flex items-center gap-4 text-sm leading-6 text-zinc-600">
        <input type="hidden" name={@name} value="false" />
        <input
          type="checkbox"
          id={@id}
          name={@name}
          value="true"
          checked={@checked}
          class="rounded border-zinc-300 text-zinc-900 focus:ring-0"
          {@rest}
        />
        {@label}
      </label>
      <.error :for={msg <- @errors}>{inspect(msg)}</.error>
    </div>
    """
  end

  def input(%{type: "select"} = assigns) do
    ~H"""
    <div>
      <.label for={@id}>{@label}</.label>
      <select
        id={@id}
        name={@name}
        class="mt-2 block w-full rounded-md border border-gray-300 bg-white shadow-sm focus:border-zinc-400 focus:ring-0 sm:text-sm"
        multiple={@multiple}
        {@rest}
      >
        <option :if={@prompt} value="" selected>{@prompt}</option>
        {Phoenix.HTML.Form.options_for_select(@options, @value)}
      </select>
      <.error :for={msg <- @errors}>{inspect(msg)}</.error>
    </div>
    """
  end

  def input(%{type: "textarea"} = assigns) do
    ~H"""
    <div>
      <.label for={@id}>{@label}</.label>
      <textarea
        id={@id}
        name={@name}
        class={[
          "mt-2 block w-full rounded-lg text-zinc-900 focus:ring-0 sm:text-sm sm:leading-6 min-h-[6rem]",
          @errors == [] && "border-zinc-300 focus:border-zinc-400",
          @errors != [] && "border-rose-400 focus:border-rose-400"
        ]}
        {@rest}
      ><%= Phoenix.HTML.Form.normalize_value("textarea", @value) %></textarea>
      <.error :for={msg <- @errors}>{inspect(msg)}</.error>
    </div>
    """
  end

  # All other inputs text, datetime-local, url, password, etc. are handled here...
  def input(assigns) do
    ~H"""
    <div>
      <.label for={@id}>{@label}</.label>
      <input
        type={@type}
        name={@name}
        id={@id}
        value={Phoenix.HTML.Form.normalize_value(@type, @value)}
        class={[
          "mt-2 block w-full border rounded-lg text-zinc-900 focus:ring-0 sm:text-sm sm:leading-6",
          @errors == [] && "border-zinc-300 focus:border-zinc-400",
          @errors != [] && "border-rose-400 focus:border-rose-400"
        ]}
        {@rest}
      />
      <.error :for={msg <- @errors}>{inspect(msg)}</.error>
    </div>
    """
  end

  @doc """
  Renders a label.
  """
  attr :for, :string, default: nil
  slot :inner_block, required: true

  def label(assigns) do
    ~H"""
    <label for={@for} class="block text-sm font-semibold leading-6 text-zinc-800">
      {render_slot(@inner_block)}
    </label>
    """
  end

  @doc """
  Generates a generic error message.
  """
  slot :inner_block, required: true

  def error(assigns) do
    ~H"""
    <p class="mt-3 flex gap-3 text-sm leading-6 text-rose-600">
      {render_slot(@inner_block)}
    </p>
    """
  end
end

defmodule Example.Router do
  use Phoenix.Router
  import Phoenix.LiveView.Router

  pipeline :browser do
    plug(:accepts, ["html"])
  end

  scope "/", Example do
    pipe_through(:browser)

    live("/", HomeLive, :index)
  end
end

defmodule Example.Endpoint do
  use Phoenix.Endpoint, otp_app: :sample
  socket("/live", Phoenix.LiveView.Socket)

  plug Plug.Static, from: {:phoenix, "priv/static"}, at: "/assets/phoenix"
  plug Plug.Static, from: {:phoenix_live_view, "priv/static"}, at: "/assets/phoenix_live_view"

  plug(Example.Router)
end

{:ok, _} = Supervisor.start_link([Example.Endpoint], strategy: :one_for_one)
Process.sleep(:infinity)

@hattmarris
Copy link
Author

Thanks @SteffenDE ! I'll look into trying to replicate with a minimal setup (haven't done this before, so may take a bit) but in the meantime I can describe the most consistent reproduction:

A LiveComponent BadgeForm.render/1 calls a privately defined function component BadgeForm.award_type_radios/1 as:

<.award_type_radios award_type={@award_type} form={@form} badge={@badge} myself={@myself} />

The actual radio inputs do not call any Phoenix CoreComponent, but are just rendered in a loop as HTML inputs:

<%= for award_type <- Badge.award_type_values() do %>
  <div
    class="flex items-center gap-4"
    phx-click="change-award-type"
    phx-value-award-type={award_type}
    phx-target={@myself}
  >
    <input
      type="radio"
      id={award_type}
      name="award-type"
      value={award_type}
      checked={(@award_type == award_type) |> IO.inspect(label: "checked")}
      required
    />
  </div>
<% end %>

There are currently 2 award_type_values -- :auto and :manual. The IO.inspect there is the log from my screencasts which shows the back end assigns state being correctly bound to checked and DOM / browser on the right.

Most interesting difference I can think of between example you posted and our setup is that it's a LiveComponent instead of a LiveView?

Hope that helps, I'll look into reproducing more minimally on my end 👍

@hattmarris
Copy link
Author

Okay @SteffenDE I think we've got a minimal reproduction here -- the situation occurs when radio inputs are children of a parent form with a phx-change binding, but they have their own state managed by a LiveComponent (& phx-click binding). The example from our app is a virtual / computed value (award_type) - that is not a field on the struct for the parent form (it's state is managed by the child component).

We confirmed this example works in 1.0.2, but then fails in latest from main.

Application.put_env(:sample, Example.Endpoint,
  http: [ip: {127, 0, 0, 1}, port: 5001],
  server: true,
  live_view: [signing_salt: "aaaaaaaa"],
  secret_key_base: String.duplicate("a", 64)
)

Mix.install([
  {:plug_cowboy, "~> 2.5"},
  {:jason, "~> 1.0"},
  {:phoenix, "~> 1.7"},
  # please test your issue using the latest version of LV from GitHub!
  {:phoenix_live_view,
   github: "phoenixframework/phoenix_live_view", branch: "main", override: true},
  {:ecto, "~> 3.11"},
  {:phoenix_ecto, "~> 4.5"}
])

# build the LiveView JavaScript assets (this needs mix and npm available in your path!)
path = Phoenix.LiveView.__info__(:compile)[:source] |> Path.dirname() |> Path.join("../")
System.cmd("mix", ["deps.get"], cd: path, into: IO.binstream())
System.cmd("npm", ["install"], cd: Path.join(path, "./assets"), into: IO.binstream())
System.cmd("mix", ["assets.build"], cd: path, into: IO.binstream())

defmodule Example.ErrorView do
  def render(template, _), do: Phoenix.Controller.status_message_from_template(template)
end

defmodule Example.BadgeForm do
  use Phoenix.LiveComponent

  def mount(socket) do
    socket =
      socket
      |> assign(:type, :huey)

    {:ok, socket}
  end

  def update(assigns, socket) do
    socket =
      socket
      |> assign(:form, assigns.form)

    {:ok, socket}
  end

  def render(assigns) do
    ~H"""
    <div>
      <.form
        for={@form}
        id="foo"
        class="max-w-lg p-8 flex flex-col gap-4"
        phx-change="change"
        phx-submit="submit"
      >
        <.radios type={@type} form={@form} myself={@myself} />
      </.form>
    </div>
    """
  end

  defp radios(assigns) do
    ~H"""
    <fieldset>
      <legend>Radio example:</legend>
      <%= for type <- [:huey, :dewey] do %>
        <div
          phx-click="change-type"
          phx-value-type={type}
          phx-target={@myself}
        >
          <input
            type="radio"
            id={type}
            name="type"
            value={type}
            checked={(@type == type) |> IO.inspect(label: "checked")}
          />
          <label for={type}>{type}</label>
        </div>
      <% end %>
    </fieldset>
    """
  end

  def handle_event("change-type", %{"type" => type}, socket) do
    type = String.to_existing_atom(type)
    socket = assign(socket, :type, type)
    {:noreply, socket}
  end
end

defmodule Example.HomeLive do
  use Phoenix.LiveView, layout: {__MODULE__, :live}

  defp changeset(params) do
    data = %{}

    types = %{
      type: :string
    }

    {data, types}
    |> Ecto.Changeset.cast(params, Map.keys(types))
    |> Ecto.Changeset.validate_required(:type)
  end

  def mount(_params, _session, socket) do
    {:ok, assign(socket, form: to_form(changeset(%{}), as: :foo), payload: nil)}
  end

  def render("live.html", assigns) do
    ~H"""
    <script src="/assets/phoenix/phoenix.js">
    </script>
    <script src="/assets/phoenix_live_view/phoenix_live_view.js">
    </script>
    <%!-- uncomment to use enable tailwind --%>
    <script src="https://cdn.tailwindcss.com">
    </script>
    <script>
      let liveSocket = new window.LiveView.LiveSocket("/live", window.Phoenix.Socket)
      liveSocket.connect()
    </script>
    {@inner_content}
    """
  end

  def render(assigns) do
    ~H"""
    <.live_component
      id="badge_form"
      module={Example.BadgeForm}
      action={@live_action}
      form={@form}
    />
    """
  end

  def handle_event("change", params, socket) do
    {:noreply, socket}
  end

  def handle_event("submit", params, socket) do
    {:noreply, socket}
  end
end

defmodule Example.Router do
  use Phoenix.Router
  import Phoenix.LiveView.Router

  pipeline :browser do
    plug(:accepts, ["html"])
  end

  scope "/", Example do
    pipe_through(:browser)

    live("/", HomeLive, :index)
  end
end

defmodule Example.Endpoint do
  use Phoenix.Endpoint, otp_app: :sample
  socket("/live", Phoenix.LiveView.Socket)

  plug(Plug.Static, from: {:phoenix, "priv/static"}, at: "/assets/phoenix")
  plug(Plug.Static, from: {:phoenix_live_view, "priv/static"}, at: "/assets/phoenix_live_view")

  plug(Example.Router)
end

{:ok, _} = Supervisor.start_link([Example.Endpoint], strategy: :one_for_one)
Process.sleep(:infinity)

@SteffenDE
Copy link
Collaborator

Perfect, thank you!

@hattmarris
Copy link
Author

Sure thing! And FWIW - I'm realizing this is definitely a case of live view changes surfacing a questionable pattern in our code too, so I really appreciate your time.

As an immediate fix, we're going to refactor code in our app to match on _target for the field for the phx-change binding of the form, and do away with the separate phx-click binding for the radios.

@SteffenDE SteffenDE added the bug label Feb 25, 2025
SteffenDE added a commit that referenced this issue Feb 25, 2025
Fixes: #3684.
Relates to: #3652

The issue was that when we locked a form with an input inside where the
input changed, the form was cloned and then later the input inside was
cloned as well, stored inside the already cloned parent. When undoing,
previously the simple patch copied over the PHX_PRIVATE of the cloned
elements, but with the full DOMPatch, the privates of the source element
were copied, discarding the clone of the input. With this change, we check
if we are undoing locks and manually copy over nested clones to ensure
that they can be correctly applied afterwards.
@SteffenDE SteffenDE linked a pull request Feb 25, 2025 that will close this issue
SteffenDE added a commit that referenced this issue Feb 25, 2025
Fixes: #3684.
Relates to: #3652

The issue was that when we locked a form with an input inside where the
input changed, the form was cloned and then later the input inside was
cloned as well, stored inside the already cloned parent. When undoing,
previously the simple patch copied over the PHX_PRIVATE of the cloned
elements, but with the full DOMPatch, the privates of the source element
were copied, discarding the clone of the input. With this change, we check
if we are undoing locks and manually copy over nested clones to ensure
that they can be correctly applied afterwards.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants