Fix: Omit hidden input before checkbox group (#314)

Omit hidden 'reset' input that appears before checkbox group if any checkboxes are checked.

## What changed?
- Better preserve DOM order of form elements better when building form data.

## Why make these changes?
A hidden input before a checkbox group can be used to reset the value if all checkboxes are unchecked.
However, if checkboxes are checked, then the hidden input should be effectively ignored.
Plug/Phoenix can handle this if the field values arrive in DOM order.

In forms like:

```html
<input type="hidden" name="list" value="" />
<input type="checkbox" name="list[]" value="one" />
<input type="checkbox" name="list[]" value="two" />
```

PhoenixTest previously collected controls by selector category and stored `FormData` in a map. That lost DOM order and collapsed repeated field names, which could cause the hidden scalar entry to interfere with the checkbox array payload.

This commit is only a partial improvement. The ideal solution would be to submit form data in the exact DOM order a browser would, end to end.

Author: ftes

lib/phoenix_test/element/form.ex

@@ -69,70 +69,58 @@ defmodule PhoenixTest.Element.Form do
 
   def has_action?(form), do: Utils.present?(form.action)
 
-  @simple_value_types ~w(
-    date
-    datetime-local
-    email
-    month
-    number
-    password
-    range
-    search
-    tel
-    text
-    time
-    url
-    week
-  )
-
-  @hidden_inputs "input[type='hidden']"
-  @checked_radio_buttons "input:not([disabled])[type='radio'][value]:checked"
-  @checked_checkboxes "input:not([disabled])[type='checkbox'][value]:checked"
-  @pre_filled_default_text_inputs "input:not([disabled]):not([type])[value]"
-
-  @pre_filled_simple_value_inputs Enum.map_join(
-                                    @simple_value_types,
-                                    ",",
-                                    &"input:not([disabled])[type='#{&1}'][value]"
-                                  )
+  @enabled_controls ":is(input, textarea, select):not([disabled])[name]"
 
   defp form_data(form) do
-    FormData.new()
-    |> FormData.add_data(form_data(@hidden_inputs, form))
-    |> FormData.add_data(form_data(@checked_radio_buttons, form))
-    |> FormData.add_data(form_data(@checked_checkboxes, form))
-    |> FormData.add_data(form_data(@pre_filled_simple_value_inputs, form))
-    |> FormData.add_data(form_data(@pre_filled_default_text_inputs, form))
-    |> FormData.add_data(form_data_textarea(form))
-    |> FormData.add_data(form_data_select(form))
+    form
+    |> Html.all(@enabled_controls)
+    |> Enum.reduce(FormData.new(), &append_form_field(&2, Html.tag(&1), &1))
   end
 
-  defp form_data(selector, form) do
-    form
-    |> Html.all(selector)
-    |> Enum.map(&to_form_field/1)
+  def put_button_data(form, nil), do: form
+
+  def put_button_data(form, %Button{} = button) do
+    Map.update!(form, :form_data, &FormData.add_data(&1, button))
   end
 
-  defp form_data_textarea(form) do
-    form
-    |> Html.all("textarea:not([disabled])")
-    |> Enum.map(&to_form_field/1)
+  defp append_form_field(form_data, "input", element) do
+    name = Html.attribute(element, "name")
+    value = Html.attribute(element, "value")
+    type = Html.attribute(element, "type")
+
+    cond do
+      !value ->
+        form_data
+
+      type == "hidden" ->
+        FormData.add_data(form_data, name, value)
+
+      type in ~w(radio checkbox) and Html.attribute(element, "checked") ->
+        FormData.add_data(form_data, name, value)
+
+      type in ~w(radio checkbox) ->
+        form_data
+
+      true ->
+        FormData.add_data(form_data, name, value)
+    end
   end
 
-  defp form_data_select(form) do
-    form
-    |> Html.all("select:not([disabled])")
-    |> Enum.flat_map(fn select ->
-      select
-      |> Html.selected_options()
-      |> Enum.map(&to_form_field(select, &1))
-    end)
+  defp append_form_field(form_data, "textarea", element) do
+    FormData.add_data(form_data, to_form_field(element))
   end
 
-  def put_button_data(form, nil), do: form
+  defp append_form_field(form_data, "select", select) do
+    values =
+      select
+      |> Html.selected_options()
+      |> Enum.map(&element_value/1)
 
-  def put_button_data(form, %Button{} = button) do
-    Map.update!(form, :form_data, &FormData.add_data(&1, button))
+    case values do
+      [] -> form_data
+      [value] -> FormData.add_data(form_data, Html.attribute(select, "name"), value)
+      many -> FormData.add_data(form_data, Html.attribute(select, "name"), many)
+    end
   end
 
   defp to_form_field(element) do

lib/phoenix_test/form_data.ex

@@ -5,7 +5,7 @@ defmodule PhoenixTest.FormData do
   alias PhoenixTest.Element.Field
   alias PhoenixTest.Element.Select
 
-  defstruct data: %{}
+  defstruct data: []
 
   def new, do: %__MODULE__{}
 
@@ -35,82 +35,106 @@ defmodule PhoenixTest.FormData do
 
   def add_data(%__MODULE__{} = form_data, name, value) do
     if allows_multiple_values?(name) do
-      new_data =
-        Map.update(form_data.data, name, List.wrap(value), fn existing_value ->
-          if value in existing_value do
-            existing_value
-          else
-            existing_value ++ List.wrap(value)
-          end
-        end)
+      existing_values = values_for_name(form_data.data, name)
+
+      new_entries =
+        value
+        |> List.wrap()
+        |> Enum.reject(&(&1 in existing_values))
+        |> Enum.map(&{name, &1})
 
-      %__MODULE__{form_data | data: new_data}
+      %__MODULE__{form_data | data: form_data.data ++ new_entries}
     else
-      %__MODULE__{form_data | data: Map.put(form_data.data, name, value)}
+      put_data(form_data, name, value)
     end
   end
 
-  def merge(%__MODULE__{data: data1}, %__MODULE__{data: data2}) do
-    data =
-      Map.merge(data1, data2, fn k, v1, v2 ->
-        if allows_multiple_values?(k) do
-          Enum.uniq(v1 ++ v2)
-        else
-          v2
-        end
-      end)
-
-    %__MODULE__{data: data}
+  def merge(%__MODULE__{} = form_data1, %__MODULE__{} = form_data2) do
+    form_data2
+    |> field_names()
+    |> Enum.reduce(form_data1, fn name, acc ->
+      if allows_multiple_values?(name) do
+        add_data(acc, name, get_data(form_data2, name))
+      else
+        put_data(acc, name, get_data(form_data2, name))
+      end
+    end)
   end
 
-  def override(%__MODULE__{data: data1}, %__MODULE__{data: data2}) do
-    %__MODULE__{data: Map.merge(data1, data2)}
+  def override(%__MODULE__{} = form_data1, %__MODULE__{} = form_data2) do
+    form_data2
+    |> field_names()
+    |> Enum.reduce(form_data1, fn name, acc ->
+      put_data(acc, name, get_data(form_data2, name))
+    end)
   end
 
   def get_data(%__MODULE__{data: data}, name) do
-    Map.get(data, name)
+    values = values_for_name(data, name)
+
+    cond do
+      values == [] -> nil
+      allows_multiple_values?(name) or length(values) > 1 -> values
+      true -> hd(values)
+    end
   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)}
+    new_entries =
+      value
+      |> List.wrap()
+      |> Enum.reject(&is_nil/1)
+      |> Enum.map(&{name, &1})
+
+    %__MODULE__{form_data | data: replace_entries(form_data.data, name, new_entries)}
   end
 
   defp allows_multiple_values?(field_name), do: String.ends_with?(field_name, "[]")
 
   def filter(%__MODULE__{data: data}, fun) do
-    data =
-      data
-      |> Enum.filter(fn {name, value} -> fun.(%{name: name, value: value}) end)
-      |> Map.new()
-
-    %__MODULE__{data: data}
+    %__MODULE__{
+      data:
+        Enum.filter(data, fn {name, value} ->
+          fun.(%{name: name, value: value})
+        end)
+    }
   end
 
   def empty?(%__MODULE__{data: data}) do
     Enum.empty?(data)
   end
 
-  def has_data?(%__MODULE__{data: data}, name, value) do
-    field_data = Map.get(data, name, [])
+  def has_data?(%__MODULE__{} = form_data, name, value) do
+    field_data = get_data(form_data, name)
 
     value == field_data or value in List.wrap(field_data)
   end
 
   def field_names(%__MODULE__{data: data}) do
-    Map.keys(data)
+    data
+    |> Enum.map(&elem(&1, 0))
+    |> Enum.uniq()
   end
 
-  def to_list(%__MODULE__{data: data}) do
-    data
-    |> Enum.map(fn
-      {key, values} when is_list(values) ->
-        Enum.map(values, &{key, &1})
+  def to_list(%__MODULE__{data: data}), do: data
 
-      {_key, _value} = field ->
-        field
-    end)
-    |> List.flatten()
+  defp replace_entries(data, name, new_entries) do
+    case Enum.find_index(data, fn {n, _} -> n == name end) do
+      nil ->
+        data ++ new_entries
+
+      first_index ->
+        filtered_data = Enum.reject(data, fn {n, _} -> n == name end)
+        {before, after_entries} = Enum.split(filtered_data, first_index)
+        before ++ new_entries ++ after_entries
+    end
+  end
+
+  defp values_for_name(data, name) do
+    data
+    |> Enum.filter(fn {n, _} -> n == name end)
+    |> Enum.map(fn {_, v} -> v end)
   end
 end

lib/phoenix_test/html.ex

@@ -95,6 +95,13 @@ defmodule PhoenixTest.Html do
     end
   end
 
+  def tag(%LazyHTML{} = html) do
+    case element(html) do
+      {tag, _, _} -> to_string(tag)
+      _ -> nil
+    end
+  end
+
   defp normalize_whitespace(string) do
     String.replace(string, ~r/[\s]+/, " ")
   end

lib/phoenix_test/query.ex

@@ -505,8 +505,8 @@ defmodule PhoenixTest.Query do
   end
 
   defp selected_option_texts(element) do
-    case Html.element(element) do
-      {"select", _attrs, _children} ->
+    case Html.tag(element) do
+      "select" ->
         element
         |> Html.selected_options()
         |> Enum.map(&Html.element_text/1)

test/phoenix_test/element/form_test.exs

@@ -245,6 +245,26 @@ defmodule PhoenixTest.Element.FormTest do
 
       assert FormData.has_data?(form.form_data, "checkbox", "checked")
     end
+
+    test "preserves successful control DOM order in submission entries" do
+      html = """
+      <form id="form">
+        <input type="hidden" name="mixed_items" value="" />
+        <input type="checkbox" name="mixed_items[]" value="one" checked />
+        <input type="text" name="after" value="later" />
+        <input type="checkbox" name="mixed_items[]" value="two" checked />
+      </form>
+      """
+
+      form = Form.find!(html, "form")
+
+      assert FormData.to_list(form.form_data) == [
+               {"mixed_items", ""},
+               {"mixed_items[]", "one"},
+               {"after", "later"},
+               {"mixed_items[]", "two"}
+             ]
+    end
   end
 
   describe "form.submit_button" do

test/phoenix_test/form_data_test.exs

@@ -190,6 +190,29 @@ defmodule PhoenixTest.FormDataTest do
 
       assert FormData.to_list(form_data) == [{"items[]", ""}]
     end
+
+    test "preserves field order when user input for checkbox group overrides hidden input" do
+      base =
+        FormData.new()
+        |> FormData.add_data("mixed_items", "")
+        |> FormData.add_data("mixed_items[]", ["one", "two"])
+        |> FormData.add_data("name", "default")
+
+      override =
+        FormData.new()
+        |> FormData.put_data("mixed_items[]", ["one", "two", "three"])
+        |> FormData.put_data("name", "Bilbo")
+
+      form_data = FormData.override(base, override)
+
+      assert FormData.to_list(form_data) == [
+               {"mixed_items", ""},
+               {"mixed_items[]", "one"},
+               {"mixed_items[]", "two"},
+               {"mixed_items[]", "three"},
+               {"name", "Bilbo"}
+             ]
+    end
   end
 
   describe "filter" do
@@ -249,7 +272,7 @@ defmodule PhoenixTest.FormDataTest do
 
       list = FormData.to_list(form_data)
 
-      assert list == [{"email", "frodo@fellowship.com"}, {"name", "frodo"}]
+      assert list == [{"name", "frodo"}, {"email", "frodo@fellowship.com"}]
     end
 
     test "preserves select options ordering" do

test/phoenix_test/form_payload_test.exs

@@ -2,6 +2,7 @@ defmodule PhoenixTest.FormPayloadTest do
   use ExUnit.Case, async: true
 
   alias PhoenixTest.Element.Form
+  alias PhoenixTest.FormData
   alias PhoenixTest.FormPayload
 
   describe "new" do
@@ -132,6 +133,28 @@ defmodule PhoenixTest.FormPayloadTest do
 
       assert %{"checkbox" => "unchecked"} = FormPayload.new(form.form_data)
     end
+
+    test "preserves array values when hidden scalar and array entries for multiple fields are interleaved" do
+      form_data =
+        FormData.new()
+        |> FormData.add_data("preferences[color]", "")
+        |> FormData.add_data("preferences[color][]", "blue")
+        |> FormData.add_data("preferences[size]", "")
+        |> FormData.add_data("preferences[size][]", "medium")
+        |> FormData.add_data("preferences[tag]", "")
+        |> FormData.add_data("preferences[tag][]", [
+          "new",
+          "sale"
+        ])
+
+      assert %{
+               "preferences" => %{
+                 "color" => ["blue"],
+                 "size" => ["medium"],
+                 "tag" => ["new", "sale"]
+               }
+             } = FormPayload.new(form_data)
+    end
   end
 
   describe "add_form_data" do