From 343897980cf9c1a8c1d102df3afb0631f5c9145b Mon Sep 17 00:00:00 2001 From: Hartley McGuire Date: Fri, 24 Nov 2023 11:24:07 -0500 Subject: [PATCH] Deprecate content for void elements in TagBuilder According to the [HTML5 Spec][1] > Void elements can't have any contents (since there's no end tag, no > content can be put between the start tag and the end tag). Up to this point, the only special handling of void elements has been to use ">" to close them instead of "/>" (which is optional but valid according to the spec) > Then, if the element is one of the void elements, ... , then there may > be a single U+002F SOLIDUS character (/), ... . On void elements, it > does not mark the start tag as self-closing but instead is unnecessary > and has no effect of any kind. This commit deprecates the ability to pass content (either through the positional "content" parameter or a block) to a void element since it is not valid according to the Spec. This has the benefit of both encouraging more correct HTML generation, and also simplifying the method definition of void elements once the deprecation has been removed. This commit additionally tweaks the signature of "void_tag_string" slightly with two changes. The first change is renaming it to be "self_closing_tag_string". This is more accurate than "void_tag_string" because the definition of "void element" is more specific and has more requirements than "self closing element". For example, tags in the SVG namespace _can_ be self closing but the "/" at the end of the start tag is _not_ optional because they are not void elements. The second change to this method is swapping from a boolean "self_closing" parameter to a string "tag_suffix" parameter. This enables the void element method definition to specialize the tag_suffix (to just ">") without either void elements or self closing elements having to pay the runtime cost of the self_closing conditional since we know at method definition time which suffix each type of tag should use. [1]: https://html.spec.whatwg.org/multipage/syntax.html#void-elements --- actionview/CHANGELOG.md | 4 +++ .../lib/action_view/helpers/tag_helper.rb | 31 ++++++++++++++----- actionview/test/template/tag_helper_test.rb | 8 +++-- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md index c1d56bada9..d12513b52c 100644 --- a/actionview/CHANGELOG.md +++ b/actionview/CHANGELOG.md @@ -1,3 +1,7 @@ +* Deprecate passing content to void elements when using `tag.br` type tag builders. + + *Hartley McGuire* + * Fix the `number_to_human_size` view helper to correctly work with negative numbers. *Earlopain* diff --git a/actionview/lib/action_view/helpers/tag_helper.rb b/actionview/lib/action_view/helpers/tag_helper.rb index 01e7629f68..dbf79c4f39 100644 --- a/actionview/lib/action_view/helpers/tag_helper.rb +++ b/actionview/lib/action_view/helpers/tag_helper.rb @@ -58,22 +58,39 @@ def #{method_name}(content = nil, escape: true, **options, &block) end end - def self.define_void_element(name, code_generator:, method_name: name.to_s.underscore, self_closing: false) + def self.define_void_element(name, code_generator:, method_name: name.to_s.underscore) code_generator.define_cached_method(method_name, namespace: :tag_builder) do |batch| batch.push(<<~RUBY) def #{method_name}(content = nil, escape: true, **options, &block) if content || block - tag_string(#{name.inspect}, content, escape: escape, **options, &block) + ActionView.deprecator.warn <<~TEXT + Putting content inside a void element (#{name}) is invalid + according to the HTML5 spec, and so it is being deprecated + without replacement. In Rails 7.3, passing content as a + positional argument will raise, and using a block will have + no effect. + TEXT + tag_string("#{name}", content, escape: escape, **options, &block) else - void_tag_string(#{name.inspect}, options, escape, #{self_closing}) + self_closing_tag_string("#{name}", options, escape, ">") end end RUBY end end - def self.define_self_closing_element(name, **options) - define_void_element(name, self_closing: true, **options) + def self.define_self_closing_element(name, code_generator:, method_name: name.to_s.underscore) + code_generator.define_cached_method(method_name, namespace: :tag_builder) do |batch| + batch.push(<<~RUBY) + def #{method_name}(content = nil, escape: true, **options, &block) + if content || block + tag_string("#{name}", content, escape: escape, **options, &block) + else + self_closing_tag_string("#{name}", options, escape) + end + end + RUBY + end end ActiveSupport::CodeGenerator.batch(self, __FILE__, __LINE__) do |code_generator| @@ -228,8 +245,8 @@ def tag_string(name, content = nil, escape: true, **options, &block) content_tag_string(name, content, options, escape) end - def void_tag_string(name, options, escape = true, self_closing = false) - "<#{name}#{tag_options(options, escape)}#{self_closing ? " />" : ">"}".html_safe + def self_closing_tag_string(name, options, escape = true, tag_suffix = " />") + "<#{name}#{tag_options(options, escape)}#{tag_suffix}".html_safe end def content_tag_string(name, content, options, escape = true) diff --git a/actionview/test/template/tag_helper_test.rb b/actionview/test/template/tag_helper_test.rb index da02f3984e..76ccf26394 100644 --- a/actionview/test/template/tag_helper_test.rb +++ b/actionview/test/template/tag_helper_test.rb @@ -27,11 +27,15 @@ def test_tag_builder_void_tag end def test_tag_builder_void_tag_with_forced_content - assert_equal "
some content
", tag.br("some content") + assert_deprecated(ActionView.deprecator) do + assert_equal "
some content
", tag.br("some content") + end end def test_tag_builder_void_tag_with_empty_content - assert_equal "

", tag.br("") + assert_deprecated(ActionView.deprecator) do + assert_equal "

", tag.br("") + end end def test_tag_builder_self_closing_tag