-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add an html_attr function to make outputting HTML attributes easier
#3930
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
base: 3.x
Are you sure you want to change the base?
Conversation
b680b5f to
9c29c4d
Compare
fabpot
left a comment
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.
I like it a lot. Let's continue the work. Tell me of you need help @mpdude
extra/html-extra/HtmlExtension.php
Outdated
|
|
||
| $result = ''; | ||
| foreach ($attr as $name => $value) { | ||
| $result .= twig_escape_filter($env, $name, 'html_attr').'="'.htmlspecialchars($value).'" '; |
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.
Let's consider false as a way to disable the attribute? And true would not generate the value part?
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.
I guess that makes sense. I'd have false omit the attribute output altogether. For true, I'd use something like name="name" for backwards compat with (X)HTML.
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.
Almost everywhere else, indeed, false should remove/hide the attribute.
"" should not happen, i guess, but should be dealt with attention, because the following have all the same meaning:
disabled=""disableddisabled="false"disabled="disabled"
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.
For the aria cases, if you want, there is some classic cases + docs here
with some test cases if you need too https://github.com/symfony/ux/blob/647532ab688f79acfbcb1c895e88a8b2f1a502f6/src/Icons/tests/Unit/IconTest.php#L226
(other similar in the TwigComponent / ComponentAttributes)
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.
Except for aria-* attributes that support keyword and enumerated values. In some cases aria-* attributes support true and false string values.
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.
Fortunately, Twig and PHP have separate types for strings and booleans. Passing the string true or false will not trigger the special logic for boolean values.
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.
@stof I guess what I was trying to say is that yes you could pass 'true' or 'false' as parameters but this would require string coercion when setting the value.
The previous comment stated that:
Almost everywhere else, indeed, false should remove/hide the attribute.
I was pointing out that aria-* attributes do accept a string value of 'false' and in those cases a coercion of bool false to string 'false' might be warranted. This could also apply to data-* attributes.
IMO:
nullvalue always removes the attribute from the final string rendering.trueandfalsevalues foraria-*attributes should coerce boolean to their string values'true'and'false'trueandfalsevalues for all other attributes should act as boolean html attributes. ie true renders the attribute, false removes the attribute (as per @fabpot comment).
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.
☝🏻 Agree with that.
Additionally, there are places where "" makes sense, so we should not take users the option to use that and print as attr="".
f65ee8f to
c9acca6
Compare
c7bacfd to
03a5de1
Compare
|
@fabpot Regarding tests: I suppose the fixture-based tests ( What's the best way to cover lots of scenarios in the fixture-style tests? It's easy to get lost when there are lots of cases but just one |
You can write both, but
That's indeed a current limitation. |
extra/html-extra/HtmlExtension.php
Outdated
| return $result; | ||
| } | ||
|
|
||
| public static function htmlAttr(Environment $env, ...$args): string |
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.
Maybe a @param for $args?
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.
Same question as above
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.
this expects array<string, mixed> for each of the value of the variadic argument.
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.
In fact, it can be iterable|string|false|null, since the arguments are first forwarded to htmlAttrMerge().
cf673c1 to
9722108
Compare
|
Added a first load of tests for the |
|
For reference Yii2 has a similar function: https://github.com/yiisoft/yii2/blob/master/framework/helpers/BaseHtml.php#L1966-L2046 The renderTagAttributes method has the following rules:
CraftCMS uses twig and provides an Vuejs v2 -> v3 also went through some changes for |
|
How will merging of attributes that support multiple values work? Eg Example: <div {{ attr(
{'aria-labelledby': 'id1 id2'},
{'aria-labelledby': 'id3'},
) }}></div>Should the result be:
This could also apply to |
993f1b5 to
3aee494
Compare
doc/filters/html_attr_merge.rst
Outdated
| based on conditions. Values like ``false``, ``null`` as well as empty arrays are | ||
| skipped, making conditional merging convenient: |
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.
Does it mean one cannot override 'disabled: true' with later value ?
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.
Need to improve this wording obviously.
It means that in the preceding example, e. g. variant2 could be false, null or an empty array and would be skipped then.
If one of these values itself is an array and contains a key-value pair with a false or null value, that will be used to override...
{ disabled: true }|html_attr_merge({ disabled: false }) will be { disabled: false }.
doc/filters/html_attr_merge.rst
Outdated
| size == 'large' ? {class: ['btn-lg']}, | ||
| size == 'small' ? {class: ['btn-sm']}, |
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.
Is class required to be an array ? If no, i think we should have at least one example with a string value
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.
No, it's not. But...
{ class: 'foo' }|html_attr_merge({ class: 'bar' }) will be { class: 'bar' }, because simple scalar merge semantics apply.
This is what I meant in my initial description with users having to be consistent and disciplined about using either scalars (strings) or arrays.
For attributes where multiple values make sense, users should use arrays. We don't want to know which attributes those are, so we burden the users with knowing the attribute semantics and being consistent about it.
Makes sense?
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.
Maybe 230bcf7 clarifies it.
| Content | ||
| </div> | ||
|
|
||
| {# Output: <div data-config="{"theme":"dark","size":"large"}" data-bool="true">Content</div> #} |
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.
data-false="false" no ?
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.
No. For data-*, true bill be "true", but false will not be printed.
Referring to symfony/ux#3269 (comment), this is the React way of handling these attributes.
I am not doing frontend/client side development myself, but guess it makes sense because that way, you can write el.dataset.boolValue in JavaScript for data-bool-value="true" or a missing data-bool-value attribute?
| namespace Twig\Extra\Html\HtmlAttr; | ||
|
|
||
| /** | ||
| * Interface for attribute values that support custom merge behavior. |
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.
They are not mergeable, they are merging strategies.
And we should precise exactly who is merged into who, what precedence is used, etc.
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.
Do you have a suggestion?
(cc @stof)
| * | ||
| * Returns the new, resulting value as a new instance. Does not modify either $this not $previous. | ||
| */ | ||
| public function mergeInto(mixed $previous): self; |
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.
Should describe types a bit more here... because this will make unsafe to use in thrid-party code, when one would have no guarantee of what it's merged into, and so would not be able to perform the operation :|
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.
Better with bd41ee6?
I am afraid we cannot make any guarantees as to the values one might find in the arrays.
| The ``style`` type is automatically applied by the :ref:`html_attr` function when | ||
| it encounters an array value for the ``style`` attribute. |
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.
This would have been the case for both previous example, no? I mean, what would happen to both examples without the |html_attr_type ?
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.
Yes, it would, because of the default handling of attributes named style with this type.
TBH I am not sure if this type should be exposed in the first place.
WDYT – do you see any situation where one might want to have inline CSS in another attribute? That could be used to improve this section.
|
|
||
| .. code-block:: html+twig | ||
|
|
||
| <div {{ html_attr({class: ['btn', 'btn-primary', 'btn-lg']}) }}> |
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.
Is it possible to use merge multiple syntax like Vue.js does? E.g.: ['btn btn-sm', { 'btn-active': isActive }]
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.
No, currently attribute values can be either scalars or iterables that will have their values concatenated. Having another array inside an array value is probably not going to work with the current implementation, but this might be fixable with special AttributeValue implementations.
What might work (haven't tried it) is:
['btn', 'btn-sm', isActive ? 'btn-active']
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.
Thanks, and is the hash syntax alone supported?
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.
You mean <div {{ html_attr({ class: { type: 'btn' } }) }}>? Sure.
The main purpose I see for using hashes at the level of attribute values is that you can use them to designate elements that can later on be replaced. Like so:
{% set attrs = { class: { type: 'btn', role: 'btn-primary', importance: 'normal' } } %}
...
{% set attrs = attrs|html_attr_merge(hasErrors ? { class: { importance: 'warning' } }) %}
<div {{ html_attr(attrs) }} />
or same, without using the intermediate assignment:
<div {{ html_attr(attrs, hasErrors ? { class: { importance: 'warning' } }) }} />If hasErrors is true, this will print <div class="btn btn-primary warning"/>.
Co-authored-by: Simon André <[email protected]>
Updated: This description has been updated to reflect changes from the discussion up to #3930 (comment).
This PR suggests adding an
html_attrfunction and two filtershtml_attr_mergeandhtml_attr_type. Together, they are intended to make it easier to collect HTML attributes in arrays, in order to pass them in Twig to included templates or macros, and to ultimately print such attribute sets as valid and properly escaped HTML.html_attr_mergecan be used to either merge such arrays over default values, or to override (say, inside a macro) particular values in a given attribute array. As described in #3907, it favors overwriting simple (scalar) attribute values and appending to multi-valued attributes over all the other operations one could conceive (like, for example, replacing a list of two CSSclassnames with two other ones). This is a design decision to keep the API simple and optimized for the primary use case that I see. But, since we're mostly dealing with arrays after all, users are free to do in parallel any other kind of array wrangling they see fit.So, this PR is not trying to design a full-fledged, object-oriented API with all the necessary methods to add, replace, remove attributes; to add, change or toggle elements in "list" style attributes like
class; to provide extension points for custom (arbitrary) attributes or to provide a fluent API to do all that from within PHP code. See symfony/ux#3269 for a Symfony UX component RFC that does that.A little bit of special case handling is present for
aria-*,data-*and inline CSSstyleattributes. But apart from that, there is no special knowledge about the attributes defined in HTML, ARIA or other standards, nor about the structure and sementic of attribute values. The approach works in a generic way, so it should be possible to use it for many custom attributes as well.In order to support "list" style attributes like
class,aria-labelledbyorsrcsetthat may come in different flavors, users have to be disciplined and consistently use iterables (arrays) to represent such attribute values, possibly assisted by thehtml_attr_typefilter (see below).Motivation and practical examples
I have seen repeating patterns when dealing with HTML attributes in Twig templates and macros. Typical examples can be found in Symfony's form theme, where an
attrvariable is present in various blocks.https://github.com/symfony/symfony/blob/4a5d8cf03e1e31d1a7591921c6fa1fe7ec1c2015/src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig#L453-L458
{%- block widget_attributes -%} id="{{ id }}" name="{{ full_name }}" {%- if disabled %} disabled="disabled"{% endif -%} {%- if required %} required="required"{% endif -%} {{ block('attributes') }} {%- endblock widget_attributes -%}Could be along the lines of:
{{ html_attr(attr, { id, name: full_name, disabled: disabled ? true : false, required : required ? true : false }) }}.If
disabledandrequiredwere guaranteed to be booleans (I haven't checked), even better:{{ html_attr(attr, { id, name: full_name, disabled, required }) }}https://github.com/symfony/symfony/blob/4a5d8cf03e1e31d1a7591921c6fa1fe7ec1c2015/src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig#L347-L360
{%- set attr = {} -%} {%- set aria_describedby = [] -%} {%- if help -%} {%- set aria_describedby = aria_describedby|merge([id ~ '_help']) -%} {%- endif -%} {%- if errors|length > 0 -%} {%- set aria_describedby = aria_describedby|merge(errors|map((_, index) => id ~ '_error' ~ (index + 1))) -%} {%- endif -%} {%- if aria_describedby|length > 0 -%} {%- set attr = attr|merge({'aria-describedby': aria_describedby|join(' ')}) -%} {%- endif -%} {%- if errors|length > 0 -%} {%- set attr = attr|merge({'aria-invalid': 'true'}) -%} {%- endif -%}Could be:
{%- set attr = {}|html_attr_merge( help ? { 'aria-describedby': [id ~ '_help'] }, errors|length ? { 'aria-invalid': true, 'aria-describedby': errors|map((_, index) => id ~ '_error' ~ (index + 1)) } ) -%}https://github.com/symfony/symfony/blob/4a5d8cf03e1e31d1a7591921c6fa1fe7ec1c2015/src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig#L470-L481
{% block attributes -%} {%- for attrname, attrvalue in attr -%} {{- ' ' -}} {%- if attrname in ['placeholder', 'title'] -%} {{- attrname }}="{{ translation_domain is same as(false) or attrvalue is null ? attrvalue : attrvalue|trans(attr_translation_parameters, translation_domain) }}" {%- elseif attrvalue is same as(true) -%} {{- attrname }}="{{ attrname }}" {%- elseif attrvalue is not same as(false) -%} {{- attrname }}="{{ attrvalue }}" {%- endif -%} {%- endfor -%} {%- endblock attributes -%}This should basically be the same as
{{ html_attr(attr) }}, ignoring edge cases fornullvalues. Handling of thetranslation_domainmight require a precedinghtml_attr_mergestep to replace values with translations.Finally,
{% set id = 'id value' %} {% set href = 'href value' %} {% set disabled = true %} <div {{ html_attr( { id, href }, disabled ? { 'aria-disabled': 'true' }, not disabled ? { 'aria-enabled' : true }, { class: ['zero', 'first'] }, { class: ['second'] }, true ? { class: 'third' }, { style: { color: 'red' } }, { style: { 'background-color': 'green' } }, { style: { color: 'blue' } }, { 'data-test': 'some value' }, { 'data-test': 'other value', 'data-bar': 'baz' }}, { 'dangerous=yes foo' : 'xss' }, { style: ['text-decoration: underline'] }, ) }}></div>will generate HTML markup:
Details on
html_attr_mergeThis filter merges an
attrstyle array with one or several other arrays given as arguments. All of those arrays should reasonably be mappings, i. e. use keys that denote attribute names, and not sequences or lists with numeric keys.Empty arrays, empty strings or false values in the argument list will be ignored, which can be used to conditionally include values in the merge list like so:
{% set attr = attr|html_attr_merge( condition ? { attrname: "attrvalue", other: "value" } ) %}The merging of attribute values is similar to PHP's
array_mergefunction. Latter (right) values generally override former (left) values, as follows:When two values are to be merged and both are either scalars or objects, the latter (right) value overrides the previous (left) one.
When both values are iterables,
array_merge/spread operator behavior is used: Numeric indices will be appended, whereas non-numeric ones will be replaced. This can be used to override designated elements in sets like CSS classes:{% set attr = { class: ['foo', 'bar'] }|html_attr_merge( { class: { importance: 'normal' } }, critical ? { class: { importance: 'high' } } ) %}To provide more flexibility with regards to different merging strategies, the
MergeInterfaceis provided as a flex point for advanced use cases of power users.When an attribute value that represents a "right hand side" value has to be merged and implements
MergeInterface, itsmergeInto()method will be passed the previous (left hand side) value. That method will return the merge result.Otherwise, when the "left hand side" value implements
MergeInterface,appendFrom()will be passed the new (right hand side) value. Again, that method returns the merge result.Other combinations of values are rejected, an exception is thrown. This is a design decision to clearly and early notify users of combinations that might have unclear or unpredictable results, like merging a string like
'foo'with an array like['bar', 'baz']for aclassattribute – should this override, since one value is a scalar, or append, since the other one is an array?Details on
html_attrThe
html_attr()function prints attribute-arrays as HTML. It will perform appropriate escaping of attribute names and values.One or several attribute arrays can be passed to
html_attr, andhtml_attr_mergewill be used first to merge those.In general, scalar attribute values (including the empty string
'') will be printed as-is. For booleans andnullvalues, the following extra rules apply:aria-*, booleantrueandfalsewill be coalesced to"true"and"false", respectively.data-*, booleantruewill be coalesced to"true", and non-scalar values will be JSON-encodedfalseandnullattribute values will always omit printing of the attribute.truevalues will print the attribute asattributeName="". This is equivalent to printing<... attributeName>, but is X(HT)ML compliant. The user-agent will fill in the attribute's empty default value.These rules derive from the comparison provided at symfony/ux#3269 (comment) that shows how React and Vue as front-end frameworks behave in the same situation.
For values that implement
AttributeValueInterface, itsgetValue(): ?stringmethod will be called first. The attribute will be omitted for anullreturn value, otherwise printed with the returned string.This interface could be used to provide (outside the scope of this PR or even outside Twig itself) classes that could e. g. help building more complex attribute values, like for image
srcset. But the primary reason for adding it was to be able to deal with attributes values that are lists or hashes and need to be printed in different ways:classoraria-labelledbyuse space-separated tokens as their valuesrcsetorsizesfor<img>use comma-separated tokenshtml_attrwill generally print array values as a space-separated list of values.A special case is the attribute name
styleif its value is an array. It will be converted to inline CSS. Values with numeric keys will be printed followed by a;. Non-numeric keys will print a pattern ofkey: value;.Details on
html_attr_typeThe
html_attr_typefilter can be used to convert an array passed into it into implementations of the mentioned interfaces in a few predefined ways. It takes a single argument indicating the type to use – similar to theescapefilter in Twig that knows abouthtml,js,cssand a few more.sstmeansspace separated tokenscstmeanscomma separated tokensstylemeans "inline CSS", for completenessSo, the following will construct an
attrarray for animgtag, wheresizesneeds to be printed separated by commas.{% set attr = { srcset: ['small.jpg 480w']|html_attr_type('cst'), alt: 'A cute kitten' } %} {# amend the srcset #} {% set attr = attr|html_attr_merge({ srcset: ['medium.jpg 800w', 'large.jpg 1200w'] }) %} <img {{ html_attr(attr) />This works because the
SeparatedTokenListthat is used forsstandcstimplements merge behavior where a given value can be extended by merging arrays.Design considerations
Exposing behavior for different attribute types through these interfaces may not be the 100% perfect, nice, automagic solution. But it has the big advantage that we are not committing ourselves to a particular list of attributes for which standards-specific knowledge would have to be put in the code. I would consider that a maintenance nightmare, since it would require us to decide which attribute/special case to support and which not. Every single change in that list would be BC-breaking.
The two interfaces put control over that in the hands of power users or extension authors. Arbitrary ways could be conceived to create instances of these interfaces.
The
html_attr_typefilter provides built-in access to the two types I see relevant in the HTML 5 standard, the space and comma separated token lists.The built-in default conversion of arrays to space-separated token lists should further reduce visibility of that problem for average template authors, who can hopefully ignore the problem most of the time.
Closes #3907, which outlined the initial idea.
TODO:
Co-authored-by: @polarbirke