[Icons] Improve aria attributes rendering

[smnandre]

Fix: #1796

• Convert aria boolean values to "true/false" strings (as previously done in TwigComponent)
• Fix aria-hidden is added when source icon contains aria label
• Add pre-rendering filter chain in IconRenderer (internal)

Update:

After discussion in this issue, i suggest we keep the current behaviour: "boolean false removes an attribute"

Attribute \ Value	true	false	"true"	"false"
required	required		required="true"	required=false"
aria-required	aria-required="true"		aria-required="true"	aria-required="false"

So just something to ease the DX but that let the developer responsible of setting a correct value.

[norkunas]

Can we do this only for these attributes?: aria-atomic, aria-busy, aria-checked, aria-disabled, aria-expanded, aria-grabbed, aria-haspopup, aria-hidden, aria-invalid, aria-multiline, aria-multiselectable, aria-pressed, aria-readonly, aria-required, aria-selected

ref #1709 (comment)

[smnandre]

First of all, sorry for the BC @norkunas

I'm very not found of having distinct behaviour for specific attributes (one per one i mean), that will be something hard to maintain.

I'd suggest another option: just hande "true" (boolean) for aria-attribute

That would work like this

Attribute \ Value	true	false	"true"	"false"
required	required		required="true"	required=false"
aria-required	aria-required="true"		aria-required="true"	aria-required="false"

So just something to ease the DX but that let the developer responsible of setting a correct value.

WDYT ?

[norkunas]

It's NOT a very big list to maintain, so imho to restore previous behavior it's easy to check for those attributes, and I have a WIP code for twig components

[smnandre]

My suggestion would not work for you ?

[norkunas]

If you mean that real boolean false will omit the attribute from being rendered, then probably yes, okay for me :)

[smnandre]

As it's already the case for all attributes.

The only aria specific behaiour would be transforming bool true into string "true" .

I think this could be the easiest to maintain/document

[norkunas]

I think 'Passing "null" as an attribute value is deprecated and will throw an exception in 3.0.' was wrong, and it could easily solve this problem for me - I'd use null for omitting attributes, and then false or true could be casted to strings without a problem for me. When this deprecation was added I needed to switch from null to false

[smnandre]

I think 'Passing "null" as an attribute value is deprecated and will throw an exception in 3.0.' was wrong, and it could easily solve this problem for me

Well we cannot change this now 🤷‍♂️

Users do want to use booleans in their code (that was a recurrent demand, same thing for icon, CVA, live attributes ...)

So i think my proposition works, but let's see what others think

@kbond @WebMamba (and everyone else :) )?

[WebMamba]

I feel like aria is a topic more complex than that and we tried to be too opinioned about that. If we look at the doc here: https://w3.org/WAI/WCAG21/Techniques/aria/ARIA24 they don't recommend setting the aria-label to true. WDYT ?

[smnandre]

No one wants or recommands to set the aria-label to true :) Where did you read that ?

[norkunas]

I think 'Passing "null" as an attribute value is deprecated and will throw an exception in 3.0.' was wrong, and it could easily solve this problem for me

Well we cannot change this now 🤷‍♂️

And why not? 🙂 undeprecating is easy, isn't it? 🙂

[smnandre]

And why not? 🙂 undeprecating is easy, isn't it? 🙂

I should have quoted more text :)

A choice has been made to use the boolean "false" to remove an attribute, as many persons asked, i don't see myself asking people to switch again and lose this feature (that would be a BC break).

[kbond]

Thanks Simon.