-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
Multiple inline SVGs on a page can clash #8297
Comments
See also: #8191 |
Yes, this was discussed in #8191 previously. If someone want to submit a PR enabling to provide custom SVGO options, why not, but I'm not really willing to turn |
Thanks for looking into this! I may not be familiar with the conventions used in this repo, but I'm concerned that closing this issue could suggest to someone doing a web search that this bug is fixed. And we're both in agreement that this is an open bug, right? At the moment, no fix has been put forward. The only way I was able to get around the issue was by using the I would be happy to work on a solution. But I would like us to consider what the options are for workarounds before going headlong into coding. Here are some ideas, I don't know if they are all valid:
Any other options? |
I do think it's a valid bug, but I've temporarily lost my god powers to reopen this. I think it's better if the option is on by default—it's too surprising to users with little benefits. |
IMHO it's not a bug. The bug is you using duplicate ids for multiple SVG elements in the first place. Turning this setting on is a breaking change for users already targeting the id with CSS. And it will make the selectors more complex for users not needing the anti-clashing feature + the class name generation logic may change over time and create more issues. If this setting was safe for 100% of users, it would have been a SVGO default. I agree with the authors of SVGO: this shouldn't be turned on by default. I'd rather have you being annoyed by the clashes, rather than having all other users being confused by some kind of magical behavior. Similarly, if you write a custom React component using Your 4) webpack solution looks fine. At most we would enable you to do exactly that in a more robust way. I don't want Docusaurus to be opinionated on how SVGO is used. If we diverge from default settings, there must be a good reason. If the change is not clearly a benefit for all users, then let's stick to the default. In my case I clearly don't agree with you here and wouldn't want this option to be applied by default to all my Docusaurus sites. In general, I prefer to design Docusaurus features following the extensible web manifesto: flexible low-level primitives first, then shortcuts and strong opinions with ability to opt-out if needed. |
Can you detail what you have in mind exactly? I'm not sure to understand. |
I think there's a long-standing misunderstanding here... SVGO does not preserve your original IDs; it minifies your IDs. And it does so without a global state, so as soon as you have two SVGs with IDs, they are sure to clash. See this in the issue description:
Here's the repro: <svg width="80" height="80" xmlns="http://www.w3.org/2000/svg">
<rect width="100%" height="100%" fill="url(#fuschia-aqua-gradient)" />
<linearGradient id="fuschia-aqua-gradient" x1="0" x2="0" y1="0" y2="1">
<stop offset="0%" stop-color="fuchsia" />
<stop offset="100%" stop-color="aqua" />
</linearGradient>
</svg> <svg width="80" height="80" xmlns="http://www.w3.org/2000/svg">
<rect width="100%" height="100%" fill="url(#yellow-red-gradient)" />
<linearGradient id="yellow-red-gradient" x1="0" x2="0" y1="0" y2="1">
<stop offset="0%" stop-color="yellow" />
<stop offset="100%" stop-color="red" />
</linearGradient>
</svg> When you load those two SVGs onto the same page, the IDs both become "a" and the |
@Josh-Cena, I simplified my minimal repro on Stackblitz this morning and was about to essentially type up the same thing you just wrote. I was also getting the sense that there was a misunderstanding. |
Oh also, I wanted to point out that I think it's telling that SVGR implicitly enables the prefixIds SVGO plugin when SVGO is toggled on. |
Oh I understand better now, I see thanks 👍 I'm ok to apply the same settings as SVGR. We are using SVGR, so why isn't this automatically applied for us? Maybe they turned it on recently or did we override this by mistake? Can turning this on cause troubles for existing users? I guess not because the ids were already modified 🤷♂️ |
In fact, users can't target SVG IDs with external CSS—SVGO removes all unreferenced IDs. This can be symphasized with—all tools I know (i.e. Inkscape, Illustrator) add an ID to literally every path, so it's mostly useless. |
I'm not 100% sure but I think Webpack SVGR exposes two separate configuration variables:
I think that if you use the true/false But that's just my guess. |
This bug is nasty and cost ~5 hours on what should have been a 10 minute task. I think the approach you are considering here is wrong. Instead of enabling
Enabling both On a related note, enabling id minification without handling collisions seems like an insane default from |
Fix I'm using, bear in mind that docusaurus is using plugins: [
function svgFix() {
return {
name: 'svg-fix',
configureWebpack(config) {
const svgRuleIndex = config.module.rules.findIndex((r) => r.test.test('file.svg'))
const svgrConfigIndex = config.module.rules[svgRuleIndex].oneOf.findIndex((r) => {
if (!Array.isArray(r.use) || r.use.length === 0) return false
return r.use[0].loader.indexOf('@svgr/webpack') !== -1
})
if (svgRuleIndex === -1 || svgrConfigIndex === -1) return
config.module.rules[svgRuleIndex].oneOf[svgrConfigIndex].use[0].options.svgoConfig.plugins[0].params.overrides.cleanupIDs = false
}
}
}
] |
Thanks for reporting and opening the discussion on the SVGO. Again I'd rather follow their decision and stay on defaults, but we can make it easier to disable or reconfigure SVGO in Docusaurus. Having less "layers" should make it easier to debug, as you can just try to disable SVGO and see if it fixes things. |
Hey hey! I recently joined as a maintainer to SVGO, so just looking through open issues etc. Just noting that disabling the
I'll look further into this and hopefully find a way to improve the situation. If Docusaurus does want to change the config to reduce the frequency of this, a better move is probably to disable the plugins: [
{
name: 'preset-default',
params: {
overrides: {
removeTitle: false,
removeViewBox: false,
cleanupIds: {
minify: false
}
},
},
},
], |
@SethFalco Thanks for the comment. Question: won't this still break ID references within external CSS files that SVGO isn't aware of but end up on the same page anyway? |
Ahh, yes indeed! I was just focusing on the issue title. For that concern, I guess the only solution really would be for Docusaurus to expose the settings, then a user could choose to use the Sorry if it seems like I'm pulling config names out of nowhere btw, we never really had good user-facing documentation, just the implementation to refer to. I'm working on this now but still a WIP: |
+1 for a solution to this. I think the option to expose the SVGO setting to disable minification on referenced IDs is the way to go. |
### Changelog none ### Description As described in facebook/docusaurus#8297, the default webpack config for docusaurus has svgo configured to replace `id` attributes with simple incrementing characters like `"a"`, `"b"`, etc. This leads to ID conflicts between logos and incorrect clipping behavior. This PR modifies the underlying webpack config to disable changes to ID attributes. Note that on mac, I was only able reproduce the visual clipping issue in Safari; FF and Chrome seemed to ignore the incorrectly duplicated IDs . <table><tr><th>Before</th><th>After</th></tr><tr><td> <img width="1193" alt="image" src="https://github.com/foxglove/mcap/assets/2367265/cd7eb926-6b21-43b6-8ffa-e8b8126612fd"> </td><td> <img width="1161" alt="image" src="https://github.com/foxglove/mcap/assets/2367265/1d3d3a07-beff-4d32-9fba-76cb21aad4e6"> </td></tr></table>
Definitely would be nice to have everything working out of the box. As others said, had to spend a few hours on this issue. Ended up with the solution below. This typescript code adds Note: if you have svgs with the same names but in different folders, remove Plugin codeimport path from "path";
import { Plugin } from "@docusaurus/types";
import { RuleSetRule } from "webpack";
import { Config as SvgrConfig } from "@svgr/core";
export function svgFixPlugin(): Plugin {
return {
name: "svg-fix",
configureWebpack(config) {
const svgRule = config.module?.rules?.find(r =>
(r as { test: RegExp }).test.test("file.svg"),
) as RuleSetRule | undefined;
if (!svgRule) {
console.warn("Failed to apply SVG fix, could not find SVG rule in webpack config!");
return {};
}
const svgrLoader = svgRule.oneOf?.find(
r =>
((r as RuleSetRule).use as object[] | undefined)?.length === 1 &&
((r as RuleSetRule).use as { loader: string }[])?.[0].loader.includes(
"@svgr/webpack",
),
);
if (!svgrLoader) {
console.warn(
"Failed to apply SVG fix, could not find svgr loader in webpack config!",
);
return {};
}
const svgoConfig = (svgrLoader.use as { options: SvgrConfig }[])[0].options.svgoConfig;
if (!svgoConfig?.plugins) {
console.warn(
"Failed to apply SVG fix, could not find svgo config in webpack config!",
);
return {};
}
svgoConfig.plugins.push({
name: "prefixIds",
params: {
delim: "_",
prefix: (element, file) => {
return path.basename(file?.path ?? "").split(".")[0];
},
prefixIds: true,
prefixClassNames: true,
},
});
return {};
},
};
} |
FYI Docusaurus v3.7 will have an SVGR plugin to let you fix SVGR/SVGO-related config issues on your own: #10677 I'll consider this issue fixed because technically you'll now have the escape hatches to adjust SVGR/SVGO to your own needs, or plainly disable it. In Docusaurus v4 will likely refactor those configs to better defaults. Please help us doing so here if you have an opinion (#10679) |
Have you read the Contributing Guidelines on issues?
Prerequisites
npm run clear
oryarn clear
command.rm -rf node_modules yarn.lock package-lock.json
and re-installing packages.Description
Including multiple inline SVGs on a page like so:
can result in the SVGs clashing with each other if those SVGs use ids internally.
The reason the inline SVGs aren't working is because of the way Docusaurus has configured its SVG loader internally. Docusaurus uses the Webpack loader for SVGR. SVGR in turn uses SVGO. By default, SVGO minifies SVG ids. When those ids get minified to single characters (like "a", "b", "c"), they can clash on the page. SVGO has an option to prefix ids, but Docusaurus has not turned that option on. I created a minimal reproduction of the bug. You can comment out one SVG at a time from the source code, and see that when only one SVG is on the page, it renders properly. But when both are on the page, one clashes with the other.
I think one possible way to fix this would be to add the SVGO option
prefixIds: true
just below line 123 in webpackUtils.ts.Reproducible demo
https://stackblitz.com/edit/github-ptytnu?file=src/pages/index.js
Steps to reproduce
All I did to create the minimal repro was to start with a fresh Docusaurus install using new.docusaurus.io, upload two SVGs that I know clash, then import and render those two SVGs into the existing index page.
Expected behavior
The SVGs should look the way they look when rendered individually.
Actual behavior
The SVGs are colored and painted incorrectly because the gradients or masks in one SVG are being applied on top of another.
Your environment
If you wish to dig in more, you can look at the history of the pull request I was working on when I ran into this bug:
Self-service
The text was updated successfully, but these errors were encountered: