Deep clone form for form recovery (#3921)

SteffenDE · gmazzamuto · web-flow · commit ce3107c0f52f · 2025-08-01T23:47:39.000+02:00
The previous algorithm had a flaw: ```javascript const clonedForm = form.cloneNode(false); DOM.copyPrivates(clonedForm, form); Array.from(form.elements).forEach((el) => { morphdom(clonedEl, el); DOM.copyPrivates(clonedEl, el); clonedForm.appendChild(clonedEl); } ``` While this kept the size of the clonedForm minimal and includes all elements, it did not consider `<fieldset>` elements. If you had something like this: ```html <form> <fieldset> <input name="foo" value="bar"> </fieldset> </form> ``` `form.elements` would return `[fieldset, input]`, thus we would actually include the input twice. Furthermore, imagine someone did `<fieldset disabled>` which disables all inputs inside and should exclude them from the form payload. But with the old code, LiveView would copy the input separately. Because it is not disabled, it would be included in the recovery payload. The fix is to actually copy the whole form and then query the DOM for any associated elements `[form=${form.id}]` to include them. In the future, we should consider simplifying this code to serialize the form early and not need to clone DOM nodes, but for now, I think it is fine. Closes #3914. Co-authored-by: Giacomo Mazzamuto <g.mazzamuto@gmail.com>
diff --git a/assets/js/phoenix_live_view/view.js b/assets/js/phoenix_live_view/view.js
@@ -2061,6 +2061,19 @@ export default class View {
   }
 
   getFormsForRecovery() {
+    // Form recovery is complex in LiveView:
+    // We want to support nested LiveViews and also provide a good user experience.
+    // Therefore, when the channel rejoins, we copy all forms that are eligible for
+    // recovery to be able to access them later.
+    // Why do we need to copy them? Because when the main LiveView joins, any forms
+    // in nested LiveViews would be lost.
+    //
+    // We should rework this in the future to serialize the form payload here
+    // instead of cloning the DOM nodes, but making this work correctly is tedious,
+    // as sending the correct form payload relies on JS.push to extract values
+    // from JS commands (phx-change={JS.push("event", value: ..., target: ...)}),
+    // as well as view.pushInput, which expects DOM elements.
+
     if (this.joinCount === 0) {
       return {};
     }
@@ -2075,19 +2088,33 @@ export default class View {
           form.getAttribute(this.binding(PHX_AUTO_RECOVER)) !== "ignore",
       )
       .map((form) => {
-        // we perform a shallow clone and manually copy all elements
-        const clonedForm = form.cloneNode(false);
-        // we need to copy the private data as it contains
-        // the information about touched fields
-        DOM.copyPrivates(clonedForm, form);
-        Array.from(form.elements).forEach((el) => {
-          // we need to clone all child nodes as well,
-          // because those could also be selects
+        // We need to clone the whole form, as relying on form.elements can lead to
+        // situations where we have
+        //
+        //   <form><fieldset disabled><input name="foo" value="bar"></fieldset></form>
+        //
+        // and form.elements returns both the fieldset and the input separately.
+        // Because the fieldset is disabled, the input should NOT be sent though.
+        // We can only reliably serialize the form by cloning it fully.
+        const clonedForm = form.cloneNode(true);
+        // we call morphdom to copy any special state
+        // like the selected option of a <select> element;
+        // any also copy over privates (which contain information about touched fields)
+        morphdom(clonedForm, form, {
+          onBeforeElUpdated: (fromEl, toEl) => {
+            DOM.copyPrivates(fromEl, toEl);
+            return true;
+          },
+        });
+        // next up, we also need to clone any elements with form="id" parameter
+        const externalElements = document.querySelectorAll(
+          `[form="${form.id}"]`,
+        );
+        Array.from(externalElements).forEach((el) => {
+          if (form.contains(el)) {
+            return;
+          }
           const clonedEl = el.cloneNode(true);
-          // we call morphdom to copy any special state
-          // like the selected option of a <select> element;
-          // this should be plenty fast as we call it on a small subset of the DOM,
-          // single inputs or a select with children
           morphdom(clonedEl, el);
           DOM.copyPrivates(clonedEl, el);
           clonedForm.appendChild(clonedEl);
diff --git a/test/e2e/support/form_dynamic_inputs_live.ex b/test/e2e/support/form_dynamic_inputs_live.ex
@@ -76,38 +76,40 @@ defmodule Phoenix.LiveViewTest.E2E.FormDynamicInputsLive do
       phx-submit="save"
       style="display: flex; flex-direction: column; gap: 4px; max-width: 500px;"
     >
-      <input
-        type="text"
-        id={@form[:name].id}
-        name={@form[:name].name}
-        value={@form[:name].value}
-        placeholder="name"
-      />
-      <.inputs_for :let={ef} field={@form[:users]} default={[]}>
-        <div style="padding: 4px; border: 1px solid gray;">
-          <input type="hidden" name="my_form[users_sort][]" value={ef.index} />
-          <input
-            type="text"
-            id={ef[:name].id}
-            name={ef[:name].name}
-            value={ef[:name].value}
-            placeholder="name"
-          />
+      <fieldset>
+        <input
+          type="text"
+          id={@form[:name].id}
+          name={@form[:name].name}
+          value={@form[:name].value}
+          placeholder="name"
+        />
+        <.inputs_for :let={ef} field={@form[:users]} default={[]}>
+          <div style="padding: 4px; border: 1px solid gray;">
+            <input type="hidden" name="my_form[users_sort][]" value={ef.index} />
+            <input
+              type="text"
+              id={ef[:name].id}
+              name={ef[:name].name}
+              value={ef[:name].value}
+              placeholder="name"
+            />
 
-          <button
-            :if={!@checkboxes}
-            type="button"
-            name="my_form[users_drop][]"
-            value={ef.index}
-            phx-click={JS.dispatch("change")}
-          >
-            Remove
-          </button>
-          <label :if={@checkboxes}>
-            <input type="checkbox" name="my_form[users_drop][]" value={ef.index} /> Remove
-          </label>
-        </div>
-      </.inputs_for>
+            <button
+              :if={!@checkboxes}
+              type="button"
+              name="my_form[users_drop][]"
+              value={ef.index}
+              phx-click={JS.dispatch("change")}
+            >
+              Remove
+            </button>
+            <label :if={@checkboxes}>
+              <input type="checkbox" name="my_form[users_drop][]" value={ef.index} /> Remove
+            </label>
+          </div>
+        </.inputs_for>
+      </fieldset>
 
       <input type="hidden" name="my_form[users_drop][]" />
 
diff --git a/test/e2e/support/form_live.ex b/test/e2e/support/form_live.ex
@@ -141,8 +141,10 @@ defmodule Phoenix.LiveViewTest.E2E.FormLive do
       phx-target={assigns[:"phx-target"]}
       class="myformclass"
     >
-      <input type="text" name="a" readonly value={@params["a"]} />
-      <input type="text" name="b" value={@params["b"]} />
+      <fieldset disabled={@params["disabled-fieldset"]}>
+        <input type="text" name="a" readonly value={@params["a"]} />
+        <input type="text" name="b" value={@params["b"]} />
+      </fieldset>
       <input
         type="text"
         name="c"
diff --git a/test/e2e/tests/forms.spec.js b/test/e2e/tests/forms.spec.js
@@ -263,6 +263,48 @@ for (const path of ["/form/nested", "/form"]) {
           ]),
         );
       });
+
+      test("respects disabled state of a fieldset", async ({ page }) => {
+        let webSocketEvents = [];
+        page.on("websocket", (ws) => {
+          ws.on("framesent", (event) =>
+            webSocketEvents.push({ type: "sent", payload: event.payload }),
+          );
+          ws.on("framereceived", (event) =>
+            webSocketEvents.push({ type: "received", payload: event.payload }),
+          );
+          ws.on("close", () => webSocketEvents.push({ type: "close" }));
+        });
+
+        await page.goto(path + "?disabled-fieldset=true&" + additionalParams);
+        await syncLV(page);
+
+        await page.locator("input[name=c]").fill("hello world");
+        await page.locator("select[name=d]").selectOption("bar");
+        await expect(page.locator("input[name=c]")).toBeFocused();
+        await syncLV(page);
+
+        await page.evaluate(
+          () => new Promise((resolve) => window.liveSocket.disconnect(resolve)),
+        );
+        await expect(page.locator(".phx-loading")).toHaveCount(1);
+
+        webSocketEvents = [];
+        await page.evaluate(() => window.liveSocket.connect());
+        await syncLV(page);
+        await expect(page.locator(".phx-loading")).toHaveCount(0);
+
+        expect(webSocketEvents).toEqual(
+          expect.arrayContaining([
+            { type: "sent", payload: expect.stringContaining("phx_join") },
+            { type: "received", payload: expect.stringContaining("phx_reply") },
+            {
+              type: "sent",
+              payload: expect.stringMatching(/event.*"c=hello\+world&d=bar"/),
+            },
+          ]),
+        );
+      });
     });
   }
 
@@ -604,6 +646,46 @@ test("can dynamically add/remove inputs using checkboxes", async ({ page }) => {
   );
 });
 
+// this happened from v1.0.17 when the sort_params field is nested in a fieldset
+test("form recovery does not create duplicates of dynamically added fields", async ({
+  page,
+}) => {
+  await page.goto("/form/dynamic-inputs");
+  await syncLV(page);
+
+  const formData = () =>
+    page
+      .locator("form")
+      .evaluate((form) => Object.fromEntries(new FormData(form).entries()));
+
+  expect(await formData()).toEqual({
+    "my_form[name]": "",
+    "my_form[users_drop][]": "",
+  });
+
+  await page.locator("#my-form_name").fill("Test");
+  await page.getByRole("button", { name: "add more" }).click();
+  await syncLV(page);
+
+  expect(await formData()).toEqual(
+    expect.objectContaining({
+      "my_form[name]": "Test",
+      "my_form[users][0][name]": "",
+    }),
+  );
+
+  await page.evaluate(
+    () => new Promise((resolve) => window.liveSocket.disconnect(resolve)),
+  );
+
+  await page.evaluate(() => window.liveSocket.connect());
+  await syncLV(page);
+
+  const data = await formData();
+
+  expect("my_form[users][1][name]" in data).toBe(false);
+});
+
 // phx-feedback-for was removed in LiveView 1.0, but we still test the shim applied in
 // test_helper.exs layout for backwards compatibility
 test("phx-no-feedback is applied correctly for backwards-compatible-shims", async ({
