Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

form_tag and form_with: close tag when block omitted #44275

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Jan 27, 2022

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:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • 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]
  • Tests are added or updated if you fix a bug or add a feature.
  • 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.

@rails-bot rails-bot bot added the actionview label Jan 27, 2022
@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch 4 times, most recently from 6652fea to 6b60718 Compare January 29, 2022 16:39
@nvasilevski
Copy link
Contributor

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

@matthewd
Copy link
Member

matthewd commented Feb 7, 2022

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?

@seanpdoyle
Copy link
Contributor Author

seanpdoyle commented Feb 7, 2022

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?

@rails-bot
Copy link

rails-bot bot commented May 8, 2022

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 rails-bot bot added the stale label May 8, 2022
@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch from 6b60718 to 074a5bf Compare September 6, 2022 12:28
@rails-bot rails-bot bot removed the stale label Sep 6, 2022
@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch from 074a5bf to cf62569 Compare December 23, 2022 01:15
@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch 2 times, most recently from 72f8221 to ca7ff94 Compare September 25, 2023 11:41
@seanpdoyle
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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 seanpdoyle changed the title form_with: ensure closing tag when omitting block form_tag and form_with: close tag when block omitted Oct 5, 2023
@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch from f48c19b to becbe8a Compare October 5, 2023 15:57
@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch from becbe8a to b3537c9 Compare October 5, 2023 16:05
@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch 2 times, most recently from 87360e5 to 86e6247 Compare October 16, 2023 22:15
@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch 2 times, most recently from c7f8de5 to 7ac65d2 Compare November 2, 2023 20:21
@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch from 7ac65d2 to 49fae33 Compare December 2, 2023 13:40
@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch 2 times, most recently from 4bd9be2 to ebd793c Compare December 13, 2023 17:09
@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch from ebd793c to 732a606 Compare January 2, 2024 14:37
@seanpdoyle
Copy link
Contributor Author

@rafaelfranca could you review these changes?

@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch 2 times, most recently from e4b8c7d to a22496e Compare January 10, 2024 12:30
@p8
Copy link
Member

p8 commented Jan 18, 2024

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>

@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch from a22496e to dbc32eb Compare January 30, 2024 03:12
@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch from dbc32eb to 2cb3b07 Compare March 2, 2024 15:34
@seanpdoyle
Copy link
Contributor Author

@skipkayhil are you able to review this proposal?

@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch from 2cb3b07 to 99bc8e8 Compare March 2, 2024 15:36
@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch from 99bc8e8 to b5d9b50 Compare March 24, 2024 23:48
@seanpdoyle seanpdoyle force-pushed the action-view-form-with-empty-close-tag branch 2 times, most recently from 5e28e7b to 12dfc38 Compare April 24, 2024 13:22
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:

```erb
<%= 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.

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.

[form_for]: https://github.com/rails/rails/blob/efae65d005ee298207d720fb4f61c28a38973e8e/actionview/test/template/form_helper_test.rb#L1555-L1560
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants