Remove trailing slash from void HTML elements #51646
Conversation
Always respect the `open` argument regardless of the value of the `void_element_trailing_slash` configuration.
| @@ -245,7 +247,7 @@ def tag_string(name, content = nil, options, escape: true, &block) | |||
| content_tag_string(name, content, options, escape) | |||
| end | |||
|
|
|||
| def self_closing_tag_string(name, options, escape = true, tag_suffix = " />") | |||
| def self_closing_tag_string(name, options, escape = true, tag_suffix = (void_element_trailing_slash == false ? ">" : " />")) | |||
There was a problem hiding this comment.
I'm not sure I understand why this change is being made. Is this method being used somewhere outside of define_self_closing_element and define_void_element? The idea in 3438979 was that this method is for self closing elements but can be specialized for void elements by passing in the suffix.
Since omitting the / is not valid for self-closing elements, I'm not sure it should ever be the default for this method.
There was a problem hiding this comment.
This method is used for both: void elements and the self closing SVG tags.
I moved the configuration check to the void element definition method and changed the default value to the tag closing without the ending slash at dcf7b87. This is because going forward, the ending slash should be rather an exception (i.e. SVGs) than the default.
There was a problem hiding this comment.
This is because going forward, the ending slash should be rather an exception (i.e. SVGs) than the default.
Right, I don't think this is a good idea. Void elements are a special case of self-closing tags where the / is optional instead of required. If the default is > then every caller wanting a self-closing tag must pass in /> to get a valid tag. If the default is /> then passing in > is just an optimization for void tags but the method won't generate invalid HTML.
There was a problem hiding this comment.
The default can be changed back as per this request but I'll wait for the answer to the other question before doing that.
- When using `load_defaults "7.1"` or below, default the new configuration to `true` - When using `load_defaults "7.2"` or higher, default the new configuration to `false`
|
@rafaelfranca @skipkayhil Thank you both for your kind reviews and valuable feedback. The issues pointed out in the reviews are now resolved. |
| tag_suffix = ActionView::Helpers::TagHelper.void_element_trailing_slash ? " />" : ">" | ||
| self_closing_tag_string("#{name}", options, escape, tag_suffix) |
There was a problem hiding this comment.
Are there negative performance impacts from introducing this conditional into each individual call?
Is it possible to extract the suffix String definition so that it's outside the batch.push(<<~RUBY) string, then interpolated back into the method String?
There was a problem hiding this comment.
There is not a real difference between these two the implementations.
The difference is one if statement which is linear at runtime. The string generation is done regardless of this if statement being there or not (i.e. when passing the last parameter as a constant string).
Note that the same if statement is also run at the tag helper every time.
Of course, there is an impact on performance every time more conditionals are added but at least the time complexity of both implementations is the same (linear).
If you would prefer, I can implement the change but it won't affect the performance in any way.
There was a problem hiding this comment.
I still don't understand why this method is being changed. It already does what you want by generating tags with > instead of />.
To me, the new config is about transitioning code that currently uses /> to use > instead. But since this already uses > I don't think there's any reason to make it suddenly do the wrong thing even if temporarily (and in my opinion changing it like this is actually backwards incompatible).
There was a problem hiding this comment.
@skipkayhil You are correct that it changes current behavior.
But without this change, e.g. these elements won't have the closing slash even if config.action_view.void_element_trailing_slash is set to true:
This source is generated by following the getting started guide of Rails.
It is fine by me to revert the change but then we are not respecting the configuration option in all cases, which was part of the fixes implemented after the previous review.
It's just a question what we want here:
- Keeping the current behavior of having mixed void HTML tags (what is requested here)
- Respecting the new configuration option in all cases (what was requested in the review)
There was a problem hiding this comment.
Depending on how https://github.com/rails/rails/pull/51646/files#r1578010728 is resolved, could the tag_suffix definition be inlined into the interpolation?
Then, that value could be interpolated directly into the evaluated Ruby method:
| tag_suffix = ActionView::Helpers::TagHelper.void_element_trailing_slash ? " />" : ">" | |
| self_closing_tag_string("#{name}", options, escape, tag_suffix) | |
| self_closing_tag_string("#{name}", options, escape, "#{ | |
| ActionView::Helpers::TagHelper.void_element_trailing_slash ? " />" : ">" | |
| }") |
There was a problem hiding this comment.
Furthermore, is there a reason that the void_element_trailing_slash attribute need to be defined on ActionView::Helpers::TagHelper? Could it be defined on the ActionView::Helpers::TagHelper::TagBuilder class directly?
If the mattr_accessor :void_element_trailing_slash, default: false line were moved to the TagBuilder class, that configuration could be accessed directly from the class method, instead of as a fully-qualified class variable:
| tag_suffix = ActionView::Helpers::TagHelper.void_element_trailing_slash ? " />" : ">" | |
| self_closing_tag_string("#{name}", options, escape, tag_suffix) | |
| self_closing_tag_string("#{name}", options, escape, "#{void_element_trailing_slash ? " />" : ">"}") |
There was a problem hiding this comment.
Depending on how https://github.com/rails/rails/pull/51646/files#r1578010728 is resolved, could the tag_suffix definition be inlined into the interpolation?
Yes this is of course possible. But I'll just stress that it has no implication on performance at least to my knowledge.
I can do this change if it is insisted but I will wait for the feedback regarding the question on how to proceed with this, as there is the question above which is the desired direction with this.
Furthermore, is there a reason that the void_element_trailing_slash attribute need to be defined on ActionView::Helpers::TagHelper? Could it be defined on the ActionView::Helpers::TagHelper::TagBuilder class directly?
It is also needed at TagHelper#tag method.
I don't have a preference where the configuration option is defined, I just thought it would be natural at the lowest nesting level where it is needed.
There was a problem hiding this comment.
@skipkayhil @rafaelfranca Could you chime in which would be the preferred direction forward, so that we could finalize this.
So there are two alternatives, as pointed out above:
- Not to change this method which would make the HTML inconsistent regarding the introduced configuration flag, i.e. when the ending slashes are preferred, the HTML would be inconsistent and not respect the configuration option in all places.
- Following the current direction where the configuration option is respected everywhere, but implement the changes requested here
Which one both of you think is the desired approach?
Motivation / Background
This Pull Request has been created because W3C HTML validator gives a lot of the following type of notices about the trailing slashes on void HTML elements:
Detail
This Pull Request changes the trailing slash
/>on void HTML elements to>as suggested by the W3C validator.This changes also the default value of the
taghelper'sopenargument fromfalsetoniland reflects this change in the documentation of the method.The trailing slashes are removed from all void elements but preserved for the self-closing elements within SVGs, i.e.
animate,animateMotion,animateTransform,circle,ellipse,line,path,polygon,polyline,rect,set,stop,useandviewas defined here:rails/actionview/lib/action_view/helpers/tag_helper.rb
Lines 112 to 125 in 185157f
This is to prevent any SVGs breaking due to this change.
In addition, a new configuration value
config.action_view.void_element_trailing_slashis added that can be used to revert to the legacy behavior, i.e. keeping the trailing slashes on the void elements. This is to help migrating to newer versions and to provide this as an option in case some implementations rely on the current behavior.Additional information
Fix #51643
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]