From a6d065e39f9285119d75a05ee46c659f8b3c0db8 Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Mon, 21 Nov 2016 21:48:38 +0100 Subject: form_with/fields: Don't output ids by default Continuing 67f81cc where we decided not to output ids by default in the new form helpers. Went with @dhh's suggestion of just requiring ids on fields being labelled: https://github.com/rails/rails/issues/25197#issuecomment-231797117 Seems okay enough. --- actionview/lib/action_view/helpers/form_helper.rb | 31 +- actionview/lib/action_view/helpers/tags/base.rb | 14 +- actionview/lib/action_view/helpers/tags/label.rb | 4 + .../test/template/form_helper/form_with_test.rb | 400 ++++++++++----------- 4 files changed, 243 insertions(+), 206 deletions(-) (limited to 'actionview') diff --git a/actionview/lib/action_view/helpers/form_helper.rb b/actionview/lib/action_view/helpers/form_helper.rb index e7ea267211..55389950ed 100644 --- a/actionview/lib/action_view/helpers/form_helper.rb +++ b/actionview/lib/action_view/helpers/form_helper.rb @@ -595,6 +595,16 @@ module ActionView # # Where @document = Document.find(params[:id]). # + # When using labels +form_with+ requires setting the id on the field being + # labelled: + # + # <%= form_with(model: @post) do |form| %> + # <%= form.label :title %> + # <%= form.text_field :title, id: :post_title %> + # <% end %> + # + # See +label+ for more on how the +for+ attribute is derived. + # # === Mixing with other form helpers # # While +form_with+ uses a FormBuilder object it's possible to mix and @@ -690,6 +700,8 @@ module ActionView # form_with(**options.merge(builder: LabellingFormBuilder), &block) # end def form_with(model: nil, scope: nil, url: nil, format: nil, **options) + options[:skip_default_ids] = true + if model url ||= polymorphic_path(model, format: format) @@ -986,6 +998,16 @@ module ActionView # or model is yielded, so any generated field names are prefixed with # either the passed scope or the scope inferred from the :model. # + # When using labels +fields+ requires setting the id on the field being + # labelled: + # + # <%= fields :comment do |fields| %> + # <%= fields.label :body %> + # <%= fields.text_field :body, id: :comment_body %> + # <% end %> + # + # See +label+ for more on how the +for+ attribute is derived. + # # === Mixing with other form helpers # # While +form_with+ uses a FormBuilder object it's possible to mix and @@ -1003,7 +1025,8 @@ module ActionView # to work with an object as a base, like # FormOptionHelper#collection_select and DateHelper#datetime_select. def fields(scope = nil, model: nil, **options, &block) - # TODO: Remove when ids and classes are no longer output by default. + options[:skip_default_ids] = true + if model scope ||= model_name_from_record_or_class(model).param_key end @@ -1469,7 +1492,7 @@ module ActionView private def html_options_for_form_with(url_for_options = nil, model = nil, html: {}, local: false, skip_enforcing_utf8: false, **options) - html_options = options.except(:index, :include_id, :builder).merge(html) + html_options = options.except(:index, :include_id, :skip_default_ids, :builder).merge(html) html_options[:method] ||= :patch if model.respond_to?(:persisted?) && model.persisted? html_options[:enforce_utf8] = !skip_enforcing_utf8 @@ -1603,7 +1626,7 @@ module ActionView def initialize(object_name, object, template, options) @nested_child_index = {} @object_name, @object, @template, @options = object_name, object, template, options - @default_options = @options ? @options.slice(:index, :namespace) : {} + @default_options = @options ? @options.slice(:index, :namespace, :skip_default_ids) : {} convert_to_legacy_options(@options) @@ -1910,6 +1933,8 @@ module ActionView # See the docs for the ActionView::FormHelper.fields helper method. def fields(scope = nil, model: nil, **options, &block) + options[:skip_default_ids] = true + convert_to_legacy_options(options) fields_for(scope || model, model, **options, &block) diff --git a/actionview/lib/action_view/helpers/tags/base.rb b/actionview/lib/action_view/helpers/tags/base.rb index cf8a6d6028..b8c446cbed 100644 --- a/actionview/lib/action_view/helpers/tags/base.rb +++ b/actionview/lib/action_view/helpers/tags/base.rb @@ -13,6 +13,7 @@ module ActionView @object_name.sub!(/\[\]$/, "") || @object_name.sub!(/\[\]\]$/, "]") @object = retrieve_object(options.delete(:object)) + @skip_default_ids = options.delete(:skip_default_ids) @options = options @auto_index = Regexp.last_match ? retrieve_autoindex(Regexp.last_match.pre_match) : nil end @@ -81,9 +82,12 @@ module ActionView def add_default_name_and_id(options) index = name_and_id_index(options) options["name"] = options.fetch("name") { tag_name(options["multiple"], index) } - options["id"] = options.fetch("id") { tag_id(index) } - if namespace = options.delete("namespace") - options["id"] = options["id"] ? "#{namespace}_#{options['id']}" : namespace + + unless skip_default_ids? + options["id"] = options.fetch("id") { tag_id(index) } + if namespace = options.delete("namespace") + options["id"] = options["id"] ? "#{namespace}_#{options['id']}" : namespace + end end end @@ -154,6 +158,10 @@ module ActionView def name_and_id_index(options) options.key?("index") ? options.delete("index") || "" : @auto_index end + + def skip_default_ids? + @skip_default_ids + end end end end diff --git a/actionview/lib/action_view/helpers/tags/label.rb b/actionview/lib/action_view/helpers/tags/label.rb index b31d5fda66..cab15ae201 100644 --- a/actionview/lib/action_view/helpers/tags/label.rb +++ b/actionview/lib/action_view/helpers/tags/label.rb @@ -73,6 +73,10 @@ module ActionView def render_component(builder) builder.translation end + + def skip_default_ids? + false # The id is used as the `for` attribute. + end end end end diff --git a/actionview/test/template/form_helper/form_with_test.rb b/actionview/test/template/form_helper/form_with_test.rb index c80a2f61b9..4efc93ca08 100644 --- a/actionview/test/template/form_helper/form_with_test.rb +++ b/actionview/test/template/form_helper/form_with_test.rb @@ -301,10 +301,10 @@ class FormWithActsLikeFormForTest < FormWithTest expected = whole_form("/posts/123", "create-post", method: "patch") do "" + - "" + - "" + + "" + + "" + "" + - "" + + "" + "" + "" + "" @@ -322,9 +322,9 @@ class FormWithActsLikeFormForTest < FormWithTest expected = whole_form("/posts") do "" + - "" + + "" + "" + - "" + + "" + "" end @@ -345,10 +345,10 @@ class FormWithActsLikeFormForTest < FormWithTest expected = whole_form("/posts") do "" + "" + "" end @@ -371,12 +371,12 @@ class FormWithActsLikeFormForTest < FormWithTest expected = whole_form("/posts") do "" + "" + "" + - "" + "" end assert_dom_equal expected, output_buffer @@ -392,9 +392,9 @@ class FormWithActsLikeFormForTest < FormWithTest expected = whole_form("/posts") do "" + - "" + + "" + "" + - "" + + "" + "" end @@ -411,11 +411,11 @@ class FormWithActsLikeFormForTest < FormWithTest expected = whole_form("/posts") do "" + - "" + + "" + "" + - "" + + "" + "" + - "" + + "" + "" end @@ -436,13 +436,13 @@ class FormWithActsLikeFormForTest < FormWithTest expected = whole_form("/posts") do "" + "" + "" + "" end @@ -466,15 +466,15 @@ class FormWithActsLikeFormForTest < FormWithTest expected = whole_form("/posts") do "" + "" + "" + "" + - "" + "" end assert_dom_equal expected, output_buffer @@ -491,7 +491,7 @@ class FormWithActsLikeFormForTest < FormWithTest expected = whole_form("/posts") do "" + - "" + + "" + "" end @@ -506,7 +506,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", "create-post", method: "patch", multipart: true) do - "" + "" end assert_dom_equal expected, output_buffer @@ -522,7 +522,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch", multipart: true) do - "" + "" end assert_dom_equal expected, output_buffer @@ -561,7 +561,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/44", method: "patch") do - "" + + "" + "" end @@ -579,10 +579,10 @@ class FormWithActsLikeFormForTest < FormWithTest expected = whole_form("/posts/123", "create-post", method: "patch") do "" + - "" + - "" + + "" + + "" + "" + - "" + + "" + "" end @@ -609,10 +609,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/", "create-post", method: "delete") do - "" + - "" + + "" + + "" + "" + - "" + "" end assert_dom_equal expected, output_buffer @@ -626,10 +626,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/", "create-post", method: "delete") do - "" + - "" + + "" + + "" + "" + - "" + "" end assert_dom_equal expected, output_buffer @@ -643,7 +643,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/search", "search-post", method: "get") do - "" + "" end assert_dom_equal expected, output_buffer @@ -657,10 +657,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/", "create-post", method: "patch") do - "" + - "" + + "" + + "" + "" + - "" + "" end assert_dom_equal expected, output_buffer @@ -672,7 +672,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/", skip_enforcing_utf8: true) do - "" + "" end assert_dom_equal expected, output_buffer @@ -684,7 +684,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/", skip_enforcing_utf8: false) do - "" + "" end assert_dom_equal expected, output_buffer @@ -698,10 +698,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/", "create-post") do - "" + - "" + + "" + + "" + "" + - "" + "" end assert_dom_equal expected, output_buffer @@ -717,10 +717,10 @@ class FormWithActsLikeFormForTest < FormWithTest expected = whole_form("/posts/123", method: "patch") do "" + - "" + - "" + + "" + + "" + "" + - "" + "" end assert_dom_equal expected, output_buffer @@ -734,10 +734,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - "" + - "" + + "" + + "" + "" + - "" + "" end assert_dom_equal expected, output_buffer @@ -752,7 +752,7 @@ class FormWithActsLikeFormForTest < FormWithTest expected = whole_form("/posts/123", method: "patch") do "
" + - "
" + + "
" + "" end @@ -770,7 +770,7 @@ class FormWithActsLikeFormForTest < FormWithTest expected = whole_form("/posts/123", method: "patch") do "
" + - "
" + + "
" + "" end @@ -800,10 +800,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", "namespace_edit_post_123", "edit_post", method: "patch") do - "" + - "" + + "" + + "" + "" + - "" + "" end assert_dom_equal expected, output_buffer @@ -877,7 +877,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - "" + "" end assert_dom_equal expected, output_buffer @@ -897,7 +897,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form do - "" + "" end assert_dom_equal expected, output_buffer @@ -912,8 +912,8 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - "" + - "" + "" + + "" end assert_dom_equal expected, output_buffer @@ -928,8 +928,8 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - "" + - "" + "" + + "" end assert_dom_equal expected, output_buffer @@ -943,7 +943,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - "" + "" end assert_dom_equal expected, output_buffer @@ -957,7 +957,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - "" + "" end assert_dom_equal expected, output_buffer @@ -971,7 +971,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - "" + "" end assert_dom_equal expected, output_buffer @@ -985,7 +985,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - "" + "" end assert_dom_equal expected, output_buffer @@ -999,7 +999,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - "" + "" end assert_dom_equal expected, output_buffer @@ -1019,9 +1019,9 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - "" + "" end + whole_form("/posts/123", method: "patch") do - "" + "" end assert_dom_equal expected, output_buffer @@ -1038,8 +1038,8 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + '' + + '' end assert_dom_equal expected, output_buffer @@ -1065,9 +1065,9 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1084,9 +1084,9 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1103,8 +1103,8 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + '' + + '' end assert_dom_equal expected, output_buffer @@ -1121,8 +1121,8 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + '' + + '' end assert_dom_equal expected, output_buffer @@ -1139,9 +1139,9 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1159,9 +1159,9 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1180,11 +1180,11 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + - '' + - '' + '' + + '' + + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1207,11 +1207,11 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + - '' + - '' + '' + + '' + + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1234,10 +1234,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + - '' + '' + + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1260,11 +1260,11 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + - '' + - '' + '' + + '' + + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1283,11 +1283,11 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + - '' + - '' + '' + + '' + + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1307,11 +1307,11 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + - '' + - '' + '' + + '' + + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1330,9 +1330,9 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1351,10 +1351,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + - '' + '' + + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1369,7 +1369,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + '' end assert_dom_equal expected, output_buffer @@ -1386,11 +1386,11 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + - '' + - '' + '' + + '' + + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1407,11 +1407,11 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + - '' + - '' + '' + + '' + + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1442,11 +1442,11 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + - '' + - '' + '' + + '' + + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1465,10 +1465,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + - '' + '' + + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1485,8 +1485,8 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + '' + + '' end assert_dom_equal expected, output_buffer @@ -1502,8 +1502,8 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + '' + + '' end assert_dom_equal expected, output_buffer @@ -1525,8 +1525,8 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + '' + + '' end assert_dom_equal expected, output_buffer @@ -1611,18 +1611,18 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + - '' + - '' + - '' + - '' + - '' + - '' + - '' + - '' + - '' + - '' + - '' + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' end assert_dom_equal expected, output_buffer @@ -1638,7 +1638,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - '' + '' end assert_dom_equal expected, output_buffer @@ -1652,10 +1652,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = - "" + - "" + + "" + + "" + "" + - "" + "" assert_dom_equal expected, output_buffer end @@ -1668,10 +1668,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = - "" + - "" + + "" + + "" + "" + - "" + "" assert_dom_equal expected, output_buffer end @@ -1684,10 +1684,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = - "" + - "" + + "" + + "" + "" + - "" + "" assert_dom_equal expected, output_buffer end @@ -1700,10 +1700,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = - "" + - "" + + "" + + "" + "" + - "" + "" assert_dom_equal expected, output_buffer end @@ -1716,10 +1716,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = - "" + - "" + + "" + + "" + "" + - "" + "" assert_dom_equal expected, output_buffer end @@ -1732,10 +1732,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = - "" + - "" + + "" + + "" + "" + - "" + "" assert_dom_equal expected, output_buffer end @@ -1747,7 +1747,7 @@ class FormWithActsLikeFormForTest < FormWithTest end assert_dom_equal "" + - "", + "", output_buffer end @@ -1758,7 +1758,7 @@ class FormWithActsLikeFormForTest < FormWithTest end assert_dom_equal "" + - "", + "", output_buffer end @@ -1777,10 +1777,10 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", "create-post", method: "patch") do - "" + - "" + + "" + + "" + "" + - "" + "" end assert_dom_equal expected, output_buffer @@ -1797,9 +1797,9 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", "create-post", method: "patch") do - "" + - "" + - "" + "" + + "" + + "" end assert_dom_equal expected, output_buffer @@ -1813,7 +1813,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - "" + "" end assert_dom_equal expected, output_buffer @@ -1837,9 +1837,9 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - "
" + - "
" + - "
" + "
" + + "
" + + "
" end assert_dom_equal expected, output_buffer @@ -1856,9 +1856,9 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - "
" + - "
" + - "
" + "
" + + "
" + + "
" end assert_dom_equal expected, output_buffer @@ -1875,7 +1875,7 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = whole_form("/posts/123", method: "patch") do - "
" + "
" end assert_dom_equal expected, output_buffer @@ -1890,7 +1890,7 @@ class FormWithActsLikeFormForTest < FormWithTest concat f.text_field(:title) end - expected = "
" + expected = "
" assert_dom_equal expected, output_buffer end @@ -1902,7 +1902,7 @@ class FormWithActsLikeFormForTest < FormWithTest concat f.text_field(:title) end - expected = "
" + expected = "
" assert_dom_equal expected, output_buffer end @@ -1915,9 +1915,9 @@ class FormWithActsLikeFormForTest < FormWithTest end expected = - "
" + - "
" + - "
" + "
" + + "
" + + "
" assert_dom_equal expected, output_buffer end -- cgit v1.2.3