Multiple inline SVGs on a page can clash

gabalafou commented 2 years ago

Have you read the Contributing Guidelines on issues?

[X] I have read the Contributing Guidelines on issues.

Prerequisites

[X] I'm using the latest version of Docusaurus.
[ ] I have tried the npm run clear or yarn clear command.
[ ] I have tried rm -rf node_modules yarn.lock package-lock.json and re-installing packages.
[X] I have tried creating a repro with https://new.docusaurus.io.
[ ] I have read the console error message carefully (if applicable).

Description

Including multiple inline SVGs on a page like so:

import FooSvg from '@site/static/img/foo.svg';
import BarSvg from '@site/static/img/bar.svg';

export default function SomePage() {
  return (
    <div>
      <FooSvg />
      <BarSvg />
    </div>
  );

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:

https://github.com/nebari-dev/nebari-docs/pull/207/

Self-service

[X] I'd be willing to fix this bug myself.

Josh-Cena commented 2 years ago

slorber commented 2 years ago

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 prefixIds: true by default or hardcode this setting.

gabalafou commented 2 years ago

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 img tag. But I can imagine that that wouldn't work for everyone's use case.

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:

Surface SVGO options to Docusaurus config.
Add a note in the docs pointing users to something like this shadow DOM trick on Stack Overflow.
Instead of turning prefixIds on, turn off the SVGO plugin cleanupIds.

Use configureWebpack. I really don't know how to do deep Webpack merge magic, but I verified that the following brittle code works:

// docusaurus.config.js
// ...
plugins: [
  async function prefixSvgIdsPlugin() {
    return {
      name: 'prefix-svg-ids',
      configureWebpack(config) {
        const svgRule = config.module.rules.find(rule => rule.test?.source === '\\.svg$');
        if (svgRule) {
          const {
            oneOf: [
              {
                use: [
                  {
                    options: {
                      svgoConfig
                    }
                  }
                ]
              }
            ]
          } = svgRule;
          svgoConfig.plugins.push('prefixIds');
        }
      }
    }
  }
],
// ...

Any other options?

Josh-Cena commented 2 years ago

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.

slorber commented 2 years ago

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 <div id="already-used-by-docusaurus/>, we don't want to apply a magical algo to rewrite this clashing id for you. It's your responsibility to fix the clashing ids you introduced. Eventually your tooling/CI should tell you there's a clash.

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.

slorber commented 2 years ago

Add a note in the docs pointing users to something like this shadow DOM trick on Stack Overflow.

Instead of turning prefixIds on, turn off the SVGO plugin cleanupIds.

Can you detail what you have in mind exactly? I'm not sure to understand.

Josh-Cena commented 2 years ago

The bug is you using duplicate ids for multiple SVG elements in the first place.

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:

By default, SVGO minifies SVG ids. When those ids get minified to single characters (like "a", "b", "c"), they can clash on the page.

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 fill both become url(#a)!

gabalafou commented 2 years ago

@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.

gabalafou commented 2 years ago

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.

slorber commented 2 years ago

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 🤷‍♂️

Josh-Cena commented 2 years ago

Can turning this on cause troubles for existing users?

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.

gabalafou commented 2 years ago

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?

I'm not 100% sure but I think Webpack SVGR exposes two separate configuration variables:

svgo (boolean)
svgoConfig (object of key-value mappings)

I think that if you use the true/false svgo boolean, then SVGR turns on the prefixIds plugin. But if you use svgoConfig (as Docusaurus does), I think you're on your own.

But that's just my guess.

chancehudson commented 1 year ago

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 prefixIds, why not disable cleanupIds?

cleanupIds minifies ids without consideration of the global context. Most editing tools already include id minification and prefix additions. Disabling the cleanupIds option delegates id management to the export tool. It also reduces confusion about how SVGs are processed by webpack (e.g. ids are left alone). Maybe it's not a perfect long term solution, but seems simple/backward compatible enough that it could be pushed through quickly.

Enabling both cleanupIds and prefixIds seems insane to me. Are the prefixes random? How much entropy is there? If i add 100 svgs each with 10,000 ids am I risking an id collision?? Less magic in the processing = less stuff to debug.

On a related note, enabling id minification without handling collisions seems like an insane default from svgo.

chancehudson commented 1 year ago

Fix I'm using, bear in mind that docusaurus is using svgo@2.8.0 which has a different case for the cleanupIDs option:

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
      }
    }
  }
]

slorber commented 1 year ago

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.

SethFalco commented 1 year ago

Hey hey! I recently joined as a maintainer to SVGO, so just looking through open issues etc.

Just noting that disabling the cleanupIds plugin will reduce the frequency, but will not resolve the issue entirely, as raised in the issue opened in SVGO:

I think cleanupIds is a sensible default. However, disabling this isn't a viable solution because IDs can always conflict whether they pass through SVGO or not. Something will need to be done to actively prevent it in the context of Docusaurus.

— https://github.com/svg/svgo/issues/1714#issuecomment-1735045116

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 minify parameter of cleanupIds rather than disable cleanupIds entirely. That way, unused IDs are still removed, but referenced ones just won't be minified, which is what's ultimately causing the clashes.

plugins: [
  {
    name: 'preset-default',
    params: {
      overrides: {
        removeTitle: false,
        removeViewBox: false,
        cleanupIds: {
          minify: false
        }
      },
    },
  },
],

Josh-Cena commented 1 year ago

@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?

SethFalco commented 1 year ago

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 preservePrefixes parameter of Cleanup IDs to preserve the ones they want left intact.

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:

https://svgo.dev/docs/plugins/cleanupIds/

techbridgedev commented 12 months ago

+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.

shadowusr commented 5 months ago

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 prefixIds svgo plugin configured to add file name prefix to svg ids. You just need to include this plugin in your docusaurus config and everything should start working fine.

Note: if you have svgs with the same names but in different folders, remove path.basename and either use the full path or some more sensible value for your project.

Plugin code

```typescript import 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 {}; }, }; } ```

facebook / docusaurus