[TwigComponent] Reconsider behaviour for `(bool) false` value for ARIA attributes

[polarbirke]

#1805 removed the ability to render ARIA attributes with a (string) false value by passing a (bool) false value to the attributes helper.

Example:

{# templates/components/Button.html.twig #}
<button {{ attributes }}>Toggle Foo</button>

{# render component #}
<twig:Button aria-expanded="{{ user.alwaysShowFoo }}" />

{# renders as: #}
user.alwaysShowFoo
? <button aria-expanded="true">Toggle Foo</button>
: <button>Toggle Foo</button>

{# desired output: #}
user.alwaysShowFoo
? <button aria-expanded="true">Toggle Foo</button>
: <button aria-expanded="false">Toggle Foo</button>

The reason given in #1797 (comment) is a prior decision to use (bool) false to signal that an attribute should be omitted.

This decision is generally fine for HTML attributes.

I would have preferred to finetune the behaviour for ARIA attributes in a slightly more nuanced way than is already the case. As was suggested in #1797 (comment), there would be a viable alternative to use null to not render an ARIA attribute and have (bool) false cast to a string value of "false". It feels off to have special behaviour for ARIA attributes for (bool) true only.

I would like to submit that the discussion is reopened.

[smnandre]

Hi @polarbirke!

This is a very interesting topic

#1805 removed the ability to [...]

Well.. to be fair, #1805 fixed a major BC break introduced (by me) two weeks earlier, when this capability was first added.

It feels off to have special behaviour for ARIA attributes for (bool) true only.

That special case was mostly a DX shortcut to simplify usage, in response to user requests. But I agree it is not perfect right now, and this kind of 'special behaviour' was originally something I wanted to avoid (see my first reaction).

A complex situation

The reality is that we probably will not find a perfect solution here, because HTML is complex:
https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#enumerated-att

Some attributes, called enumerated attributes, take on a finite set of states. The state for such an attribute is derived by combining the attribute's value, a set of keyword/state mappings, and three possible special states that can also be given in the specification of the attribute. These special states are the invalid value default, the missing value default, and the empty value default.

Like, pretty complex:
https://developer.mozilla.org/en-US/docs/Web/API/Element/ariaExpanded

A string with one of the following values:

• "true": The grouping element this element owns or controls is expanded.
• "false": The grouping element this element owns or controls is collapsed.
• "undefined": The element does not own or control a grouping element that is expandable.

I mean, annoyingly complex:
https://w3c.github.io/aria/#typemapping

What can we do?

UX 2.0

For most users today, false is the way to unset an attribute. That is how it worked historically.

Also, there are now many more people using TwigComponent, directly or via Sylius, PrestaShop, EasyAdmin, etc.

So, to maintain the Symfony BC promise, we cannot change this behaviour in UX 2.0.

UX 3.0

To me, there is no real added value in swapping false and null here. It would inevitably lead to confusion and errors between documentation versions, compatibility issues, and so on.

More broadly, I am not entirely sure we should try to solve this inside TwigComponent at all. We have seen similar issues in Icons, and we had a similar one in Stimulus not so long ago, etc.

UX ??

But I do have a proposal that could address this problem at a broader level (at least across the whole UX initiative). Sorry if I cannot write it up tonight: I will post it tomorrow morning at the latest.

Spoiler: a dedicated component.

[smnandre]

--> #3269

[mpdude]

Disclaimer: I'm affiliated with the OP, and we worked on twigphp/Twig#3930 together.

💯 agree regarding the complexity of attribute definitions and the difficulties of understanding the specs, lest implementing them correctly.

The observation was that it is surprising to have special handling of true for aria-* to coalesce to "true", whereas false does not lead to "false" but instead omits the attribute. That means it falls back to its missing default value, which may be different from "false".

WAI-ARIA may have tristate or undefined in their type list, but the two boolean values consistently are mapped to "true" and "false" in the suggested type mapping.

Regarding BC:

The problem is that using null as an attribute value is deprecated as of UX 2.8 and will currently be interpreted as true. So, the only way in 2.x to make any attribute disappear in the attributes helper (ARIA or not) currently is the false boolean value.

What could be done:

• Change the current deprecation, so that null values will not throw in the next major, but instead will omit the attribute
• In UX 3, trigger a deprecation notice when false is used for an aria-* attribute; stating that null should be used instead to omit the attribute (which is what false does until then). Announce that false will be coalesced to "false" in 4.0.
• In UX 4, consistently coalesce true and false for aria-* to "true" and "false", respectively.

[smnandre]

Disclaimer (..)

🤝 for this great PR !

The observation was that it is surprising to have special handling of true for aria-* to coalesce to "true", whereas false does not lead to "false" but instead omits the attribute. That means it falls back to its missing default value, which may be different from "false".

Again, I totally agree here :)

But replacing a specific incomplete treatment for aria-* by another specific incomplete... I'm not entirely sold here.

So, the only way in 2.x to make any attribute disappear in the attributes helper (ARIA or not) currently is the false boolean value.

Yes, and so moving back to force users using currently false is not an improvment for them 🤷

Do we want them to receive potentially hundreds of deprecations per page ?

In UX 4, consistently coalesce true and false for aria-* to "true" and "false", respectively.

This is also weird .. look at aria-checked, or aria-busy ... Not that simple. I've opened this RFC for this specific subject, as it would allow us different behaviour per aria attribute.. but I also understand and agree with lot of @Kocal comments ...

---

To be honest, i'm not sure what the ideal direction is here. :)

[mpdude]

I am not sure I understand your points correctly, my apologies.

But replacing a specific incomplete treatment for aria-* by another specific incomplete... I'm not entirely sold here.
[...] This is also weird .. look at aria-checked, or aria-busy ... Not that simple.

Do you mean that this would not cover ARIA tri-state attribute values like "true"/"false"/"mixed", and thus it would be "incomplete"?

Referring to the table at #3269 (comment):

null values are consistently used in both React and Vue to suppress an attribute for data-* and aria-*, and probably work the same for other HTML attributes (although the table does not mention that). Also twigphp/Twig#3930 and twigphp/Twig#4405 handle it that way. Only TwigComponents plan to throw on null values in 3.0.

Conversely, both React and Vue print "true" and "false" for true/false values in aria-* attributes. Again, that's what the aforementioned PRs intend to do. Only TwigComponents is asymmetric in this regard with special case code for true, but not for false.

It is as it is, and I am not blaming anyone for this situation. In contrary, thanks to you and anyone who worked on that in the past and stepped up to make decisions 💚 .

I was only suggesting that we could push this into a direction where it would be more consistent and in line with the other frameworks. We could do so with clean deprecation messages in 2.x and 3.x and reach that goal in 4.0. And we could do it regardless of how #3269 develops.

Yes, and so moving back to force users using currently false is not an improvment for them 🤷
Do we want them to receive potentially hundreds of deprecations per page ?

I cannot tell for symfony/UX specifically. But when I contributed to such discussions elsewhere, the expected number of deprecation messages for users was not a reason as long as everybody agreed it would improve things.

[smnandre]

@mpdude Thank you for this answer and the clear and convincing arguments.

I was only suggesting that we could push this into a direction where it would be more consistent and in line with the other frameworks.

I assure you this is all I want too.. as long:

1. "in line with others" doesn't hurt DX for this framework
2. we have no BC break

---

So we have multiple questions to answer here:

• should we use null as marker to remove an attribute ?
• should we use false as marker to remove an attribute ?
• should we make null THE standard method to remove an attribute ?
• should we convert false to "false" for aria-* ?
• should we have different behaviour for aria-* ?

Here my personal answers would be:

1. Should we use null as marker to remove an attribute ?

I cannot thin of a reason against, so i'd be okay with this.

2. Should we use false as marker to remove an attribute ?

false is much easier to produce in Twig than null, and has been the DX for people the last 3 years.

In most case (ignoring aria-* for now), asking them to change would not have any added value for them.

So i'd really would like we continue to allow it.

3. Should we make null THE standard method to remove an attribute ?

Based on my previous answer, I don't think so.

4. Should we have different behaviour for aria-*

As aria- attributes are a bit different, I do agree we could (this was what my first PR on this topic was about)

5. Should we convert false to "false" for aria-* ?

Again, I don't oppose that. At all.

My point was more to say this is also in fact an imperfect answer. Asymetry now is not in the lib, but in the generated HTML, as we will generate invalid HTML on other attributes (aria-labelledby="false" for instance).

And imperfect because indeed, a special treatment for all aria attributes and not for enumerated ones (with no default attributes equivalent to false) is by definition imperfect.

Would it entirely remove all problems ? Nope.

Would the overall situation be better ? Certainly (again, this is why I tried to do this at the time).

---

We could do so with clean deprecation messages in 2.x and 3.x and reach that goal in 4.0

What would we deprecate in 2.x ? Not sure to understand ?

In UX 3, trigger a deprecation notice when false is used for an aria-* attribute; stating that null should be used instead to omit the attribute (which is what false does until then).

We would have two different methods to remove an attribute.

But... if someone absolutely wants an unique method, they can use null everywhere, so 👍

---

But when I contributed to such discussions elsewhere, the expected number of deprecation messages for users was not a reason as long as everybody agreed it would improve things.

I may have been a bit affected by some things around here last year.. it tends to make me more defensive than I should be here (something @Kocal tells me once a week).. I'll try to make some positivity effort :)

Be sure I'm very very happy to read such PR, and to have this discussion :)

[smnandre]

Good for you @Kocal @mpdude ? Or do you want a radical move ?

[Kocal]

If your solution is identical to what React/Vue does, it's good 😇

[polarbirke]

Thank you all for the illuminating discussion. 🙏

It seems to me there are two main viewpoints that can be argued:

1. good DX is to have (bool) false do the exact same thing for all attributes with no exceptions (=> omit rendering)
2. good DX is to have convenient boolean state-to-string casting for ARIA attributes in line with popular JS frameworks

I am in favour of the second one (2), personally¹, but I can understand why the first is appealing.

Let me address a specific counterargument against (2) that you mentioned:

we will generate invalid HTML on other attributes (aria-labelledby="false" for instance)

In general I would like to presume that developers know what attribute values are valid and prepare them accordingly, but if developers were currently passing string|false as the attribute value of aria-labelledby because they have learned to rely on the current behaviour, (2) would indeed be a breaking change for them. Which is why it should be introduced slowly over the next 2 versions, so developers can act on deprecation notices on their own time and pass string|null if their code is set up that way. Everyone who used a long-form condition would be unaffected:

{%- if ariaId is not empty -%}
    {% set attr = attr|merge({ 'aria-labelledby': ariaId }) %}
{%- endif -%}

I'm looking forward to see what final consensus will look like. Thanks a lot for engaging with such a controversial rabbit hole!

Footnotes

1. For two main reasons: it fits my mental modal 😄 and it's in line with the model of other frameworks, so there is less cognitive load when switching. ↩

[mpdude]

Thank you Simon for keeping track of this. Personally, I remained silent for the past two weeks because I wanted to see what direction the discussion at twigphp/Twig#3930 takes, and it had a few twists that came surprising to me. If we had reached consensus on that, being consistent would be one more argument.

1. Should we use null as marker to remove an attribute ?

👍 we agree

2. Should we use false as marker to remove an attribute ?

If I do not misunderstand your answer, I think we agree: It should remain possible to use both null and false (or null aka none and false respectively in Twig templates) to omit the attribute, except for aria-* where false will be introduced to mean "false" in a backwards compatible way.

3. Should we make null THE standard method to remove an attribute ?

Apart from their difference at aria-*, both false and null are equally valid.

But, referring to this comparison table, null is the only value that consistently omits an attribute across all cases and in both mentioned JavaScript frameworks.

So, maybe not "THE standard", but the recommended way?

4. Should we have different behaviour for aria-*

👍

5. Should we convert false to "false" for aria-* ?

Again, I don't oppose that. At all.

🎉

Let me see if I can come up with a PR.

[mpdude]

#3307 – my first contribution here, hope I get it right