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

fix(utils): make SVGO not remove title #6684

Merged
merged 2 commits into from
Feb 15, 2022
Merged

Conversation

erickzhao
Copy link
Contributor

@erickzhao erickzhao commented Feb 15, 2022

Motivation

By default, SVGR removes the <title> tag from SVG inputs. This hinders a11y since "the <title> element provides an accessible, short-text description of any SVG container element or graphics element."

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

The <title> tag can be set via the SVG React component's title prop, but it's unintuitive that existing <title> tags from the SVG files themselves get stripped beforehand.

This will not break behaviour for existing users since the title prop seems to override the image's default <title> tag if set according to the SVGR API docs:

If titleProp is set to true and no title is provided (title={undefined}) at render time, this will fallback to an existing title element in the svg if exists.

See: https://react-svgr.com/docs/options/#title

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Updated the existing test at packages/docusaurus-utils/src/__tests__/webpackUtils.test.ts.

Related PRs

N/A

By default, SVGR removes the `<title>` tag from SVG inputs.
This hinders a11y since "the `<title>` element provides an
accessible, short-text description of any SVG container
element or graphics element".

Modern browsers also show tooltips on hover for inline
SVG with the `<title>` tag.

See https://developer.mozilla.org/en-US/docs/Web/SVG/Element/title
@netlify
Copy link

netlify bot commented Feb 15, 2022

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: aa63e1c

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/620b5512fb025d0008fd5ede

😎 Browse the preview: https://deploy-preview-6684--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Feb 15, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 49
🟢 Accessibility 100
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 92

Lighthouse ran on https://deploy-preview-6684--docusaurus-2.netlify.app/

@Josh-Cena Josh-Cena changed the title chore(utils): add removeTitle: false to svg loader fix(utils): make SVGO not remove title Feb 15, 2022
Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Was wondering, last time I checked, aria-labelledby is the one that helps with a11y, and title is more of a supplement; I guess it's just because title can be more ergonomically embeded in the SVG?

@Josh-Cena Josh-Cena merged commit 0c6165b into facebook:main Feb 15, 2022
@erickzhao erickzhao deleted the patch-2 branch February 15, 2022 09:15
@erickzhao
Copy link
Contributor Author

aria-labelledby is the one that helps with a11y, and title is more of a supplement;

Hope this isn't an anti-pattern, but I have a use-case where I'm displaying an SVG icon without an element that labels it, so aria-labelledby doesn't exactly work.

@Josh-Cena
Copy link
Collaborator

Yeah, I just checked, seems in SVG land title does help with a11y. All good then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants