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

Remove the removeViewBox plugin from the default plugins list #1461

Merged
merged 1 commit into from
May 24, 2024

Conversation

JohnAlbin
Copy link
Contributor

@JohnAlbin JohnAlbin commented Mar 31, 2021

Given the discussions in #1128, we should remove the removeViewBox plugin from the list of default plugins.

The stated goal of svgo is to optimize SVGs while not changing their renderings. While SVGs can be rendered in many different contexts, one import context is rendering inside HTML documents. When an svg is placed inline with other HTML, removing the viewBox attribute (even when it is a duplicate of the data in the width/height attributes) does change how the svg is rendered.

According to the SVG 1.1 specification, section 7.7 The ‘viewBox’ attribute:

It is often desirable to specify that a given set of graphics stretch to fit a particular container element. The viewBox attribute provides this capability.

When svgo removes the viewBox attribute, it removes that SVG's ability to be scaled to fit its containing HTML element via CSS. In other words, the Scalable Vector Graphic is no longer scalable.

Here's a codepen with an extensive example: https://codepen.io/johnalbin/pen/yLggyXv You can see from the screenshot below that the SVG does not scale properly when CSS resizes the SVG without the viewBox attribute.

screenshot of codepen URL above showing how viewBox affects rendering

Almost all responsive web design websites use this (or a slight variant of the following CSS):

img,
svg {
  max-width: 100%;
  height: auto;
}

When that CSS is used, the browser treats the width and height attributes as the intrinsic size of the image as it scales the SVG canvas (what the SVG spec calls the viewport), but the contents of the SVG (what the spec calls the user space coordinates) are not scaled unless the viewBox attribute is present.

That means removing the viewBox attribute changes the rendering of the SVG. And, from the svgo homepage:

SVG files, especially those exported from various editors, usually contain a lot of redundant and useless information. This can include editor metadata, comments, hidden elements, default or non-optimal values and other stuff that can be safely removed or converted without affecting the SVG rendering result. (emphasis mine)

Which means the removeViewBox should not be included in the list of default plugins.

Here's the PR that fixes this bug.

@ryanb
Copy link

ryanb commented Jun 2, 2021

Any update on this? It would be great to get this merged in.

I just finished adding back a bunch of missing viewBox attributes because svgo stripped them. I assumed svgo default settings would not strip anything important. This PR would have saved me a lot of time.

webdesserts added a commit to webdesserts/app-scripts that referenced this pull request Jun 18, 2021
svgo has been getting a bit over aggressive with its optimizations and I'm generally hand optimizing my svg anyway

see svg/svgo#1461
webdesserts added a commit to webdesserts/scripts that referenced this pull request Jul 5, 2021
svgo has been getting a bit over aggressive with its optimizations and I'm generally hand optimizing my svg anyway

see svg/svgo#1461
@rossmoody
Copy link
Contributor

I noticed this as I was working through my little browser extension that uses SVGO and thought it was odd. The default configuration made it much less dependable to scale the svgs I'm scraping. I removed it from the "default config" as well.

@lzl124631x
Copy link

lzl124631x commented Sep 10, 2021

+1 on this. Without the viewBox I can't scale the svg icons (while good scaling support is my point of using SVG icons). I'm surpised that they remove it by default for saving bits.

@firefoxic
Copy link

Merge this PR, please 🥺

@oetiker
Copy link

oetiker commented May 11, 2022

Meanwhile, this setting helps …

module.exports = {
  plugins: [
    {
      name: 'preset-default',
      params: {
        overrides: {
          removeViewBox: false,
        },
      },
    },
  ],
};

@JohnAlbin
Copy link
Contributor Author

I've updated the PR to match the code updates made in prep for 3.x releases.

@JohnAlbin
Copy link
Contributor Author

JohnAlbin commented Oct 23, 2022

@TrySound I've updated the initial PR description comment above to point out what the SVG 1.1 spec says about the viewBox attribute's importance.

I would very much like to see the removeViewBox plugin removed from the default preset. If other maintainers would like to create a new preset that uses that plugin, that's fine with me, but it really shouldn't be a part of the default preset for all the reasons stated above.

@mahnunchik
Copy link

I'm looking forward to have it merged

@XhmikosR
Copy link
Contributor

@TrySound could you also have a look at this PR?

We plan to disable the plugin in SVGOMG, but if you land this it would make more sense :)

@TrySound
Copy link
Member

Breaking change 😄

@XhmikosR
Copy link
Contributor

Or a feature since it fixes important issues 🙂

Maybe a minor release would be OK then?

@firefoxic
Copy link

Breaking change smile

I doubt this change will break anything. Rather, it will do the opposite.

@JohnAlbin
Copy link
Contributor Author

@TrySound This is a fix for a bug that prevents all SVGs from being scalable on all web pages.

So its a bug fix, not a breaking change feature request.

@firefoxic
Copy link

In my gulp-stacksvg I even have to replace width/height with viewBox, mainly because of the default svgo setting.

@Lucienest
Copy link

Breaking change 😄

it fixes things (not breaking it) and makes the svgo fully SVG-compliant

@Lucienest
Copy link

In my gulp-stacksvg I even have to replace width/height with viewBox, mainly because of the default svgo setting.

I wasn't much aware of the viewBox property until the same thing happened,
why would u remove it to save a few bytes?

while most projects are bloated with countless dependencies.

@hashimaziz1
Copy link

Since this project apparently now has a new maintainer, who was presumably the one who closed #1128, is any work being done to finally merge this PR?

@Lucienest

This comment was marked as abuse.

@SethFalco
Copy link
Member

SethFalco commented Nov 13, 2023

Unless I meet any resistance, I'd be happy to merge this in future. Unfortunately… this will be a long-winded response due to the heated discussions surrounding the topic historically, especially as my stance differs from other maintainers.

Why won't I merge it now?

Based on the discussions in other issues, I'd have to treat this as a feature rather than a bug, so it is indeed a breaking change.

I would also like to give other maintainers time to contest this response if they have any qualms with it. This is important because if other maintainers disagree, it'll just be enabled again anyway. I would rather the project show a unified front than make SVGO a playground where maintainers with differing opinions override each other's changes.

With that in mind, I would like to merge this into v4.

Why wasn't it disabled sooner?

I think the problem stems from the mentality that we must reduce bytes at all costs. In my opinion, this isn't what an optimizer should be striving for. In fact, reducing bytes should be 4th or 5th priority.

In my opinion, an optimizer's priorities should be:

  1. Adhering to the spec. Even if clients allow us to do something, we probably shouldn't if it's not compliant with the spec.
  2. Adhering to the law. We must preserve copyright notices, licenses, and attribution by default.
  3. Preserving (not adding) accessibility, like title and description. Reducing bytes isn't worth discriminating against users. This also ties in with adhering to the law.
  4. Preserving (not adding) compatibility, SVG 1.1 has more support than SVG 2, and we know this. That's why we still use the deprecated xlink namespace rather than use the href attribute available in SVG 2.
  5. And finally… we can start reducing bytes where reasonable.

Optimizers aren't meant to "reduce bytes at all costs" because even for an optimizer, there are more important things than optimization.

Why do I agree to disable it?

After a user reported an issue with this plugin in 2013, Kir Belevich (the original author of SVGO) disabled the plugin himself. It was then silently enabled again 3.5 years later. There was a reason it was disabled, but I don't see a public discussion/reason it was enabled again, so that probably shouldn't have happened. ^-^'

The use case to disable this is extremely common. In fact, it's essential when using SVGs on responsive webites, hence most web toolkits disable the plugin by default. This is also where a substantial number of our NPM downloads stem from.

An optimizer should be safe by default, and the consensus is that removeViewBox isn't safe. I've also been bitten by this previously. Even if SVGO supports different modes/presets in future, I'd expect the safest mode to be the default mode.

It's clear that among users and engineers, this plugin should be off. Even large projects like Docusaurus and SVGR disable this by default for users, and I'd like to think Meta and Greg Bergé know what they're doing.

On Stack Overflow, a significant number of questions with the svgo tag are specifically looking for how to configure SVGO in the various ways to use it to disable removeViewBox. It's clear there is a demand for this.

As an open-source project making software for the community, we should value what the community has to say within reasonable measure. In this case, everyone has made overwhelming clear across mediums, directly and indirectly, that SVGO is handling this wrong, so we should step up to address it.

@mryechkin
Copy link

@SethFalco well said, and thank you for taking such a reasonable approach to this issue!

@firefoxic
Copy link

@SethFalco Thanks a lot! ❤️‍🔥

I think the problem stems from the mentality that we must reduce bytes at all costs. In my opinion, this isn't what an optimizer should be striving for. In fact, reducing bytes should be 4th or 5th priority.

Probably off-topic, but I hope this revision of the approach will someday touch the default of converting relative commands of the d attribute into absolute ones to save bytes. Now it breaks the ability to move the whole vector figure after optimization by shifting only the coordinates of the start point.

@lzl124631x
Copy link

Well said. Thanks a lot! @SethFalco

@SethFalco
Copy link
Member

SethFalco commented Nov 13, 2023

@firefoxic

Probably off-topic, but I hope this revision of the approach will someday touch the default of converting relative commands of the d attribute into absolute ones to save bytes. Now it breaks the ability to move the whole vector figure after optimization by shifting only the coordinates of the start point.

Yeah, I'd say it's off-topic, but I'll respond here anyway. If you or anyone else wants to add more, I'd recommend opening a separate issue, and we'll continue the discussion there. (Or shoot me an email!)

That isn't something I favor myself because that impacts maintainability (human-editability) of the file, not the usability (rendering/scaling) of the vector.

Most optimizations in SVGO impact the maintainability significantly, and that applies to most other optimizers too. Whether that's absolute to relative coordinates, converting shapes to paths, inlining styles, omitting spaces between attributes or transforms, the SVG will inevitably become a hassle to maintain.

The main difference compared to other optimizers is that we've encouraged overwriting the SVG by documenting that as the primary usage and making it the default behavior of the CLI. That workflow is flawed though, considering it's annoying to edit after and if the SVG breaks, the original file is lost. 😨 (i.e. #1461 (comment))

I was actually planning to add a warning to the documentation, that the output may be difficult to maintain, so to encourage treating the original SVG as the project file, and the output of SVGO as the final product. That's effectively what users of Docusaurus and SVGR are inadvertently doing, so they don't have that problem.

I don't want to rush into making drastic changes, but in future I do want to get feedback on if we should change the default behavior of the CLI to output to a new file called filename.min.svg instead of overwriting, to promote the separation. 🤔

PS: Instead of changing the start point, you could add a transform (translate) to the path. It'll be optimized away next time you use SVGO anyway.

@Lucienest
Copy link

Well said, I've been tracking this issue for years now

@SethFalco SethFalco merged commit 3d2a624 into svg:main May 24, 2024
9 checks passed
@JohnAlbin
Copy link
Contributor Author

O M G 😭

My fellow web developers, our long svgo configuration nightmare is over.

@MoritzLost
Copy link

I love how this was just silently merged with no comment, no fanfare, no great upheaval. The end of an era heralded not by a bang, but by the click of a button. One man bravely stood firm, refusing to budge even as the entire internet told him that he's wrong. And now his watch is ended.

@firefoxic
Copy link

@SethFalco, just thank you so much! 🙏

@SethFalco
Copy link
Member

It'll still be a bit of time before it's officially released, but it's coming sooner rather than later. I was planning to bring attention to the removeViewBox change, but in the v4.0.0 release notes rather than in this issue/PR.

After my little hiccup with v3.3.0 and v3.3.1 due to the ESM related changes, I'm being more careful with v4.0.0. Hoping to get a release candidate out next week for anyone who may want to test and report issues. ^-^'

Reference: https://github.com/svg/svgo/releases

Meanwhile, in my comment earlier, I made note of what I believe the priorities of an optimizer should be. I've been working a lot on the documentation, and will add a dedicated page to make this more explicit. Welcome to check out svgo.dev and tell us content you think is missing, and I can work on this. 👍🏽

TL;DR: Usability > Optimization

Thank you everyone for being patient, though! I know you've been waiting years for this, and even since my last comment it'd been over 6 months.

@Lucienest
Copy link

O M G 😭

My fellow web developers, our long svgo configuration nightmare is over.

A massive victory ✌️, the endless battle between arrogance and facts is now ended.

@GreLI
Copy link
Member

GreLI commented Jun 9, 2024

Just encountered this topic, and want to answer this quote:

After a #139 in 2013, Kir Belevich (the original author of SVGO) disabled the plugin himself. It was then silently enabled again 3.5 years later. There was a reason it was disabled, but I don't see a public discussion/reason it was enabled again, so that probably shouldn't have happened. ^-^'

AFAIK, removeViewBox directly contradicted the plugin which removed width and height attributes from SVG. (removeSizes?) That was much worse in my opinion, since it breaks image sizes when one uses SVG as an image or background image (unless its container has the same size set). That is why removeViewBox was disabled. You can't have both at the same time. And there were complains about this too. So, I reverted this to removeViewBox enabled instead of this plugin. Moreover, problems people complain about removeViewBox to are the same thing — SVG images somehow lose their sizes.

I found your arguments reasonable. However, I think their should applied to the default preset. Many people using SVGO for many reasons. But I myself, Kir, and others are mostly using SVGO for web development given images (mostly icons) from design department. In this workflow there is no need to preserve copyright, SVG2 is probably ok, 2 fractional digits is enough for the most reasonable uses, and so on. As someone says “every byte matters”. So, there should be eager preset to squeeze images at most. That's why SVGO was created in the first place.

@SethFalco
Copy link
Member

SethFalco commented Jun 10, 2024

Hey hey! Thanks for chiming in!

AFAIK, removeViewBox directly contradicted the plugin which removed width and height attributes from SVG. (removeSizes?)

There is a plugin which is the implied opposite called removeDimensions, but they don't contradict each other. They have different purposes.

Width and height are to set the intended size to display the image/document to a consumer such as an SVG viewer or browser, while viewBox is to set the virtual size within the document itself. Both are functionally distinct and just as important as the other.

It's a similar relationship to viewport size (in-game dimensions) vs. window size (operating system window dimensions) in the realm of game development.

Or in other words, viewBox is for the document's internal coordinate system, while width and height are for indicating how it'd like to be presented in an external/parent coordinate system.

That was much worse in my opinion, since it breaks image sizes when one uses SVG as an image or background image (unless its container has the same size set).

Just my thoughts, but neither should get removed by default. Even in the case that both are equal:

  • if we remove viewBox, then we lose scaling, which may be desirable and probably the very reason it was present in the SVG in the first place.
  • if we remove width and height, we lose the original size, which may be desirable in certain cases like icons.

However, based on the direction modern web development has taken, as well as my experience which matches other users here, scalability is a more common issue. Websites are typically developed mobile first and are designed to be responsive, where having a fixed size for assets isn't a common use case anymore, even for icons. Icon sizes are typically based on the font-size or device width in some cases, rather than the original width and height in the document. Particular for interactable icons, to ensure they have large touch targets on mobile. (A static width and height value can't be responsive.)

I think it's very possible that at the time the decision was made, it made sense to use removeViewBox, but I don't think it works out with how websites are developed today.

That is why removeViewBox was disabled. You can't have both at the same time. And there were complains about this too.

You can have both properties at the same time!

If you are able to point me to where having both lead to a problem, I'd be happy to investigate it. However, if both being defined causes issues, I could only imagine it was an issue in the SVG itself, and not SVGO's optimization of it.

Edit: I misunderstood this, see: #1461 (comment)

But I myself, Kir, and others are mostly using SVGO for web development given images (mostly icons) from design department.

This is also how I use SVGO, and I believe how many of our users too, who have encountered these issues. In fact, where I'm working atm, we had to disable removeViewBox too. ^-^'

Furthermore, we even have to disable removeViewBox for svgo.dev, otherwise it'd screw up the responsiveness of the website. I also had to disable removeTitle on svgo.dev too, so assistive technologies could read and interact with the Docusaurus and PostCSS link when they were focused, which SVGO removed before. If we need to disable these plugins ourselves for a basic use-case, it demonstrates why it's been such a widespread problem.

In this workflow there is no need to preserve copyright

That's fair, in some workflows, copyright information doesn't have to be preserved. But SVGO certainly shouldn't be putting users at risk of breaching licenses by default. For example, many are using SVGs from Creative Commons Search, downloading from Font Awesome, or from Wikipedia.

It's like proposing that websites/companies should delete LICENSE files by default before serving their software because it's bloat. But, it's not bloat, it's the license, and they're required to serve the license and attribute the copyright holders if they use the software/library.

Given we're open-source maintainers that distribute licenses ourselves, I'm sure it's easy to understand that others want their licenses respected too. Removing license information (which there are use cases for) should be a conscious decision, not an accident.

So, there should be eager preset to squeeze images at most.

That's fair, in fact, I can even pick this up as part of the next release. I'll work on it next week.

I was thinking about making presets for two other purposes too:

  • sprite sheets - a few people have raised that they wanted this
  • inlined SVGs - it's such a popular use-case, and we have a few plugins that were made just for this (disabled by default because they break SVGs outside the specific use-case)

That's why SVGO was created in the first place.

I'm not personally familiar with the motivation and mindset that went into creating SVGO when it started. However, I think I can comfortably describe what a modern take on an optimizer is and what those expectations are, most businesses value legal compliance and accessibility a lot more than shaving bytes. Reducing bytes is very important, but not if the alternative is losing users or even litigation.

Not sure about other places, but in the US and UK though, companies can get fined for discrimination if they don't build software/websites with accessibility in mind. (Which I'm in favor of, just to be explicit.)

@GreLI
Copy link
Member

GreLI commented Jun 10, 2024

You can have both properties at the same time!

I meant that if you have both plugins enabled (especially by default) it tottally ruins your SVG image. That's why one of them had to be disabled.

@firefoxic
Copy link

firefoxic commented Jun 10, 2024

That's why one of them had to be disabled.

But that doesn't answer the question of why removeViewBox was left out of the two plugins. It's already been said more than once above, but I'll say it again:

  • The viewBox attribute is a characteristic of the SVG internals and therefore should remain within the SVG file itself. Because it indicates which part of the infinite canvas to render. It's like a frame in Figma (which incidentally becomes the viewBox attribute when exporting to SVG).
  • The width and height attributes are external (to the SVG file) because they are the dimensions of where the SVG file needs to be embedded, whether in markup (<img> or <svg> tags) or in styles (background-size or mask-size).

And arguments like “every byte matters” are strange, because the string viewBox="0 0 80 80" is shorter than width="80" height="80" (I hope nobody will think that we are asking to replace removeViewBox with removeDimensions because of saving these 3 bytes 🤭).

@SethFalco
Copy link
Member

SethFalco commented Jun 10, 2024

But that doesn't answer the question of why removeViewBox was left out of the two plugins

I believe that was addressed, actually.

That was much worse in my opinion, since it breaks image sizes when one uses SVG as an image or background image

#1461 (comment)

My interpretation of this is that in a world where one of the two must be enabled to reduce bytes, there is a trade off:

  1. Sacrifice scalability (removeViewBox)
  2. Sacrifice the original dimensions the designer exported the SVG with, so it will always fill the parent element unless overridden otherwise (removeDimensions)

removeViewBox was disabled because it was decided that using the original dimensions was more valuable, or at least more correct, than the scalability. (Though this doesn't adhere to modern design paradigms.)

The reason I have a different perspective is that I think it's sensible to take the fourth option, which is not to enable either of them. Though if I were to enable one, I'd definitely go for removeDimensions personally, since that's overridable in CSS, while removeViewBox can't be worked around after the fact.


In any case, I don't think there's any need to drag this out too much. It sounds to me like Lev mostly wanted to share the motivations behind enabling it, which I did value, and I'm happy to introduce an aggressive preset which includes plugins like removeViewBox and perhaps some of our newer plugins that haven't been promoted to preset-default yet.

@firefoxic
Copy link

the fourth option, which is not to enable either of them.

Now that's sad news. I thought that I would be able to delete the whole config file, which does nothing but replace one plugin with another. But it seems that only one line of this config file can be removed.

@SethFalco
Copy link
Member

SethFalco commented Jun 10, 2024

But it seems that only one line of this config file can be removed.

Ahh, yeah sorry if that wasn't clear! I'm not enabling removeDimensions. If you'd like, you're welcome to make a pitch/RFC in a separate issue, and we'll talk about it. However, that's a visual change imo and should be consciously enabled by end-users rather than us.

My rational is that for people like you and me, we know what's up, and we understand what's happening when the width and height are removed. However, the product/design person or learner who's grabbing SVGO off the shelf doesn't, and this is just breaking the SVG for them.

Even if we argue that they'd be wrong to think it's broken, it doesn't align with the user's expectations, and so has a major impact on usability.

This is especially true for icons where I've seen many export them with a size of width="1em" height="1em", or using 16px literally, and they wouldn't be happy to see it any other way.

But the goal here was never to make SVGO configless for any particular group of people, that wouldn't be possible. If we enabled it, sure you could delete your config, but many others will now need to add a config, there's no winning there. People do in fact have different use-cases, so if anything, I'd say the expectation is that to get the most of SVGO, most people should configure it.

The priority is not necessary to reduce config overrides, it's just to make sure that someone who grabs a tool and uses it off the shelf doesn't get any unwelcome surprises.

Ofc, we'll do our best to come up with a good general purpose config, but there are the keywords, general purpose.

@GreLI
Copy link
Member

GreLI commented Jun 10, 2024

…It sounds to me like Lev mostly wanted to share the motivations behind enabling it…

That's correct. I just wanted to clear what happened then, and what I realized at the time when I was digging it. Nothing else.

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 this pull request may close these issues.