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

Improve a11y in Cart icon #5

Open
YordanSoares opened this issue Feb 5, 2021 · 7 comments
Open

Improve a11y in Cart icon #5

YordanSoares opened this issue Feb 5, 2021 · 7 comments
Assignees

Comments

@YordanSoares
Copy link
Contributor

A WP.org user suggested improving the a11y for the Cart icon (Accessibility improvements) replacing <i> tags wrapper for <span> instead.

I did some quick research and found these related reading:

@alexmigf
Copy link
Member

alexmigf commented Feb 5, 2021

Curious, I did a test with Axe and no errors are showed, maybe because we use an aria attribute already:

$menu_item_icon = '<i class="wpmenucart-icon-shopping-cart-'.$icon.'" role="img" aria-label="'.__( 'Cart','woocommerce' ).'"></i>';

@YordanSoares
Copy link
Contributor Author

Indeed, I also saw that, so we met the aria attribute side, but I wonder if replacing the <i> tags for <span> could improve even more a11y and semantic.

@alexmigf
Copy link
Member

alexmigf commented Feb 8, 2021

@YordanSoares see this: FortAwesome/Font-Awesome#16790 (comment)

Of course we could replace <i> for <span> if @Spreeuw agrees with that.

@alexmigf
Copy link
Member

Here using <i> the cart icon doesn't show, replacing with <span> solves the issue.

@Spreeuw do you agree with the replacement?

@Spreeuw
Copy link
Contributor

Spreeuw commented Jun 25, 2021

@alexmigf I think that's actually a bad example. The offending style from the theme is this:

.header-style10 .header-bottom .container i

If that was a style targeting span (which is more likely to happen, actually), we'd have the same issue...

@alexmigf
Copy link
Member

Sorry, I wasn't finding the route cause for that, I see now it was the opacity in that style.

@Spreeuw
Copy link
Contributor

Spreeuw commented Jun 21, 2022

I'm hesitant to change this for backwards compatibility reasons, and FontAwesome being an industry leading example still using <i> too, I see no reason to switch. Meanwhile, we can prepare our code/CSS to no longer be dependent on the <i> element so that if we change it to <span> in the future, the transition will be more smooth.

That said, if adding aria-hidden="true" also fixes the issue, I'm fine with that (as long as it doesn't negatively affect the regular behavior).

@Terminator-Barbapapa Terminator-Barbapapa self-assigned this Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants