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
Declare ActionView::Helpers::FormBuilder#id and #field_id #40127
Conversation
bdda1a7
to
c95e8ee
Compare
# <tt>aria-describedby</tt> attribute referencing the <tt><span></tt> | ||
# element, sharing a common <tt>id</tt> root (<tt>post_title</tt>, in this | ||
# case). | ||
def field_id(object_name, name, *suffixes, index: nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying #40127 (comment) here, since the line in question has changed.
This set of changes was lifted directly from ActionView::Helpers::Tags::Base
.
rails/actionview/lib/action_view/helpers/tags/base.rb
Lines 119 to 137 in 7101489
def tag_id(index = nil) | |
# a little duplication to construct fewer strings | |
case | |
when @object_name.empty? | |
sanitized_method_name.dup | |
when index | |
"#{sanitized_object_name}_#{index}_#{sanitized_method_name}" | |
else | |
"#{sanitized_object_name}_#{sanitized_method_name}" | |
end | |
end | |
def sanitized_object_name | |
@sanitized_object_name ||= @object_name.gsub(/\]\[|[^-a-zA-Z0-9:.]/, "_").delete_suffix("_") | |
end | |
def sanitized_method_name | |
@sanitized_method_name ||= @method_name.delete_suffix("?") | |
end |
Given the "a little duplication to construct fewer strings", it seemed like that code was finely tuned for performance, which made me hesitant to abstract out a shared and re-usable chunk of code.
My gut tells me that FormBuilder#field_id
will be invoked less frequently than the Tags::Base
code. That being said, if there is an abstraction that feels right, I'm all for extracting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first attempt is to extract out a shared constant (as introduced in 419e179).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK ActionView::Helpers::Tags::Base
has acces to the template so we should probably just move tag_id
implementation to field_id
and call field_id
from tag_id
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafaelfranca I'll try that out. The # a little duplication to construct fewer strings
comments make me a little nervous about moving that code around.
To move that definition elsewhere, we'd likely lose some of the memoization that's going on. Is there a way to gauge the impact of these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafaelfranca I've pushed up c5869ce to try that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that is why I suggested moving the entire code, so we keep the # a little duplication to construct fewer strings
comment and the duplication to construct fewer strings.
0bbbb0e
to
a90f0af
Compare
This could come in handy 😄 |
The However, there are some differences that could be confusing and surprising if they shared the same name. In our examples, the |
While the lengths of the new call sites are actually longer than the original identifier declarations, there is less conceptual and incidental duplication. The `ValidationMessageFormBuilder#field_id` method abstracts this generation: Generate an HTML `id` attribute value for the given field Return the value generated by the `FormBuilder` for the given attribute name. ```html+erb <%= form_for @post do |f| %> <%= f.label :title %> <%= f.text_field :title, aria: { describedby: f.field_id(:title, :error) } %> <%= tag.span("is blank", id: f.field_id(:title, :error) %> <% end %> ``` In the example above, the `<input type="text">` element built by the call to `FormBuilder#text_field` declares an `aria-describedby` attribute referencing the `<span>` element, sharing a common `id` root (`post_title`, in this case). This contains a partial re-production of [rails/rails#40127][]. [rails/rails#40127]: rails/rails#40127
While the lengths of the new call sites are actually longer than the original identifier declarations, there is less conceptual and incidental duplication. The `ValidationMessageFormBuilder#field_id` method abstracts this generation: Generate an HTML `id` attribute value for the given field Return the value generated by the `FormBuilder` for the given attribute name. ```html+erb <%= form_for @post do |f| %> <%= f.label :title %> <%= f.text_field :title, aria: { describedby: f.field_id(:title, :error) } %> <%= tag.span("is blank", id: f.field_id(:title, :error) %> <% end %> ``` In the example above, the `<input type="text">` element built by the call to `FormBuilder#text_field` declares an `aria-describedby` attribute referencing the `<span>` element, sharing a common `id` root (`post_title`, in this case). This contains a partial re-production of [rails/rails#40127][]. [rails/rails#40127]: rails/rails#40127
While the lengths of the new call sites are actually longer than the original identifier declarations, there is less conceptual and incidental duplication. The `ValidationMessageFormBuilder#field_id` method abstracts this generation: Generate an HTML `id` attribute value for the given field Return the value generated by the `FormBuilder` for the given attribute name. ```html+erb <%= form_for @post do |f| %> <%= f.label :title %> <%= f.text_field :title, aria: { describedby: f.field_id(:title, :error) } %> <%= tag.span("is blank", id: f.field_id(:title, :error) %> <% end %> ``` In the example above, the `<input type="text">` element built by the call to `FormBuilder#text_field` declares an `aria-describedby` attribute referencing the `<span>` element, sharing a common `id` root (`post_title`, in this case). This contains a partial re-production of [rails/rails#40127][]. [rails/rails#40127]: rails/rails#40127
While the lengths of the new call sites are actually longer than the original identifier declarations, there is less conceptual and incidental duplication. The `ValidationMessageFormBuilder#field_id` method abstracts this generation: Generate an HTML `id` attribute value for the given field Return the value generated by the `FormBuilder` for the given attribute name. ```html+erb <%= form_for @post do |f| %> <%= f.label :title %> <%= f.text_field :title, aria: { describedby: f.field_id(:title, :error) } %> <%= tag.span("is blank", id: f.field_id(:title, :error) %> <% end %> ``` In the example above, the `<input type="text">` element built by the call to `FormBuilder#text_field` declares an `aria-describedby` attribute referencing the `<span>` element, sharing a common `id` root (`post_title`, in this case). This contains a partial re-production of [rails/rails#40127][]. [rails/rails#40127]: rails/rails#40127
While the lengths of the new call sites are actually longer than the original identifier declarations, there is less conceptual and incidental duplication. The `ValidationMessageFormBuilder#field_id` method abstracts this generation: Generate an HTML `id` attribute value for the given field Return the value generated by the `FormBuilder` for the given attribute name. ```html+erb <%= form_for @post do |f| %> <%= f.label :title %> <%= f.text_field :title, aria: { describedby: f.field_id(:title, :error) } %> <%= tag.span("is blank", id: f.field_id(:title, :error) %> <% end %> ``` In the example above, the `<input type="text">` element built by the call to `FormBuilder#text_field` declares an `aria-describedby` attribute referencing the `<span>` element, sharing a common `id` root (`post_title`, in this case). This contains a partial re-production of [rails/rails#40127][]. [rails/rails#40127]: rails/rails#40127
While the lengths of the new call sites are actually longer than the original identifier declarations, there is less conceptual and incidental duplication. The `ValidationMessageFormBuilder#field_id` method abstracts this generation: Generate an HTML `id` attribute value for the given field Return the value generated by the `FormBuilder` for the given attribute name. ```html+erb <%= form_for @post do |f| %> <%= f.label :title %> <%= f.text_field :title, aria: { describedby: f.field_id(:title, :error) } %> <%= tag.span("is blank", id: f.field_id(:title, :error) %> <% end %> ``` In the example above, the `<input type="text">` element built by the call to `FormBuilder#text_field` declares an `aria-describedby` attribute referencing the `<span>` element, sharing a common `id` root (`post_title`, in this case). This contains a partial re-production of [rails/rails#40127][]. [rails/rails#40127]: rails/rails#40127
While the lengths of the new call sites are actually longer than the original identifier declarations, there is less conceptual and incidental duplication. The `ValidationMessageFormBuilder#field_id` method abstracts this generation: Generate an HTML `id` attribute value for the given field Return the value generated by the `FormBuilder` for the given attribute name. ```html+erb <%= form_for @post do |f| %> <%= f.label :title %> <%= f.text_field :title, aria: { describedby: f.field_id(:title, :error) } %> <%= tag.span("is blank", id: f.field_id(:title, :error) %> <% end %> ``` In the example above, the `<input type="text">` element built by the call to `FormBuilder#text_field` declares an `aria-describedby` attribute referencing the `<span>` element, sharing a common `id` root (`post_title`, in this case). This contains a partial re-production of [rails/rails#40127][]. [rails/rails#40127]: rails/rails#40127
While the lengths of the new call sites are actually longer than the original identifier declarations, there is less conceptual and incidental duplication. The `ValidationMessageFormBuilder#field_id` method abstracts this generation: Generate an HTML `id` attribute value for the given field Return the value generated by the `FormBuilder` for the given attribute name. ```html+erb <%= form_for @post do |f| %> <%= f.label :title %> <%= f.text_field :title, aria: { describedby: f.field_id(:title, :error) } %> <%= tag.span("is blank", id: f.field_id(:title, :error) %> <% end %> ``` In the example above, the `<input type="text">` element built by the call to `FormBuilder#text_field` declares an `aria-describedby` attribute referencing the `<span>` element, sharing a common `id` root (`post_title`, in this case). This contains a partial re-production of [rails/rails#40127][]. [rails/rails#40127]: rails/rails#40127
While the lengths of the new call sites are actually longer than the original identifier declarations, there is less conceptual and incidental duplication. The `ValidationMessageFormBuilder#field_id` method abstracts this generation: Generate an HTML `id` attribute value for the given field Return the value generated by the `FormBuilder` for the given attribute name. ```html+erb <%= form_for @post do |f| %> <%= f.label :title %> <%= f.text_field :title, aria: { describedby: f.field_id(:title, :error) } %> <%= tag.span("is blank", id: f.field_id(:title, :error) %> <% end %> ``` In the example above, the `<input type="text">` element built by the call to `FormBuilder#text_field` declares an `aria-describedby` attribute referencing the `<span>` element, sharing a common `id` root (`post_title`, in this case). This contains a partial re-production of [rails/rails#40127][]. [rails/rails#40127]: rails/rails#40127
While the lengths of the new call sites are actually longer than the original identifier declarations, there is less conceptual and incidental duplication. The `ValidationMessageFormBuilder#field_id` method abstracts this generation: Generate an HTML `id` attribute value for the given field Return the value generated by the `FormBuilder` for the given attribute name. ```html+erb <%= form_for @post do |f| %> <%= f.label :title %> <%= f.text_field :title, aria: { describedby: f.field_id(:title, :error) } %> <%= tag.span("is blank", id: f.field_id(:title, :error) %> <% end %> ``` In the example above, the `<input type="text">` element built by the call to `FormBuilder#text_field` declares an `aria-describedby` attribute referencing the `<span>` element, sharing a common `id` root (`post_title`, in this case). This contains a partial re-production of [rails/rails#40127][]. [rails/rails#40127]: rails/rails#40127
While the lengths of the new call sites are actually longer than the original identifier declarations, there is less conceptual and incidental duplication. The `ValidationMessageFormBuilder#field_id` method abstracts this generation: Generate an HTML `id` attribute value for the given field Return the value generated by the `FormBuilder` for the given attribute name. ```html+erb <%= form_for @post do |f| %> <%= f.label :title %> <%= f.text_field :title, aria: { describedby: f.field_id(:title, :error) } %> <%= tag.span("is blank", id: f.field_id(:title, :error) %> <% end %> ``` In the example above, the `<input type="text">` element built by the call to `FormBuilder#text_field` declares an `aria-describedby` attribute referencing the `<span>` element, sharing a common `id` root (`post_title`, in this case). This contains a partial re-production of [rails/rails#40127][]. [rails/rails#40127]: rails/rails#40127
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. |
cb1505f
to
c5869ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small comment, otherwise looks good to me. Can you squash all commits when you are done with the last fix?
`ActionView::Helpers::FormBuilder#id` --- Generate an HTML `id` attribute value. Return the [`<form>` element's][mdn-form] `id` attribute. ```html+erb <%= form_for @post do |f| %> <%# ... %> <% content_for :sticky_footer do %> <%= form.button(form: f.id) %> <% end %> <% end %> ``` In the example above, the `:sticky_footer` content area will exist outside of the `<form>` element. [By declaring the `form` HTML attribute][mdn-button-attr-form], we hint to the browser that the generated `<button>` element should be treated as the `<form>` element's submit button, regardless of where it exists in the DOM. [A similar pattern could be used for `<input>` elements][mdn-input-attr-form] (or other form controls) that do not descend from the `<form>` element. `ActionView::Helpers::FormBuilder#field_id` --- Generate an HTML <tt>id</tt> attribute value for the given field Return the value generated by the <tt>FormBuilder</tt> for the given attribute name. ```html+erb <%= form_for @post do |f| %> <%= f.label :title %> <%= f.text_field :title, aria: { describedby: form.field_id(:title, :error) } %> <span id="<%= f.field_id(:title, :error) %>">is blank</span> <% end %> ``` In the example above, the <tt><input type="text"></tt> element built by the call to <tt>FormBuilder#text_field</tt> declares an <tt>aria-describedby</tt> attribute referencing the <tt><span></tt> element, sharing a common <tt>id</tt> root (<tt>post_title</tt>, in this case). This method is powered by the `field_id` helper declared in `action_view/helpers/form_tag_helper`, which is made available for general template calls, separate from a `FormBuilder` instance. [mdn-form]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/form [mdn-button-attr-form]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#attr-form [mdn-input-attr-form]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#attr-form [mdn-aria-describedby]: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-describedby_attribute [w3c-wai]: https://www.w3.org/WAI/tutorials/forms/notifications/#listing-errors
769caae
to
59ca21c
Compare
Squashed and pushed. Thank you @rafaelfranca. |
While the lengths of the new call sites are actually longer than the original identifier declarations, there is less conceptual and incidental duplication. The `ValidationMessageFormBuilder#field_id` method abstracts this generation: Generate an HTML `id` attribute value for the given field Return the value generated by the `FormBuilder` for the given attribute name. ```html+erb <%= form_for @post do |f| %> <%= f.label :title %> <%= f.text_field :title, aria: { describedby: f.field_id(:title, :error) } %> <%= tag.span("is blank", id: f.field_id(:title, :error) %> <% end %> ``` In the example above, the `<input type="text">` element built by the call to `FormBuilder#text_field` declares an `aria-describedby` attribute referencing the `<span>` element, sharing a common `id` root (`post_title`, in this case). This contains a partial re-production of [rails/rails#40127][]. [rails/rails#40127]: rails/rails#40127
As a follow-up to [rails#40127][], this commit adds a bug fix for nested form builders (through either `fields_for` or `fields`) incorrectly constructing a field's `[id]` attribute. To do so, treat the `@object_name` with higher precedence than the `@object`, since that will be provided as instance state during construction. [rails#40127]: rails#40127
The `field_name` helper and corresponding `FormBuilder#field_name` method provide an Action View-compliant way of overriding a form field element's `[name]` attribute (similar to `field_id` and `FormBuilder#field_id` introduced in rails#40127[][]). ```ruby text_field_tag :post, :title, name: field_name(:post, :title, :subtitle) # => <input type="text" name="post[title][subtitle]"> text_field_tag :post, :tag, name: field_name(:post, :tag, multiple: true) # => <input type="text" name="post[tag][]"> form_for @post do |f| f.field_tag :tag, name: f.field_name(:tag, multiple: true) # => <input type="text" name="post[tag][]"> end ``` [rails#40127]: rails#40127
The `field_name` helper and corresponding `FormBuilder#field_name` method provide an Action View-compliant way of overriding a form field element's `[name]` attribute (similar to `field_id` and `FormBuilder#field_id` introduced in rails#40127[][]). ```ruby text_field_tag :post, :title, name: field_name(:post, :title, :subtitle) # => <input type="text" name="post[title][subtitle]"> text_field_tag :post, :tag, name: field_name(:post, :tag, multiple: true) # => <input type="text" name="post[tag][]"> form_for @post do |f| f.field_tag :tag, name: f.field_name(:tag, multiple: true) # => <input type="text" name="post[tag][]"> end ``` [rails#40127]: rails#40127
Summary
ActionView::Helpers::FormBuilder#id
Generate an HTML
id
attribute value.Return the
<form>
element'sid
attribute.In the example above, the
:sticky_footer
content area will existoutside of the
<form>
element. By declaring theform
HTMLattribute, we hint to the browser that the
generated
<button>
element should be treated as the<form>
element'ssubmit button, regardless of where it exists in the DOM.
A similar pattern could be used for
<input>
elements (or other form controls) that do not
descend from the
<form>
element.ActionView::Helpers::FormBuilder#field_id
Generate an HTML id attribute value for the given field
Return the value generated by the FormBuilder for the given
attribute name.
In the example above, the element built by
the call to FormBuilder#text_field declares an
aria-describedby attribute referencing the
element, sharing a common id root (post_title, in this
case).
This method is powered by the
field_id
helper declared inaction_view/helpers/form_tag_helper
, which is made available forgeneral template calls, separate from a
FormBuilder
instance.