Send link as submitter to confirmMethod. Fixes #811

[excid3]

Since a[data-turbo-confirm] dynamically generates a form, there is no way to access the original element.

This PR stashes the original element on the generated form and passes it as the submitter to mimic form submitters.

<a href="#" data-turbo-confirm="Are you sure?">Delete</a>

window.Turbo.setConfirmMethod((message, element, submitter) => {
  // submitter is <a href="#" data-turbo-confirm="Are you sure?">Delete</a>
})

Fixes #811

[seanpdoyle]

I'm unsure about unintended side effects of introducing a property on Element. Is there another place we could stash this reference?

[excid3]

I couldn't think of any other good place to store the reference. Open to suggestions.

Might be better to have a turbo specific name here like turboConfirmElement so that it wouldn't clash with any other properties?

[marcoroth]

Here's an idea to resolve this: excid3#2

[seanpdoyle]

What do you think about actually writing to the SubmitEvent.submitter property?

form.addEventListener("submit", (event: SubmitEvent) => event.submitter = link, { once: true }

That way, we don't need to do any type widening or property-stashing.

The SubmitEvent.submitter is read-only, so this might not be viable. It's worth experimenting with!

[marcoroth]

That might even be cleaner, given you can override submitter and it's not being lost in-between.

[seanpdoyle]

If that doesn't work, you could try passing it to HTMLFormElement.requestSubmit as an argument:

requestAnimationFrame(() => form.requestSubmit(link))

If requestSubmit rejects the link argument because it's an HTMLAnchorElement, and the only reason we're passing it along is to make its content and attributes available to the confirmation callback, what do you think about creating a <button> element that mimics the <a>?

const submitter = document.createElement("button")
submitter.textContent = link.textContent

for (const {name, value} of link.attributes) {
  button.setAttribute(name, value)
}

requestAnimationFrame(() => form.requestSubmit(button))

[excid3]

If browser currently allow this, but start enforcing the types in the future, this could break (and possibly only in some browsers).

@marcoroth's idea of adding attributes and a querySelector seems like a safer solution?

[seanpdoyle]

@excid3 is #856 (comment) a future-compatible technique?

Its approach matches the style from the rest of the transformation logic, and it circumvents the need to stash-then-read.

[excid3]

I think so, but it still lose access to the original element that way.

It does solve the problem of passing the attributes along, so it's probably good enough™. I can't foresee a reason to need the original element personally.

I'll refactor to use that.

[seanpdoyle]

Thank you for making this change! Could we add a test to the suite to exercise this behavior and guard against future regressions?

[seanpdoyle]

I'm surprised that the TypeScript compiler allows this kind of direct access without widening the HTMLFormElement type. Am I missing something here, or misunderstanding this change?

[excid3]

This is forsubmitter which is _submitter: HTMLElement | undefined, so that shouldn't need changing right?

[seanpdoyle]

I think the submitter: type makes sense. How does the compiler understand the HTMLFormElement.originalElement property? That assignment is happening in an entirely different file, and there aren't any type annotations in the diff.

[seanpdoyle]

Even if the argument expects undefined, I'm surprised you aren't getting compile time errors for accessing the property.

[excid3]

Oh yeah, I'm not sure why that is either. 🤔

[excid3]

I'm not too familiar with TypeScript, any advice on how I should address this?

[marcoroth]

this.formElement.originalElement is typed any which technically is assignable to anything, even though it might not make sense

[excid3]

Can you think of a better way to test this? My first thought was having the confirm method save the submitter and retrieve it later. Seems like it could be done simpler some other way.

await page.evaluate(() => window.Turbo.setConfirmMethod((message, form, submitter) => {
  window.turboConfirmSubmitter = submitter
  Promise.resolve(confirm("Overridden message"))
}))
await page.click("#link-method-inside-frame-with-confirmation")
await nextBeat()
const id = await page.evaluate("window.turboConfirmSubmitter.id")
assert.equal(id, "link-method-inside-frame-with-confirmation")

[seanpdoyle]

@excid3 what about incorporating the submitted ID into the confirm message?

diff --git a/src/tests/functional/form_submission_tests.ts b/src/tests/functional/form_submission_tests.ts
index 56cd951..5d1184a 100644
--- a/src/tests/functional/form_submission_tests.ts
+++ b/src/tests/functional/form_submission_tests.ts
@@ -971,6 +971,24 @@ test("test link method form submission inside frame with confirmation cancelled"
   assert.notOk(await hasSelector(page, "#frame div.message"), "Not confirming form submission does not submit the form")
 })
 
+test("test link method form submission with custom confirmation", async ({ page }) => {
+  page.on("dialog", (dialog) => {
+    assert.equal(dialog.message(), "Submitter: #link-method-inside-frame-with-confirmation")
+    dialog.accept()
+  })
+
+  await page.evaluate(() =>
+    window.Turbo.setConfirmMethod((_message, _form, submitter) =>
+      Promise.resolve(submitter ? confirm(`Submitter: #${submitter.id}`) : false)
+    )
+  )
+
+  await page.click("#link-method-inside-frame-with-confirmation")
+  await nextBeat()
+
+  assert.equal(await page.textContent("#frame div.message"), "Link!")
+})
+
 test("test link method form submission outside frame", async ({ page }) => {
   await page.click("#link-method-outside-frame")
   await nextBody(page)

[seanpdoyle]

@excid3 with the issue of testing resolved, I'd like to take this opportunity to get up on my soapbox and vent a little bit.

With the re-introduction of the <a> based <form> submissions, Turbo is responsible for:

• creating a <form> element
• transforming each query parameter in the a[href] value into an <input type="hidden"> element, and inserting that element into the <form>
• transforming the a[href] value into a String without and query parameters, and assigning that to the <form> element's [action] attribute
• making sure the form declares [data-turbo="true"] so that it is handled by Turbo, just in case it's inserted into the document in an area that has otherwise has Turbo disabled
• making sure the <form> is visually hidden with [hidden] so that it doesn't flash visually during submission
• transforming a[data-turbo-method] into form[method]
• ensuring the presence or absence of form[data-turbo-action] based on a[data-turbo-action]
• ensuring the presence or absence of form[data-turbo-confirm] based on a[data-turbo-confirm]
• ensuring the presence or absence of form[data-turbo-stream] based on a[data-turbo-stream] which normally only affects GET requests in case a[data-turbo-method="GET"], which is an entirely counter productive yet possible use case
• ensuring the presence or absence of the Turbo-Frame header in the request based on the <a> elements targeted Turbo Frame
• appending the <form>, submitting it, listening for its next turbo:submit-end, then removing it

Now, with the addition of this change, passing the <a> element to another UJS-inspired configuration as a SubmitEvent.submitter, despite the fact that they are usually only ever <button> or <input type="submit"> elements.

I've always struggled to understand how Turbo owning the entire (probably incomplete and ever-growing) burden of responsibility outlined above is preferable over client applications using <form>+<button> elements for requests that are not GET.

What is gained by Turbo providing this support? What is lost by requiring/forcing client applications to use forms?

@excid3 I know you are not the inventor of this feature, so I want to be clear that this criticism isn't being leveled at you!

[excid3]

You know this better than me, but I can certainly see this is becoming overly complex. It would probably be good for some line to be drawn in the sand for where functionality should be expected to stop.

It seems to me like button_to is better suited to most of this functionality. Since this feature exists, adding the submitter does allow this to have some feature parity with button_to. For example, access to the a allows us to access additional attributes for the confirm modal (like description or confirmation text).

[seanpdoyle]

👍 to the change, 👎 for the feature! 🙃

[excid3]

@excid3 what about incorporating the submitted ID into the confirm message?

Oh smart!

[brunoprietog]

I also agree with this. Using links as forms that perform an action other than get should not be done for accessibility reasons, unless the link specifies the button role, which many people realistically ignore. In fact, this is mentioned in the Turbo handbook. Other needs include performing actions using buttons that are inside a form, in which case, instead of using a link, you could use a button with the formaction and formmethod attributes instead.

[pinzonjulian]

I'll chime in with a situation where I needed this a long while ago.

Problem description

I was presented with a design for a one time password validation that required a POST request to be sent from within a form:

<form method="POST" action="/sessions">
  <input type="text" name="one_time_password">
  <p>
    Didn't get the code?
    <!-- clicking this should send a request for a new one time password --> Resend the code <!-- -->
    or contact support
  </p>
  <button>Continue</button>
</form>

In this example, I needed to:

• Send a request to the server to resend the one time password to the user's phone
• Present the user with a message saying "We've resent the code to your phone number" (a flash message in the context of Rails)

A GET request doesn't quite make sense in this scenario so I wanted to use a POST request to "create a new one time password"

At that time, turbo didn't support data-turbo-method yet so my options were:

1. Change the design and pull that link out of the bounds of the form
2. Create a stimulus controller to send the request

I didn't feel a custom stimulus controller was worth it for this so I ended up changing the design so the "resend code" link/button lived outside of the <form> element.

Possible solution

I've learned since that the <button> element can target a form on a different part of the DOM so I wonder if this problem would be solved by having two, non-nested forms, and having a button targeting the appropriate form:

<form method="POST" action="/sessions">
  <input type="text" name="one_time_password">
  <p>
    Didn't get the code?
    <button form="resend-otp-form">Resend the code</button><!-- targets the form outside of this one -->
    or contact support
  </p>
  <button>Continue</button>
</form>

<form id="resend-otp-form" method="POST" action="otps"></form>

A theory on why this technique isn't used

I believe not many people know that buttons can target forms in different parts of the DOM but in the context of Rails I think the problem deepens because we're so used to using helpers for forms (form_with), links (link_to) and the button_to one that creates a form and none of these options can be used to use the technique described above.

[gucki]

@excid3 Thank you for your work on this! May I ask why this hasn't been merged yet? Is there any other workaround (except using buttons, which breaks ex. button group bootstrap styling)?