Fire `turbo:click` event when submitting `<form method="get">` #421

seanpdoyle · 2021-10-12T18:11:59Z

Prior to this change, navigating a page via an <a> element and
navigating a page via a <form method="get"> element both resulted in
turbo:visit events.

However, clicking an <a> fired a turbo:click event, whereas
submitting a <form method="get"> by clicking its <input type="submit"> or <button> element did not fire the click for the
submitter.

This commit adds support for firing turbo:click events for a
<form method="get"> element's submitter.

Prior to this change, navigating a page via an `<a>` element and navigating a page via a `<form method="get">` element both resulted in `turbo:visit` events. However, clicking an `<a>` fired a `turbo:click` event, whereas submitting a `<form method="get">` by clicking its `<input type="submit">` or `<button>` element _did not_ fire the click for the submitter. This commit adds support for firing `turbo:click` events for a `<form method="get">` element's submitter.

dhh · 2021-10-14T11:44:11Z

I wouldn't expect a form submission, even if its a GET method, to trigger turbo:click, when other form submissions do not. What gave rise to wanting this?

seanpdoyle · 2021-10-14T12:30:32Z

Form submissions with GET are a bit of an odd duck.

Clicking on an <a> fires:

turbo:click
turbo:before-visit
turbo:visit
turbo:before-cache
turbo:before-render
turbo:render
turbo:load

Submitting a <form method="post"> fires:

turbo:submit-start
turbo:before-fetch-request
turbo:before-fetch-response
turbo:submit-end
turbo:before-visit
turbo:visit
turbo:before-cache
turbo:before-render
turbo:render
turbo:load

Submitting a <form method="get"> fires:

turbo:before-visit
turbo:visit
turbo:before-cache
turbo:before-render
turbo:render
turbo:load

<a> navigations start with turbo:click, <form method="post"> submissions start with turbo:submit-start, and <form method="get"> start with turbo:before-visit.

This proposed change was motivated by a desire to have parity between clicking a <a> and submitting a <form method="get"> since they both initiate the visit->load sequence without firing form-specific events.

There are, of course, the native browser events that precede the turbo: events. <a>, <input type="submit">, and <button> events all fire click, and the <form> events all fire submit.

My worry is that the lack of consistency between navigational event sequences initiated by <a> and <form method="get"> could be surprising.

dhh · 2021-10-14T12:55:42Z

It is an odd duck, but making get-forms fire turbo:click doesn't quite make sense to me. I get that they under the good are actually more like links than forms, but that's not how a user would think of it. If anything, it should fire more of the same like post-forms. But I also think that maybe it's just fine as it is? It is a bit of a hybrid, so imo OK that it has a different event stack.

t27duck · 2021-10-14T13:23:55Z

For consistency, I would think if a form is submitting (regardless if it's a get or post) the expectation would be any of the events related to a form submitting would be fired.

So if there were to be a change, I'd vote for submitting a <form method="get">, regardless if the initial trigger was a link, to have the same events as <form method="post">

Closes hotwired#421 Closes hotwired#122 --- Fire the same sequence of events for `<form method="get">` submissions as for `<form method="post">` submissions. The [FormSubmission.prepareHeadersForRequest][] already accounts for `[method="get"]` forms by omitting the `Accept:` header with the custom `turbo-stream` MIME type. Test changes --- The `eventLogs` mechanisms we have in place declared in `src/tests/fixutres/test.js` cannot properly serialize `Element` instances, so adding `turbo:submit-start` and `turbo:submit-end` listeners to serialize events for our test suite isn't possible in the current configuration. To that end, this commit adds assertions for `<form>` submit event sequences for all events except those two. In their place, the suite adds listeners to set `[data-form-submit-start]` and `[data-form-submit-end]` to the `<html>` element when they fire. [FormSubmission.prepareHeadersForRequest]: https://github.com/hotwired/turbo/blob/58d2261274533a80a2c5efda7da211b3f20efcbb/src/core/drive/form_submission.ts#L136-L144

seanpdoyle · 2021-10-14T14:19:19Z

I've opened #424 as an alternative to this change. It changes <form method="get"> to fire the same events as other <form> submissions.

Closes hotwired#421 Closes hotwired#122 --- Fire the same sequence of events for `<form method="get">` submissions as for `<form method="post">` submissions. The [FormSubmission.prepareHeadersForRequest][] already accounts for `[method="get"]` forms by omitting the `Accept:` header with the custom `turbo-stream` MIME type. Test changes --- The `eventLogs` mechanisms we have in place declared in `src/tests/fixutres/test.js` cannot properly serialize `Element` instances, so adding `turbo:submit-start` and `turbo:submit-end` listeners to serialize events for our test suite isn't possible in the current configuration. To that end, this commit adds assertions for `<form>` submit event sequences for all events except those two. In their place, the suite adds listeners to set `[data-form-submit-start]` and `[data-form-submit-end]` to the `<html>` element when they fire. [FormSubmission.prepareHeadersForRequest]: https://github.com/hotwired/turbo/blob/58d2261274533a80a2c5efda7da211b3f20efcbb/src/core/drive/form_submission.ts#L136-L144

Closes hotwired#421 Closes hotwired#122 --- Fire the same sequence of events for `<form method="get">` submissions as for `<form method="post">` submissions. The [FormSubmission.prepareHeadersForRequest][] already accounts for `[method="get"]` forms by omitting the `Accept:` header with the custom `turbo-stream` MIME type. Visit actions --- Prior to this change, a `<form method="get">` would _always_ result in two Visits: the first with `{ action: "advance" }`, then a second with `{ action: "replace" }`. Since the response to a `<form method="get">` has the potential to be a [200 OK][] or a [201 Created][], this commit also modifies the `Visit` class to skip the `followRedirect()` call when the request is idempotent and the response is not a redirect. Test changes --- The `eventLogs` mechanisms we have in place declared in `src/tests/fixutres/test.js` cannot properly serialize `Element` instances, so adding `turbo:submit-start` and `turbo:submit-end` listeners to serialize events for our test suite isn't possible in the current configuration. To that end, this commit adds assertions for `<form>` submit event sequences for all events except those two. In their place, the suite adds listeners to set `[data-form-submit-start]` and `[data-form-submit-end]` to the `<html>` element when they fire. [FormSubmission.prepareHeadersForRequest]: https://github.com/hotwired/turbo/blob/58d2261274533a80a2c5efda7da211b3f20efcbb/src/core/drive/form_submission.ts#L136-L144 [200 OK]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200 [200 Created]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/201

seanpdoyle · 2021-10-14T16:21:52Z

The more I think about it, the more I think what's proposed in #424 is more viable.

I'm closing this in favor of #424.

Closes #421 Closes #122 --- Fire the same sequence of events for `<form method="get">` submissions as for `<form method="post">` submissions. The [FormSubmission.prepareHeadersForRequest][] already accounts for `[method="get"]` forms by omitting the `Accept:` header with the custom `turbo-stream` MIME type. Visit actions --- Prior to this change, a `<form method="get">` would _always_ result in two Visits: the first with `{ action: "advance" }`, then a second with `{ action: "replace" }`. Since the response to a `<form method="get">` has the potential to be a [200 OK][] or a [201 Created][], this commit also modifies the `Visit` class to skip the `followRedirect()` call when the request is idempotent and the response is not a redirect. Test changes --- The `eventLogs` mechanisms we have in place declared in `src/tests/fixutres/test.js` cannot properly serialize `Element` instances, so adding `turbo:submit-start` and `turbo:submit-end` listeners to serialize events for our test suite isn't possible in the current configuration. To that end, this commit adds assertions for `<form>` submit event sequences for all events except those two. In their place, the suite adds listeners to set `[data-form-submit-start]` and `[data-form-submit-end]` to the `<html>` element when they fire. [FormSubmission.prepareHeadersForRequest]: https://github.com/hotwired/turbo/blob/58d2261274533a80a2c5efda7da211b3f20efcbb/src/core/drive/form_submission.ts#L136-L144 [200 OK]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200 [200 Created]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/201

seanpdoyle mentioned this pull request Oct 12, 2021

Build-in integration with rails/ujs hotwired/turbo-rails#257

Closed

seanpdoyle mentioned this pull request Oct 14, 2021

GET Forms: fire submit-start and submit-end #424

Merged

seanpdoyle closed this Oct 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fire `turbo:click` event when submitting `<form method="get">` #421

Fire `turbo:click` event when submitting `<form method="get">` #421

seanpdoyle commented Oct 12, 2021

dhh commented Oct 14, 2021

seanpdoyle commented Oct 14, 2021

dhh commented Oct 14, 2021

t27duck commented Oct 14, 2021

seanpdoyle commented Oct 14, 2021

seanpdoyle commented Oct 14, 2021

Fire turbo:click event when submitting <form method="get"> #421

Fire turbo:click event when submitting <form method="get"> #421

Conversation

seanpdoyle commented Oct 12, 2021

dhh commented Oct 14, 2021

seanpdoyle commented Oct 14, 2021

dhh commented Oct 14, 2021

t27duck commented Oct 14, 2021

seanpdoyle commented Oct 14, 2021

seanpdoyle commented Oct 14, 2021

Fire `turbo:click` event when submitting `<form method="get">` #421

Fire `turbo:click` event when submitting `<form method="get">` #421