Handle `<turbo-stream>` name collisions with `<form>` controls #1037

seanpdoyle · 2023-10-12T21:33:54Z

Closes #879
Closes #929

At the very least, we should provide guidance based on MDN's Issues with Naming Elements:

Some names will interfere with JavaScript access to the form's
properties and elements.

For example:

<input name="id"> will take precedence over <form id="…">. This
means that form.id will not refer to the form's id, but to the
element whose name is "id". This will be the case with any other
form properties, such as <input name="action"> or <input name="post">. <input name="elements"> will render the form's
elements collection inaccessible. The reference form.elements will
now refer to the individual element.

To avoid such problems with element names:

Always use the elements collection to avoid ambiguity between an
element name and a form property.

Never use "elements" as an element name.

To guard against future regressions, this commit adds coverage for the stream.html fixture page by rendering a <form> element with <input type="hidden"> elements whose names match each StreamActions function.

Closes [hotwired#929][] At the very least, we should provide guidance based on MDN's [Issues with Naming Elements][]: > Some names will interfere with JavaScript access to the form's > properties and elements. > > For example: > > `<input name="id">` will take precedence over `<form id="…">`. This > means that `form.id` will not refer to the form's `id`, but to the > element whose `name` is `"id"`. This will be the case with any other > form properties, such as `<input name="action">` or `<input > name="post">`. `<input name="elements">` will render the form's > `elements` collection inaccessible. The reference `form.elements` will > now refer to the individual element. > > To avoid such problems with element names: > > * Always use the `elements` collection to avoid ambiguity between an > element `name` and a form property. > * Never use `"elements"` as an element name. To guard against future regressions, this commit adds coverage for the `stream.html` fixture page by rendering a `<form>` element with `<input type="hidden">` elements whose names match each `StreamActions` function. [hotwired#929]: hotwired#929 [Issues with Naming Elements]: https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement#issues_with_naming_elements

seanpdoyle · 2023-10-12T21:36:44Z

To re-iterate #929 (comment):

I'm not sure if there's a predictable way to work around the underlying problem. One thing we could do is detect a collision (with something like typeof e.remove === "function"), then log a warning and no-op.

It seems that any other counter-measure is kicking the can down the road. For example, e.remove() could be replaced by e.parentElement.removeChild(e), but if there's a form field named parentElement, we're back to square one. While it's unlikely, there's nothing preventing a re-ordering/sorting <form> element from having <input name="parentElement">.

seanpdoyle · 2023-10-12T23:02:38Z

src/tests/functional/stream_tests.js

+test("test receiving an [action=after] targeting a form", async ({ page }) => {
+  await page.evaluate(() =>
+    window.Turbo.renderStreamMessage(`
+      <turbo-stream action="after" target="form_with_action_names">
+        <template><p id="after_form_with_action_names">After</p></template>
+      </turbo-stream>
+    `)
+  )
+
+  assert.equal(await page.textContent("#after_form_with_action_names"), "After")
+})
+
+test("test receiving an [action=append] targeting a form", async ({ page }) => {
+  await page.evaluate(() =>
+    window.Turbo.renderStreamMessage(`
+      <turbo-stream action="append" target="form_with_action_names">
+        <template>Append</template>
+      </turbo-stream>
+    `)
+  )
+  await nextBeat()
+
+  assert.include(await page.textContent("#form_with_action_names"), "Append")
+})
+
+test("test receiving an [action=before] targeting a form", async ({ page }) => {
+  await page.evaluate(() =>
+    window.Turbo.renderStreamMessage(`
+      <turbo-stream action="before" target="form_with_action_names">
+        <template><p id="before_form_with_action_names">Before</p></template>
+      </turbo-stream>
+    `)
+  )
+
+  assert.equal(await page.textContent("#before_form_with_action_names"), "Before")
+})
+
+test("test receiving an [action=prepend] targeting a form", async ({ page }) => {
+  await page.evaluate(() =>
+    window.Turbo.renderStreamMessage(`
+      <turbo-stream action="prepend" target="form_with_action_names">
+        <template>Prepend</template>
+      </turbo-stream>
+    `)
+  )
+  await nextBeat()
+
+  assert.include(await page.textContent("#form_with_action_names"), "Prepend")
+})
+
+test("test receiving an [action=remove] targeting a form", async ({ page }) => {
+  await page.evaluate(() =>
+    window.Turbo.renderStreamMessage(`
+      <turbo-stream action="remove" target="form_with_action_names"></turbo-stream>
+    `)
+  )
+
+  assert.notOk(await waitUntilNoSelector(page, "#form_with_action_names"), "removes target element")
+})
+
+test("test receiving an [action=replace] targeting a form", async ({ page }) => {
+  await page.evaluate(() =>
+    window.Turbo.renderStreamMessage(`
+      <turbo-stream action="replace" target="form_with_action_names">
+        <template><div id="form_with_action_names"></div></template>
+      </turbo-stream>
+    `)
+  )
+
+  assert.notOk(await hasSelector(page, "form#form_with_action_names"))
+  assert.ok(await hasSelector(page, "div#form_with_action_names"))
+})
+
+test("test receiving an [action=update] targeting a form", async ({ page }) => {
+  await page.evaluate(() =>
+    window.Turbo.renderStreamMessage(`
+      <turbo-stream action="update" target="form_with_action_names">
+        <template><input name="updated_field"></template>
+      </turbo-stream>
+    `)
+  )
+  await nextBeat()
+
+  assert.ok(await hasSelector(page, "#form_with_action_names [name=updated_field]"))
+})
+


These tests should probably be covered by unit tests, like in https://github.com/hotwired/turbo/pull/880/files#diff-f9df76d4df4de3858e7298e7f36d9e3acc8e7f07af6759c4f66c80bcc309b465.

brunoprietog · 2023-12-09T10:19:01Z

#880 is trying to solve this too

seanpdoyle mentioned this pull request Oct 12, 2023

broadcast_remove_to conflicts with input field named "remove" #929

Open

This was referenced Oct 12, 2023

Stream append/prepend can produce duplicates if the template element is a form #879

Closed

Use getAttribute to find duplicate ids, not the id property #880

Merged

seanpdoyle commented Oct 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle `<turbo-stream>` name collisions with `<form>` controls #1037

Handle `<turbo-stream>` name collisions with `<form>` controls #1037

seanpdoyle commented Oct 12, 2023 •

edited

Loading

seanpdoyle commented Oct 12, 2023

seanpdoyle Oct 12, 2023

brunoprietog commented Dec 9, 2023

Handle <turbo-stream> name collisions with <form> controls #1037

Are you sure you want to change the base?

Handle <turbo-stream> name collisions with <form> controls #1037

Conversation

seanpdoyle commented Oct 12, 2023 • edited Loading

seanpdoyle commented Oct 12, 2023

seanpdoyle Oct 12, 2023

Choose a reason for hiding this comment

brunoprietog commented Dec 9, 2023

Handle `<turbo-stream>` name collisions with `<form>` controls #1037

Handle `<turbo-stream>` name collisions with `<form>` controls #1037

seanpdoyle commented Oct 12, 2023 •

edited

Loading