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

Default role="img" introduced in #737 #750 is a breaking change and should be reverted/have opt-out #788

Closed
pachuka opened this issue Oct 15, 2022 · 3 comments · Fixed by #789

Comments

@pachuka
Copy link
Contributor

pachuka commented Oct 15, 2022

🐛 Bug Report

Default role="img" introduced in #737 #750 is a breaking change and should be reverted/have opt-out.

Not a huge fan of this being defaulted in the way it was. While I appreciate the sentiment this is problematic since now this requires alt text/aria-labels or a role="presentational" override on every SVG output from SVGR (to my understanding, I could be wrong that a label would be required now - https://dequeuniversity.com/rules/axe/4.4/svg-img-alt?application=axeAPI).

Most of our usages of SVGs are for icons used in buttons that already configured for accessibility and the SVG doesn't need to be focusable or explained to the non visual user.

It would at the least be nice if we could opt-out of this default behavior, I noticed we do parse the svgProps but since that's a spread/merge it wont override if we set role to undefined to attempt to unset it.

image

In addition with further research seems like SVGs really shouldn't have their implict role blanket overridden with img as it is: https://www.unimelb.edu.au/accessibility/techniques/accessible-svgs#:~:text=SVG%20Roles,%3D%22graphics%2Dsymbol%22.

To Reproduce

Steps to reproduce the behavior:

@svgr/rollup >= 6.4.x, can view new role in snapshots

Expected behavior

A clear and concise description of what you expected to happen.

No default configuration for the role on SVG, if folks need that behavior they can already pass it in with the existing svgProps on the configuration. Or a way to opt out of this new behavior

Link to repl or repo (highly encouraged)

Please provide a minimal repository on GitHub.

Issues without a reproduction link are likely to stall.

Run npx envinfo --system --binaries --npmPackages @svgr/core,@svgr/cli,@svgr/webpack,@svgr/rollup --markdown --clipboard

Paste the results here:

## System:
 - OS: macOS 12.6
 - CPU: (8) arm64 Apple M1 Pro
 - Memory: 103.20 MB / 16.00 GB
 - Shell: 5.8.1 - /bin/zsh
## Binaries:
 - Node: 14.19.3 - ~/.nvm/versions/node/v14.19.3/bin/node
 - npm: 6.14.17 - ~/.nvm/versions/node/v14.19.3/bin/npm
 - Watchman: 2022.09.26.00 - /opt/homebrew/bin/watchman
## npmPackages:
 - @svgr/rollup: 6.5.0 => 6.5.0 
@pachuka
Copy link
Contributor Author

pachuka commented Oct 16, 2022

Looks like my syntax was slightly wrong for how to pass undefined(it's considered to be a dynamic value passed into the template) to override this new default via svgProps:
https://react-svgr.com/docs/options/#svg-props

    svgProps: { role: "{undefined}" },

Just FYI for anyone who wants the old behavior. I still think this shouldn't be the default, but would suggest if its kept this way that it's explicitly documented what that means for accessibility (labels required on the SVGs) as well as the specific override to get the old behavior?

@gregberge
Copy link
Owner

@pachuka I think you are right. I don't know if it should be the default behaviour or not to be honest. But user should be able to turn it off for sure. I am open for any PR to add a option to disable it. @pachuka would you like to work on it?

@pachuka
Copy link
Contributor Author

pachuka commented Oct 23, 2022

@gregberge I should have time to raise a PR this week. I'm thinking back out the changes then add an accessibility section to the documentation and show how to override it with svgProps if that works for folks? That way we keep the original default behavior but make it clear how it can be customized.

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

Successfully merging a pull request may close this issue.

2 participants