Fix uncheck for checkboxes with array name (#306)

## Problem

I've seeing a bug in which uncheck doesn't work as expected with checkboxes with array names like:

<input type="checkbox" name="items[]" value="one" />

## Issue

The issue was that `FormData.merge`combined the previous value with the newly computed value for array-named checkboxes.  In the uncheck case, that meant unchecked values could remain present because they were still in the old state. 

## Fix 

This commit introduces `FormData.override`, which replaces the old value with a newly computed one + logic to calculate this new state for checkboxes with array-like names. It also adds tests reproducing the issue.

Author: katafrakt

lib/phoenix_test/active_form.ex

@@ -15,8 +15,8 @@ defmodule PhoenixTest.ActiveForm do
     struct!(%__MODULE__{}, opts)
   end
 
-  def add_form_data(%__MODULE__{} = active_form, new_form_data) do
-    Map.update!(active_form, :form_data, &FormData.add_data(&1, new_form_data))
+  def put_form_data(%__MODULE__{} = active_form, name, value) do
+    Map.update!(active_form, :form_data, &FormData.put_data(&1, name, value))
   end
 
   def add_upload(%__MODULE__{} = active_form, new_upload) do

lib/phoenix_test/field_helpers.ex

@@ -0,0 +1,76 @@
+defmodule PhoenixTest.FieldHelpers do
+  @moduledoc false
+
+  alias PhoenixTest.ActiveForm
+  alias PhoenixTest.FormData
+  alias PhoenixTest.Html
+
+  # Computes the value to store in active_form for a field interaction.
+  # For array-named fields (name ending in "[]"), values are accumulated/removed
+  # from the existing list rather than replaced wholesale. This mirrors browser
+  # behavior where each checkbox toggle adds/removes its value from the array.
+  def next_field_value(session, form, field) do
+    current_form_data = current_form_data(session, form)
+
+    case {session.current_operation.name, field} do
+      {:check, %{name: name, value: value}} ->
+        if multiple_values_name?(name) do
+          current_form_data
+          |> FormData.get_data(name)
+          |> List.wrap()
+          |> Kernel.++([value])
+          |> Enum.uniq()
+        else
+          value
+        end
+
+      {:uncheck, %{name: name, parsed: parsed} = current_field} ->
+        if multiple_values_name?(name) do
+          checked_value = Html.attribute(parsed, "value") || "on"
+
+          current_form_data
+          |> FormData.get_data(name)
+          |> List.wrap()
+          |> Enum.reject(&(&1 == checked_value))
+        else
+          current_field.value
+        end
+
+      {_, %{name: name, value: values}} ->
+        if multiple_values_name?(name) and is_list(values) do
+          current_form_data
+          |> FormData.get_data(name)
+          |> List.wrap()
+          |> Kernel.++(values)
+          |> Enum.uniq()
+        else
+          values
+        end
+
+      _ ->
+        field.value
+    end
+  end
+
+  def current_form_data(session, form) do
+    if session.active_form.selector == form.selector do
+      FormData.override(form.form_data, session.active_form.form_data)
+    else
+      form.form_data
+    end
+  end
+
+  def active_form_for(active_form, form) do
+    if active_form.selector == form.selector do
+      active_form
+    else
+      ActiveForm.new(id: form.id, selector: form.selector)
+    end
+  end
+
+  def multiple_values_name?(name) when is_binary(name) do
+    String.ends_with?(name, "[]")
+  end
+
+  def multiple_values_name?(_name), do: false
+end

lib/phoenix_test/form_data.ex

@@ -63,6 +63,20 @@ defmodule PhoenixTest.FormData do
     %__MODULE__{data: data}
   end
 
+  def override(%__MODULE__{data: data1}, %__MODULE__{data: data2}) do
+    %__MODULE__{data: Map.merge(data1, data2)}
+  end
+
+  def get_data(%__MODULE__{data: data}, name) do
+    Map.get(data, name)
+  end
+
+  def put_data(%__MODULE__{} = form_data, name, value) when is_nil(name) or is_nil(value), do: form_data
+
+  def put_data(%__MODULE__{} = form_data, name, value) do
+    %__MODULE__{form_data | data: Map.put(form_data.data, name, value)}
+  end
+
   defp allows_multiple_values?(field_name), do: String.ends_with?(field_name, "[]")
 
   def filter(%__MODULE__{data: data}, fun) do

lib/phoenix_test/live.ex

@@ -1,6 +1,7 @@
 defmodule PhoenixTest.Live do
   @moduledoc false
   import Phoenix.LiveViewTest
+  import PhoenixTest.FieldHelpers
   import PhoenixTest.SessionHelpers, only: [scope_selector: 2]
 
   alias PhoenixTest.ActiveForm
@@ -135,7 +136,7 @@ defmodule PhoenixTest.Live do
 
         form_data =
           if active_form.selector == form.selector do
-            FormData.merge(form.form_data, active_form.form_data)
+            FormData.override(form.form_data, active_form.form_data)
           else
             form.form_data
           end
@@ -163,7 +164,7 @@ defmodule PhoenixTest.Live do
 
     existing_form_data =
       if session.active_form.selector == form.selector do
-        FormData.merge(form.form_data, session.active_form.form_data)
+        FormData.override(form.form_data, session.active_form.form_data)
       else
         form.form_data
       end
@@ -438,16 +439,13 @@ defmodule PhoenixTest.Live do
     Field.validate_name!(field)
 
     form = Field.parent_form!(field, html)
+    field_value = next_field_value(session, form, field)
 
     session =
       Map.update!(session, :active_form, fn active_form ->
-        if active_form.selector == form.selector do
-          ActiveForm.add_form_data(active_form, field)
-        else
-          [id: form.id, selector: form.selector]
-          |> ActiveForm.new()
-          |> ActiveForm.add_form_data(field)
-        end
+        active_form
+        |> active_form_for(form)
+        |> ActiveForm.put_form_data(field.name, field_value)
       end)
 
     maybe_trigger_phx_change(session, form, field)
@@ -495,7 +493,7 @@ defmodule PhoenixTest.Live do
 
   defp merged_form_data(session, %Form{} = form) do
     form.form_data
-    |> FormData.merge(session.active_form.form_data)
+    |> FormData.override(session.active_form.form_data)
     |> FormData.filter(fn %{name: name} -> name in Form.form_element_names(form) end)
   end
 
@@ -521,7 +519,7 @@ defmodule PhoenixTest.Live do
     form = Form.find!(session.current_operation.html, selector)
 
     form_data = remove_data_for_fields_that_have_been_removed(form_data, form)
-    form_data = FormData.merge(form.form_data, form_data)
+    form_data = FormData.override(form.form_data, form_data)
 
     additional_data =
       if form.submit_button do
@@ -625,7 +623,9 @@ defmodule PhoenixTest.Live do
       {:found, form} ->
         active_form = session.active_form
         active_form? = form.selector == active_form.selector
-        form_data = FormData.merge(form.form_data, if(active_form?, do: active_form.form_data, else: FormData.new()))
+
+        form_data =
+          FormData.override(form.form_data, if(active_form?, do: active_form.form_data, else: FormData.new()))
 
         %{session.conn | resp_body: html}
         |> PhoenixTest.Static.build()

lib/phoenix_test/static.ex

@@ -2,6 +2,7 @@ defmodule PhoenixTest.Static do
   @moduledoc false
 
   import Phoenix.ConnTest
+  import PhoenixTest.FieldHelpers
 
   alias ExUnit.AssertionError
   alias PhoenixTest.ActiveForm
@@ -246,7 +247,7 @@ defmodule PhoenixTest.Static do
         Form.put_button_data(form, form.submit_button)
       end)
 
-    to_submit = FormPayload.new(FormData.merge(form.form_data, form_data))
+    to_submit = FormPayload.new(FormData.override(form.form_data, form_data))
 
     session
     |> Map.put(:active_form, ActiveForm.new())
@@ -278,15 +279,12 @@ defmodule PhoenixTest.Static do
   defp fill_in_field_data(session, field) do
     Field.validate_name!(field)
     form = Field.parent_form!(field, session.current_operation.html)
+    field_value = next_field_value(session, form, field)
 
     Map.update!(session, :active_form, fn active_form ->
-      if active_form.selector == form.selector do
-        ActiveForm.add_form_data(active_form, field)
-      else
-        [id: form.id, selector: form.selector]
-        |> ActiveForm.new()
-        |> ActiveForm.add_form_data(field)
-      end
+      active_form
+      |> active_form_for(form)
+      |> ActiveForm.put_form_data(field.name, field_value)
     end)
   end
 
@@ -315,7 +313,7 @@ defmodule PhoenixTest.Static do
 
   defp build_payload(form, active_form \\ ActiveForm.new()) do
     form.form_data
-    |> FormData.merge(active_form.form_data)
+    |> FormData.override(active_form.form_data)
     |> FormPayload.new()
     |> FormPayload.add_form_data(active_form.uploads)
   end

test/phoenix_test/active_form_test.exs

@@ -4,12 +4,12 @@ defmodule PhoenixTest.ActiveFormTest do
   alias PhoenixTest.ActiveForm
   alias PhoenixTest.FormData
 
-  describe "add_form_data" do
-    test "adds form data passed" do
+  describe "put_form_data" do
+    test "puts form data with given name and value" do
       active_form =
         [id: "user-form", selector: "#user-form"]
         |> ActiveForm.new()
-        |> ActiveForm.add_form_data({"user[name]", "Frodo"})
+        |> ActiveForm.put_form_data("user[name]", "Frodo")
 
       assert FormData.has_data?(active_form.form_data, "user[name]", "Frodo")
     end

test/phoenix_test/form_data_test.exs

@@ -176,6 +176,22 @@ defmodule PhoenixTest.FormDataTest do
     end
   end
 
+  describe "override" do
+    test "replaces list values when same key is present" do
+      base =
+        FormData.new()
+        |> FormData.add_data("items[]", "")
+        |> FormData.add_data("items[]", "one")
+
+      override =
+        FormData.put_data(FormData.new(), "items[]", [""])
+
+      form_data = FormData.override(base, override)
+
+      assert FormData.to_list(form_data) == [{"items[]", ""}]
+    end
+  end
+
   describe "filter" do
     test "filters form data based on function provivded" do
       form_data =

test/phoenix_test/live_test.exs

@@ -663,6 +663,17 @@ defmodule PhoenixTest.LiveTest do
       |> assert_has("#form-data", text: "checkbox_group: [1, 2]")
     end
 
+    test "adds checked values for array named checkboxes without replacing existing values", %{conn: conn} do
+      conn
+      |> visit("/live/index")
+      |> within("#array-checkbox-form", fn session ->
+        check(session, "Three")
+      end)
+      |> assert_has("#form-data", text: "one")
+      |> assert_has("#form-data", text: "two")
+      |> assert_has("#form-data", text: "three")
+    end
+
     test "check triggers phx-change on the input if it is defined", %{conn: conn} do
       conn
       |> visit("/live/index")
@@ -784,6 +795,28 @@ defmodule PhoenixTest.LiveTest do
       |> assert_has("#form-data", text: "like-elixir: no")
     end
 
+    test "removes checked values from array named checkboxes on change", %{conn: conn} do
+      conn
+      |> visit("/live/index")
+      |> within("#array-checkbox-form", fn session ->
+        uncheck(session, "One")
+      end)
+      |> refute_has("#form-data", text: "one")
+      |> assert_has("#form-data", text: "two")
+    end
+
+    test "removes checked values from array named checkboxes on submit", %{conn: conn} do
+      conn
+      |> visit("/live/index")
+      |> within("#array-checkbox-form", fn session ->
+        session
+        |> uncheck("One")
+        |> submit()
+      end)
+      |> refute_has("#form-data", text: "one")
+      |> assert_has("#form-data", text: "two")
+    end
+
     test "raises error if checkbox doesn't have phx-click or belong to form", %{conn: conn} do
       session = visit(conn, "/live/index")
 

test/phoenix_test/static_test.exs

@@ -545,6 +545,19 @@ defmodule PhoenixTest.StaticTest do
       |> assert_has("#form-data", text: "admin: on")
     end
 
+    test "adds checked values for array named checkboxes without replacing existing values", %{conn: conn} do
+      conn
+      |> visit("/page/index")
+      |> within("#array-checkbox-form", fn session ->
+        check(session, "Three")
+      end)
+      |> submit()
+      |> assert_has("#form-data", text: "items: [")
+      |> assert_has("#form-data", text: "one")
+      |> assert_has("#form-data", text: "two")
+      |> assert_has("#form-data", text: "three")
+    end
+
     test "handle checkbox name with '?'", %{conn: conn} do
       conn
       |> visit("/page/index")
@@ -616,6 +629,17 @@ defmodule PhoenixTest.StaticTest do
       |> refute_has("#form-data", text: "like-elixir: yes")
       |> assert_has("#form-data", text: "like-elixir: no")
     end
+
+    test "removes checked values from array named checkboxes", %{conn: conn} do
+      conn
+      |> visit("/page/index")
+      |> within("#array-checkbox-form", fn session ->
+        uncheck(session, "One")
+      end)
+      |> submit()
+      |> refute_has("#form-data", text: "one")
+      |> assert_has("#form-data", text: "two")
+    end
   end
 
   describe "choose/3" do

test/support/web_app/index_live.ex

@@ -370,6 +370,21 @@ defmodule PhoenixTest.WebApp.IndexLive do
       <button type="submit">Save</button>
     </form>
 
+    <form id="array-checkbox-form" phx-change="change-form" phx-submit="change-form">
+      <input type="hidden" name="items[]" value="" />
+
+      <label for="array-item-one">One</label>
+      <input id="array-item-one" type="checkbox" name="items[]" value="one" checked />
+
+      <label for="array-item-two">Two</label>
+      <input id="array-item-two" type="checkbox" name="items[]" value="two" checked />
+
+      <label for="array-item-three">Three</label>
+      <input id="array-item-three" type="checkbox" name="items[]" value="three" />
+
+      <button type="submit">Save Array Checkbox Form</button>
+    </form>
+
     <form id="same-labels" phx-submit="save-form" phx-change="change-form">
       <fieldset name="like-elixir">
         <legend>Do you like Elixir:</legend>