Skip to content

[Icons] Add support for <title> and <desc> elements in SVG for accessibility #2904

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: 2.x
Choose a base branch
from
Open

[Icons] Add support for <title> and <desc> elements in SVG for accessibility #2904

wants to merge 7 commits into from

Conversation

xDeSwa
Copy link
Contributor

@xDeSwa xDeSwa commented Jul 11, 2025

Q A
Bug fix? no
New feature? yes
Docs? yes
Issues
License MIT

When passing title and/or desc as attributes to ux_icon(), they are now embedded directly into the SVG as <title> and child elements.

Each <title> and element is given a unique id and automatically referenced via aria-labelledby, if no such attribute was already set.

This ensures compliance with accessibility best practices for inline SVG content.
See:

https://developer.mozilla.org/en-US/docs/Web/SVG/Element/title

https://developer.mozilla.org/en-US/docs/Web/SVG/Element/desc

Example Usage:

{{ ux_icon('bi:plus-square-dotted', {
    width: '16px',
    height: '16px',
    class: 'text-success',
    title: 'Add Stock',
    desc: 'This icon indicates stock entry functionality.'
}) }}

Renders:

<svg class="text-success" width="16px" height="16px" aria-labelledby="icon-title-abc icon-desc-def">
    <title id="icon-title-abc">Add Stock</title>
    <desc id="icon-desc-def">This icon indicates stock entry functionality.</desc>
    <!-- inner SVG content -->
</svg>

xDeSwa added 2 commits July 12, 2025 00:50
@xDeSwa
Add support for <title> and <desc> elements in SVG for accessibility
b9f41c9 
When passing title and/or desc as attributes to ux_icon(), they are now embedded directly into the SVG as <title> and <desc> child elements.

Each <title> and <desc> element is given a unique id and automatically referenced via aria-labelledby, if no such attribute was already set.

This ensures compliance with accessibility best practices for inline SVG content.
See:

    https://developer.mozilla.org/en-US/docs/Web/SVG/Element/title
    https://developer.mozilla.org/en-US/docs/Web/SVG/Element/desc
@xDeSwa
Add support for <title> and <desc> elements in SVG for accessibility
423937e
@carsonbot carsonbot added Feature New Feature Status: Needs Review Needs to be reviewed labels Jul 11, 2025
Kocal
Kocal requested changes Jul 12, 2025
View reviewed changes
Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

Thanks for working in this!

Please be sure to add tests

src/Icons/src/Icon.php Outdated
Comment on lines 155 to 161
$titleId = 'title-' . bin2hex(random_bytes(4));
$labelledByIds[] = $titleId;
$a11yContent .= sprintf('<title id="%s">%s</title>', $titleId, htmlspecialchars((string) $title, ENT_QUOTES));
}

if ($desc) {
$descId = 'desc-' . bin2hex(random_bytes(4));
Copy link
Member

Choose a reason for hiding this comment

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

I feel like it's a not a good idea to generate random strings for eacg title/desc, especially when you render a lot of icons in a single page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"IDs for the <title> and elements are generated only when the aria-labelledby attribute is automatically added to reference them."

Copy link
Member

@Kocal Kocal Jul 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I know, but I don't think generating random values like that is a good thing, it can be CPU intensive and it's untestable.

Instead, either we generate these ids based on the icon (name, title, description, ...), either we use an internal incremental counter or something similar

@carsonbot carsonbot added Status: Needs Work Additional work is needed and removed Status: Needs Review Needs to be reviewed labels Jul 12, 2025
@Kocal Kocal added the Icons label Jul 12, 2025
@Kocal Kocal changed the title Add support for <title> and <desc> elements in SVG for accessibility [Icons] Add support for <title> and <desc> elements in SVG for accessibility Jul 12, 2025
@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Jul 12, 2025
@xDeSwa
Copy link
Contributor Author

xDeSwa commented Jul 12, 2025 •
edited
Loading

Thanks for working in this!

Please be sure to add tests

Thanks for suggestions. can you check again please.

@xDeSwa xDeSwa requested a review from Kocal July 12, 2025 10:44
Kocal
Kocal requested changes Jul 12, 2025
View reviewed changes
Copy link
Member

@Kocal Kocal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm sorry, but I don't understand the benefits over using <title> and <desc>, instead of using aria-label and aria-description attribute. Do you have more information about that?

Tests are still missing, please make sure to add tests for making the CI green, thanks

src/Icons/doc/index.rst
Comment on lines +510 to +512
<svg class="text-success" width="16px" height="16px" aria-labelledby="icon-title-abc icon-desc-def">
<title id="icon-title-abc">Add Stock</title>
<desc id="icon-desc-def">This icon indicates stock entry functionality.</desc>
Copy link
Member

Choose a reason for hiding this comment

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

The id attributes do no match the actual behaviour

src/Icons/src/Icon.php Outdated
Comment on lines 185 to 203
// Special case for aria-* attributes
// https://www.w3.org/TR/wai-aria-1.1/#state_prop_def
if (true === $value && str_starts_with($name, 'aria-')) {
if ($value === true && str_starts_with($name, 'aria-')) {
$value = 'true';
}

$htmlAttributes .= ' '.$name;
if (true === $value) {

$htmlAttributes .= ' ' . $name;

if ($value === true) {
continue;
}

$value = htmlspecialchars($value, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8');
$htmlAttributes .= '="'.$value.'"';
$htmlAttributes .= '="' . $value . '"';
}

return '<svg'.$htmlAttributes.'>'.$this->innerSvg.'</svg>';

// Inject <title> and <desc> before inner content
return '<svg' . $htmlAttributes . '>' . $a11yContent . $innerSvg . '</svg>';
Copy link
Member

Choose a reason for hiding this comment

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

Please revert all changes here, except for $a11yContent usage, PR must contains only the necessary changes

@carsonbot carsonbot added Status: Needs Work Additional work is needed and removed Status: Needs Review Needs to be reviewed labels Jul 12, 2025
@Kocal
Copy link
Member

Kocal commented Jul 12, 2025

And apply Fabbot suggestions aswell, thanks

@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Jul 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kocal Kocal Kocal requested changes

Assignees
No one assigned
Labels
Feature New Feature Icons Status: Needs Review Needs to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xDeSwa @Kocal @carsonbot