ux icon indicating copy to clipboard operation
ux copied to clipboard

[Icons][DX] Accessibility helpers

Open yguedidi opened this issue 1 year ago • 10 comments

Based on the proposed documentation update here from @smnandre, what do you think about the following helpers:

  • ux_icon_accessible(icon, label, attributes) for informative and functional icons, will enforce the label parameter and always add the aria-label attribute
  • ux_icon_decorative(icon, attributes) for obviously decorative icons, will always add the aria-hidden attribute set to true

I can try to come with a PR if the idea get some traction, but I'm fine if someone else takes it :)

yguedidi avatar Mar 18 '24 22:03 yguedidi

I think this could be automatic, based on the arguments passed.

Something like

  • you pass a label -> aria-label,
  • else we set it aria-hidden

WDYT ?

I'm not sure we should add multiple functions, as this is harder to dynamise in templates.

(poke @kbond)

smnandre avatar Mar 18 '24 22:03 smnandre

I would love that the unique ux_icon supports that! but then I thought about what would be its API, and I found 2 options:

  • ux_icon(icon, label, attributes) but not sure it's OK to have the label before attributes is OK for general purpose, do we want to enforce it? should it be passed as null for decorative? should named argument be used for attributes so skip label?
  • ux_icon(icon, attributes, label) could work for decorative ones but I find it a bit weird to have the label after attributes for informative/functional ones (but clearly a personal feeling here)

Another idea that pops on my mind could be to make ux_icon kind of polymorphic, supporting both ux_icon(string icon, array attributes) and ux_icon(string icon, string label, array attributes) and adapt the behavior depending on the type of the second argument. But do we accept such "magic"?

yguedidi avatar Mar 18 '24 23:03 yguedidi

We should add a bit of magic there if we want people to follow best practices :)

What about this :

{# A- Without label #}
<twig:Ux:Icon name="foo:bar" />

{# B- With label #}
<twig:Ux:Icon name="foo:bar" label="Profile" />

{# C- With aria-label #}
<twig:Ux:Icon name="foo:bar" aria-label="Profile" />
<!-- A- No accessible text: hidden -->
<svg aria-hidden="true"  role="presentation none">
<!--...--> 
</svg>

<!-- B- Label -> aria-label -->
<svg aria-label="Profile">
<!--...--> 
</svg>

<!-- C- Same result -->
<svg aria-label="Profile">
<!--...--> 
</svg>

Some readings i'd need to check more in depth :

  • https://www.w3.org/TR/graphics-aria-1.0/#roles
  • https://svgwg.org/svg2-draft/struct.html#implicit-aria-semantics

Update: same thing for Twig functions

{{ ux_icon('foo:bar', {label: "Profile"}) }}

smnandre avatar Mar 18 '24 23:03 smnandre

My initial intent with dedicated functions is to enforce to pass the label to follow accessibility best practices when using an icon in an informative/functional purpose. That's why I went for dedicated functions, to throw an error if you don't pass the label when using the accessible variant, and not supporting a label at all for decorative ones.

Alternative idea could be to have the existing ux_icon require the label and have a single new function ux_icon_decorative (or ux_icon_hidden) without label. Or the other way around.

What do you think?

yguedidi avatar Mar 19 '24 00:03 yguedidi

I don't really like the idea of adding additional functions. Is there an example elsewhere of twig enforcing accessibility best practices? My feeling is that this (and twig in general) is a lower level.

That being said, this issue is still very valid. I wonder if the attribute customization system @smnandre is working on could accommodate.

kbond avatar Mar 21 '24 02:03 kbond

This is maybe something we could suggest in a more DX-friendly-way, via the WDT / profiler. It could be pretty easy to parse the DOM and find icons that are only child of links and have no label / title / etc.

My position, for now, would be

  • having multiple methods will not encourage people to use them, as they will stay with what they use the most
  • the same logic could justify a lot of other variants, and same thing i don't think it's something we want

I also think that

  • accessibility is an important matter
  • as package developpers, we have a responsability to allow / ease / encourage best practices

And in the same time i do think that

  • we cannot enforce something against the will of the developpers
  • there are a loooot of things more important to do in Twig "usage/practices" to improve on this matter
  • and as @kbond said Twig is a low-level tool, not an end-user product

So yeah, let's try to find smart ways to educate & motivate people (ourselves first certainely), while keeping things simple and future-proof :)

smnandre avatar Mar 21 '24 02:03 smnandre

I agree with all your points! Just one thing, the proposed support for accessibility is not on Twig, that is indeed low-level, but on UX Icons, and I believe accessibility is part of UX 🙂 I find the solution proposed by @smnandre in https://github.com/symfony/ux/issues/1629#issuecomment-2005308626 a very good option: always add the aria-hidden="true" except if a label or aria-label is explicitly passed.

yguedidi avatar Mar 29 '24 23:03 yguedidi

but on UX Icons, and I believe accessibility is part of UX 🙂

Fair point!

I find the solution proposed by @smnandre in #1629 (comment) a very good option: always add the aria-hidden="true" except if a label or aria-label is explicitly passed.

I'm good with this.

kbond avatar Mar 29 '24 23:03 kbond

thanks @kbond! @smnandre do you want to build your solution or can I give it a try (I never contributed PRs to Symfony UX yet 😅)

yguedidi avatar Mar 29 '24 23:03 yguedidi

I started some things, but i'll need your feedback / vision @kbond

I want to (when all done) have several "attributesRenderer" and "innerSvgRender" (or even one Interface with those two method" and a RendererChain (that's the idea not the perfect name i guess)

But i'm not too sure on how to deal with the subtle difference (IconRenderer currently get the "name" ... and some of what i just suggested currently cannot have it)....

So we could

  • A) add name and prefix (or just name) in Icon (personal vision: it makes sense.... dont know what could be sides effects.. but like that an icon does not know they are identifies in ur system.
  • B) create a IconMetadata (that would at a short-but-still-longer-than-A to keep "icon" / "prefix" / ... in a value we have access to.. but for now the data would only be "name" or "name:prefix" so .... (later on i'd like to have cachestatus, file info, iconset info (or a short version of) and initial storage of icon (locale, iconify, ...)
  • C) it's already blurry enough in my head ^^

So @kbond if/when you have some time (even next week) poke me :))

smnandre avatar Mar 30 '24 03:03 smnandre