Skip to content
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

feat(docs): added accessibility guide #2122

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

feat(docs): added accessibility guide #2122

wants to merge 12 commits into from

Conversation

karsa-mistmere
Copy link
Member

closes #2121

What is the purpose of this pull request?

  • Documentation update

Before Submitting

@github-actions github-actions bot added 📖 documentation Improvements or additions to documentation 🌍 site Has to do something with the Lucide website labels May 3, 2024
Copy link
Member

@jguddas jguddas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The content of the images is not accessible, the alt text is different from the content of the image. I would generally prefer if we moved the text out of the images and made them just normal text.
  • Would be nice to have a section on flashing images and use reduce motion as well
  • Also aria-label is not perfect, using something like RadixUIs AccessibleIcon is something that we should point people to use instead.
  • A section on who this for and why you should do this would also be nice.

docs/images/a11y/alttext-buttons.svg Outdated Show resolved Hide resolved
docs/guide/advanced/accessibility.md Outdated Show resolved Hide resolved
@jguddas
Copy link
Member

jguddas commented May 3, 2024

We can also point people to resources that they should reference, be it just the WCAG or even component libraries or guides like https://inclusive-components.design/toggle-button/

@karsa-mistmere
Copy link
Member Author

karsa-mistmere commented May 3, 2024

  • The content of the images is not accessible, the alt text is different from the content of the image. I would generally prefer if we moved the text out of the images and made them just normal text.

For the most part this is entirely intentional, these are illustrations, I've added accessible text where there was information otherwise missing, but most of these illustrations contain redundant information that is not useful to be read out as is. There's probably room for improvement, on that I can agree.

Would be nice to have a section on flashing images and use reduce motion as well

We don't really have ootb support for such animations, so I'm not entirely sure if it's relevant within the context of Lucide.

Also aria-label is not perfect, using something like RadixUIs AccessibleIcon is something that we should point people to use instead.

Well, the current guide does recommend not using it in places where it's problematic, but you're right, there probably space for some more nuance about this.

A section on who this for and why you should do this would also be nice.

Is it not for everyone using Lucide? 🧌

We can also point people to resources that they should reference, be it just the WCAG or even component libraries or guides like https://inclusive-components.design/toggle-button/

Great idea, we should definitely add a "Further resources" section!

@jguddas
Copy link
Member

jguddas commented May 3, 2024

A section on who this for and why you should do this would also be nice.

Is it not for everyone using Lucide? 🧌

Example: Your should give your icons enough contrast so people with visual impairments, be it physical or just them being outside in the glaring sun can still get the amazing benefit of looking at those cute icons.

@jguddas
Copy link
Member

jguddas commented May 3, 2024

Would be nice to have a section on flashing images and use reduce motion as well

We don't really have ootb support for such animations, so I'm not entirely sure if it's relevant within the context of Lucide.

Tailwind based example: You can do <DogIcon className="animate-ping" /> but what you should consider using <DogIcon className="motion-safe:animate-ping" /> instead.

@karsa-mistmere
Copy link
Member Author

Would be nice to have a section on flashing images and use reduce motion as well

We don't really have ootb support for such animations, so I'm not entirely sure if it's relevant within the context of Lucide.

Tailwind based example: You can do <DogIcon className="animate-ping" /> but what you should consider using <DogIcon className="motion-safe:animate-ping" /> instead.

Yeah, but this is entirely dependant on Tailwind, I think it should be their responsibility to educate people about using their animations responsibly. :)

(To be frank, this should be their default, it shouldn't be something to opt into.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could even set aria-hidden="true" on decorative icons like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some actuall user research on this, having decortive images be read out as image by your screen reader is apperently super annoying.

So 👍 for adding aria-hidden="true".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused, does this mean that simple SVGs with no accessible text are currently being read out? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The read-out text for voiceover for example is "You are currently on an image. To begin interacting with the contents of this image, press Control-Option-Shift-Down Arrow.", that's so super not helpful for decorative images.

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh jeez louise, that's awful. 🤮

+1 for adding aria-hidden then. Best course of action would actually be to automatically add that no matter what.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hiding the icon by default sounds super dangerous.

As I see it there are 2 okayish options:

  1. Add a default label that is a descriptive representation of the icon. Set default `aria-label` or add `<title>` to SVG for accessibility #2121
  2. Make setting a label / aria-hidden attribute mandatory and well documented for when to use what.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Add a default label that is a descriptive representation of the icon. Set default `aria-label` or add `<title>` to SVG for accessibility #2121

Terrible-terrible idea, as I've already explained in #2121:

  • Accessible labels on functional icons should describe the use case, not the graphical representation (and even that isn't necessarily unambiguous, especially considering abstract icons).
  • Decorative icons on the other hand should have no accessible label whatsoever.
  1. Make setting a label / aria-hidden attribute mandatory and well documented for when to use what.

Supporting any kind of accessible label would lead to breaking changes, since we cannot use either <title> nor aria-label="..." as already discussed in previous threads. That means we would have to add at least one extra element to the DOM and inject CSS where there previously were none (or use inline CSS for our visually-hidden util).

Hiding the icon by default sounds super dangerous.

I don't really see why, it is practically never advisable to set an accessible label on an icon itself, but instead on other interactive elements, or using some kind of visually-hidden/sr-only util class, but that by nature will be on a different element as well.

Copy link
Member

@ericfennis ericfennis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review.
This is such an awesome guide, thank you @karsa-mistmere super useful!
We can improve this even more if we add some small code examples for some things to make it visible in the code.

I've provided some suggestions and one comment

screen readers, leading to nonsensical text.

![](../../images/a11y/alttext-buttons.svg?raw=true)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#### In code
```tsx
// Don't do this
<button>
<Plus aria-label="Plus icon"/>
Add document
</button>
// Do this, just leave it
<button>
<Plus />
Add document
</button>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericfennis: These could be useful, but they break up the guide quite a bit, I've hidden them within details elements.

contained icon.

![](../../images/a11y/alttext-iconbuttons.svg?raw=true)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#### In code
```tsx
// Don't do this
<button>
<Home />
</button>
// Do this
<button aria-label="Go to home">
<Plus />
</button>

Comment on lines 122 to 123
generally recommend against this for and instead suggest that
you [use your chosen CSS framework's "visually hidden" utility whenever possible](https://gomakethings.com/revisting-aria-label-versus-a-visually-hidden-class/).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why we should be "against" this? It would be nice if we explained this more in the text, about why using "visually hidden" is better than using aria-label, we also could provide some sources for blog posts or documentation to prove our "recommendation".

Also maybe this reads a bit nicer

Suggested change
generally recommend against this for and instead suggest that
you [use your chosen CSS framework's "visually hidden" utility whenever possible](https://gomakethings.com/revisting-aria-label-versus-a-visually-hidden-class/).
generally recommend visually hiding textual descriptions. For example by [using utility CSS framework's "visually hidden"](https://gomakethings.com/revisting-aria-label-versus-a-visually-hidden-class/).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means something different though. Also, the link itself is the resource that explains why .visually-hidden > [aria-label], I've rephrased the link text so that it's clearer.

Suggested change
generally recommend against this for and instead suggest that
you [use your chosen CSS framework's "visually hidden" utility whenever possible](https://gomakethings.com/revisting-aria-label-versus-a-visually-hidden-class/).
generally recommend against this and instead suggest that you use your chosen CSS framework's "visually hidden" utility whenever possible. You can [read more about why `aria-label` might not be the best solution](https://gomakethings.com/revisting-aria-label-versus-a-visually-hidden-class/).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📖 documentation Improvements or additions to documentation 🚀 Lucide v1 🌍 site Has to do something with the Lucide website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants