Skip to content

[Icons] Set aria-hidden="true" when title/label not set #1690

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
Merged
merged 1 commit into from
Apr 9, 2024
Merged

[Icons] Set aria-hidden="true" when title/label not set #1690

merged 1 commit into from
Apr 9, 2024

Conversation

smnandre
Copy link
Member

@smnandre smnandre commented Apr 7, 2024

Q A
Bug fix? no
New feature? yes
Issues Fix #1629
License MIT

Add "aria-hidden=true" when the icon has no title, aria-label or aria-labelledby attribute

(option enabled per default)

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Apr 7, 2024
WebMamba
WebMamba reviewed Apr 7, 2024
View reviewed changes
Copy link
Contributor

@WebMamba WebMamba left a comment

Choose a reason for hiding this comment

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

Why instead of an aria-hidden attribute not fill the aria-label with the name from the metadata ?

@smnandre
Copy link
Member Author

smnandre commented Apr 7, 2024

Why instead of an aria-hidden attribute not fill the aria-label with the name from the metadata ?

I don't remember precisely where, but i posted some links about it in the original issue discussion...

aria-hidden is really the best thing to do when an icon is used with a label (like the green "open" badge on top of this screen)

But your solution could absolutely be something to consider

(that's why i'm inclined to keep the configuration, there are so many use cases i'd rather no "force" things for users that don't want some attributes.... or have very good reasons to not want them)

yguedidi
yguedidi suggested changes Apr 7, 2024
View reviewed changes
Copy link

@yguedidi yguedidi left a comment

Choose a reason for hiding this comment

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

Thank you @smnandre for taking this!
so in the end no special label attribute accepted? I'm fine with the title one only, just curious

@carsonbot carsonbot added Status: Needs Work Additional work is needed Status: Needs Review Needs to be reviewed and removed Status: Needs Review Needs to be reviewed Status: Needs Work Additional work is needed labels Apr 7, 2024
@smnandre smnandre requested a review from yguedidi April 8, 2024 20:34
@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Apr 8, 2024
@smnandre
Copy link
Member Author

smnandre commented Apr 8, 2024

Why instead of an aria-hidden attribute not fill the aria-label with the name from the metadata ?

I don't remember precisely where, but i posted some links about it in the original issue discussion...

aria-hidden is really the best thing to do when an icon is used with a label (like the green "open" badge on top of this screen)

But your solution could absolutely be something to consider

(that's why i'm inclined to keep the configuration, there are so many use cases i'd rather no "force" things for users that don't want some attributes.... or have very good reasons to not want them)

Ok for you @WebMamba ?

@smnandre smnandre mentioned this pull request Apr 8, 2024
Closed
@WebMamba
Copy link
Contributor

WebMamba commented Apr 9, 2024

Hum no 😁 I don't think it should be a configuration, it should be a behavior that can be overwrite directly when you use the component. And we should document it

@smnandre
Copy link
Member Author

smnandre commented Apr 9, 2024

Hum no 😁 I don't think it should be a configuration, it should be a behavior that can be overwrite directly when you use the component. And we should document it

So people would need to add "aria-hidden: false" for all their icons ?

That's a niche use case, so why not yes

@kbond
Copy link
Member

kbond commented Apr 9, 2024 •
edited
Loading

so in the end no special label attribute accepted?

Is label a valid html attribute? If not, I'd prefer keeping as is to keep the complexity to a minimum.

I don't think it should be a configuration, it should be a behavior that can be overwrite directly when you use the component.

I think this should be the default behaviour for the reasons discussed in #1629 but I want to hear your hesitation @WebMamba. Is there an alternate way we can achieve? My thought for this was adding to the default_icon_attributes configuration option but because some logic is required, this seems better.

@smnandre
Copy link
Member Author

smnandre commented Apr 9, 2024

Is label a valid html attribute? If not, I'd prefer keeping as is to keep the complexity to a minimum.

Yep that was my reasoning to not implement this in the end. I don't want to confuse users with non-existing svg attributes.

WebMamba
WebMamba approved these changes Apr 9, 2024
View reviewed changes
Copy link
Contributor

@WebMamba WebMamba left a comment

Choose a reason for hiding this comment

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

Thanks @smnandre! This DX looks good! Sorry to ask you to rewrite all the tests 😁

src/Icons/src/IconRenderer.php
if ([] === array_intersect(['aria-hidden', 'aria-label', 'aria-labelledby', 'title'], array_keys($attributes))) {
$iconAttributes['aria-hidden'] = 'true';
}
if ([] === array_intersect(['aria-hidden', 'aria-label', 'aria-labelledby', 'title'], array_keys($attributes))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

simple curiosity where did you get this list?

Copy link
Member Author

Choose a reason for hiding this comment

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

An image’s alt attribute.
A SVG’s child title element.
Using aria-label or aria-labelledby on an image or SVG.
Using a title attribute on an image or SVG.

https://www.scottohara.me/blog/2019/05/22/contextual-images-svgs-and-a11y.html

kbond
kbond requested changes Apr 9, 2024
View reviewed changes
@carsonbot carsonbot added Status: Needs Work Additional work is needed Status: Needs Review Needs to be reviewed and removed Status: Reviewed Has been reviewed by a maintainer Status: Needs Work Additional work is needed labels Apr 9, 2024
@smnandre smnandre requested a review from kbond April 9, 2024 23:14
@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Apr 9, 2024
@kbond kbond force-pushed the icons/aria-hidden-auto branch from f50f456 to 36600e3 Compare April 9, 2024 23:25
@kbond
Copy link
Member

kbond commented Apr 9, 2024

Thanks Simon.

@kbond kbond merged commit 722ac82 into symfony:2.x Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbond kbond kbond approved these changes

@yguedidi yguedidi yguedidi approved these changes

@WebMamba WebMamba WebMamba approved these changes

Assignees
No one assigned
Labels
Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Icons][DX] Accessibility helpers
5 participants
@smnandre @WebMamba @kbond @yguedidi @carsonbot