rails icon indicating copy to clipboard operation
rails copied to clipboard

`form_tag` and `form_with`: close tag when block omitted

Open seanpdoyle opened this issue 2 years ago • 9 comments

Motivation / Background

Prior to this change, calling a form_tag or form_with without passing a block generates a <form> element without a closing </form> tag.

The form_for version of building a <form> element requires a block to execute, so omitting the closing </form> element isn't a possibility.

A current work-around to achieve the desired behavior is to declare the form_tag or form_with call with an empty block:

<%= form_tag "https://example.com" do %>
<% end %>

<%= form_with url: "https://example.com" do %>
<% end %>

Unfortunately, being unaware of this issue will likely yield HTML that is silently invalid, which can cause surprising nesting and behavior, such as elements declared after the <form> becoming descendants instead of siblings.

Detail

Introduce the config.action_view.closes_form_tag_without_block configuration value.

By default (in versions prior to 7.2), that value is false and preserves backwards compatibility. When set to true, calls to form_with and form_tag without a block will result in elements that close themselves.

Checklist

Before submitting the PR make sure the following are checked:

  • [x] This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • [x] Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • [x] Tests are added or updated if you fix a bug or add a feature.
  • [x] CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

seanpdoyle avatar Jan 27 '22 18:01 seanpdoyle

Sounds reasonable, and the change looks good. Just wanted to bump this PR 🙃

nvasilevski avatar Feb 06 '22 15:02 nvasilevski

This is the described intent of the method, per #26976 -- it has a non-block form so that it can be support the form_tag use case as well as the form_for one.

I'm doubtful that entirely-empty-form-tag is a common enough use case that people are likely to call this blocklessly and expect/want that result... is that a frequent pattern in a context I'm unfamiliar with?

matthewd avatar Feb 07 '22 01:02 matthewd

is that a frequent pattern in a context I'm unfamiliar with?

The use case that got me digging was inline editing:

<%# app/views/articles/_inline_edit.html.erb %>

<% frame_id = dom_id(model, "#{method}_turbo_frame") %>

<%= form_with model: model, data: { turbo_frame: frame_id } do %>
  <turbo-frame id="<%= frame_id %>" class="contents group inline-edit">
    <%= yield %>

    <%= link_to edit_article_path(model) do %>
      Edit <%= model.class.human_attribute_name(method) %>
    <% end %>
  </turbo-frame>
<% end %>

Rendering the partial with a block would wrap the contents (through the <%= yield %> line) within a <form> generated by form_with. It's invalid HTML to nest a <form> within another <form>, so to prevent accidents, the original version of the partial rendered the <turbo-frame> and <form> as siblings, and rendered an empty <form> with an [id] attribute:

<% frame_id = dom_id(model, "#{method}_turbo_frame") %>

<%= form_with model: model, id: "#{method}_form", data: { turbo_frame: frame_id } %>

<turbo-frame id="<%= frame_id %>" class="contents group inline-edit">
  <%= yield %>

  <%= link_to edit_article_path(model) do %>
    Edit <%= model.class.human_attribute_name(method) %>
  <% end %>
</turbo-frame>

This version would require callers to render fields with a [form] attribute that referred to the <form> element by [id] (i.e. "#{method}_form". This way, there's no risk of doubly-nested <form> elements. Once I discovered that I needed to call the form_with with an empty block or with a hard-coded </form> tag as a sibling, it worked as intended.

I'd never deployed the pattern before. It's certainly an edge case that's extremely uncommon. I think my surprise stems from the overlap between empty <form> elements being so uncommon and the form_with use case that depends on the omission of the trailing </form> tag being so uncommon. The form_with documentation doesn't mention this behavior, and there aren't any tests that assert the omission.

If ensuring the </form> tag is a step in the wrong direction, should there be an alternative PR that documents the behavior and adds tests to guard against introducing a regression in the future?

seanpdoyle avatar Feb 07 '22 16:02 seanpdoyle

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

rails-bot[bot] avatar May 08 '22 17:05 rails-bot[bot]

@matthewd I'm sorry to have let this PR drift so far behind main. I've rebased it.

To re-iterate my main motivation for this change:

I think my surprise stems from the overlap between empty <form> elements being so uncommon and the behavior of a block-less form_with being somewhat difficult to predict.

Even if it isn't intended as an end-result, a call to <%= form_with ... %> without a block includes all subsequent elements as its children, since it isn't self-closing. Accidents that introduce an open <form> element early in the page are what I'm most afraid of.

seanpdoyle avatar Sep 25 '23 17:09 seanpdoyle

@matthewd I've expanded this work to include a new config.action_view.renders_void_form_elements configuration value and 7.2 default.

Does that level on granular control improve this change's backwards compatibility? I'm hopeful that it could also provide an avenue toward deprecation, then maybe global support in the next major release.

seanpdoyle avatar Oct 05 '23 15:10 seanpdoyle

@rafaelfranca could you review these changes?

seanpdoyle avatar Jan 03 '24 20:01 seanpdoyle

Another use case for an empty form, is a form with the submit button outside the form element:

<form id="create_like" action="likes" method="POST" >
    <input type="hidden" name="authenticity_token" value="zZEbm" />
</form>
<button type="submit" form="create_like">:+1</button>

This would be useful when the button is already in a form:

<form id="create_like" action="likes" method="POST" >
   ...
</form>

<form action="comment" method="POST">
   ....
   <button type="submit" form="create_like">:+1</button>
</form>

p8 avatar Jan 18 '24 20:01 p8

@skipkayhil are you able to review this proposal?

seanpdoyle avatar Mar 02 '24 15:03 seanpdoyle