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

What is the benefit of stripping viewBox? #1128

Closed
avwie opened this issue Jul 15, 2019 · 108 comments
Closed

What is the benefit of stripping viewBox? #1128

avwie opened this issue Jul 15, 2019 · 108 comments

Comments

@avwie
Copy link

avwie commented Jul 15, 2019

Some libraries use SVGO for optimizing the SVG and I notice that stripping the viewBox is almost always used. However, I always disable stripping because it breaks proper scaling of the SVG. Unless I totally don't understand, what is the 'optimization' that stripping a viewBox provides?

@GreLI
Copy link
Member

GreLI commented Jul 15, 2019

Just as any stripping—minimize number of bytes, increase informational density.

@GreLI GreLI closed this as completed Jul 15, 2019
@avwie
Copy link
Author

avwie commented Jul 15, 2019

According to your readme:

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.

Removing viewBox definitely affects the rendering result, since scaling is then made almost impossible. The question is, why is it turned on by default? It isn't simple meta-data, it is critical viewport information.

@GreLI
Copy link
Member

GreLI commented Jul 15, 2019

Why? It's removed only when width and height attributes are present and equal to, so no information is lost. Finally, one can turn it off if needed.

@robertknight
Copy link

Removing viewBox still affects the rendering output when width and height are present and viewBox is equal to 0 0 <width> <height>.

I encountered this issue when an upgrade to svgo broke rendering of some scaled inline SVGs on our website (hypothesis/h#5656).

The SVGs in question had an outer <svg> element like this:

<svg xmlns="http://www.w3.org/2000/svg" width="120" height="120" viewBox="0 0 120 120"></svg>

And were being rendered with a CSS class applied that scaled them down to a much smaller size (eg. 16px x 16px).

@GreLI
Copy link
Member

GreLI commented Jul 17, 2019

@robertknight that's incorrect usage, nothing to do with SVGO. Of course if you rewrite width and height, you'll get an error, no matter in which way.

@robertknight
Copy link

Please excuse the stupid question, but can you explain what you mean by incorrect usage? My understanding is that viewBox specifies the user-space coordinates which are then mapped to the viewport which has a default width and height controlled by the corresponding attributes on the <svg>, but can be overridden using CSS to rescale the image.

FWIW the original SVG file was produced some time ago by a designer using Sketch.

@GreLI
Copy link
Member

GreLI commented Jul 17, 2019

SVG is an image which may have properties like width and height. Without viewBox attribute it has value equal to 0 0 width height. So if you change size in any way, you have corresponding result. SVG highly depends on the usage way, so do optimizations. That's is why SVGO is highly configurable.

@flogy
Copy link

flogy commented Mar 10, 2020

I have also encountered an issue with this in IE11 as described by someone else here: https://stackoverflow.com/questions/27970520/svg-with-width-height-doesnt-scale-on-ie9-10-11.

When I add the viewBox property with 0 0 width height value it worked fine.

I know we can enable/disable the related plugins to not strip viewBox ourselves but wouldn't it be worth thinking about not stripping the property by default in order to avoid those issues?

@drewbaker
Copy link

drewbaker commented May 1, 2020

Just ran into this with SVGO defaults. Seems crazy that by default you break SVG scaling using CSS.

// This won't scale using CSS. This is the default.
<svg xmlns="http://www.w3.org/2000/svg" width="21.001" height="13">

// This will scale using CSS, this is better and should be default.
<svg xmlns="http://www.w3.org/2000/svg" width="21.001" height="13" viewBox="0 0 21.001 13">

@jdahdah
Copy link

jdahdah commented May 13, 2020

The viewBox needs to be preserved for inline <svg> elements in HTML to properly scale. Leaving out the viewBox breaks this behavior. I don't know enough about the SVG specification to say if it's supposed to default to viewBox="0 0 width height" or not, but the fact is that this issue is present in the latest versions of Chrome, Firefox and Safari. SVGs need to be inlined for a number of common use-cases, usually for changing colors on hover states or different themes like dark mode.

The fact that SVGO is so configurable is really awesome, but many apps like Image Shrinker use the default settings and don't offer options, ultimately breaking SVGs for anyone wanting to scale them inline.

Please see this CodePen: https://codepen.io/dahdah/pen/YzyjRaw
This is a very simple SVG run through Image Shrinker. I've restored the viewBox in one of the copies.

Screenshot:
SVGs with and without viewBox

Please reconsider changing the default value of removeViewBox to false. It's a few bytes more with a significant improvement for web developers on a wide range of tools. And finally, to reiterate on what @Rj-coding said:

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.

The above example shows pretty clearly that this setting does indeed affect the rendering result, and is not in line with what the SVGO readme promises.

@drewbaker
Copy link

@GreLI are you prepared to reconsider this?

@kylirh
Copy link

kylirh commented Jul 17, 2020

Please change this. I spent a couple hours today digging around to figure out what's going on. The current default is super annoying...

@Naxit
Copy link

Naxit commented Sep 24, 2020

Same for me, the most used case is scalable svgs that doesn't broke with css.
I'm here because I have to modify this weird default behavior and I needed to know how to do it, I see I'm not the only one in this case^^
@GreLI Please reconsider this <3

@mahnunchik
Copy link

The same issue...

@mahnunchik
Copy link

@GreLI Why this and #505 issues are closed?

@Naxit
Copy link

Naxit commented Oct 1, 2020

I used the --icon prop that set '1em' to height and width and keep the viewbox, if this can help someone...
It did the job for me but it's a weird workaround^^

My package.json command line look like this :
build:svg": "svgr --icon --ignore-existing (...more stuff...)",

@woodreamz
Copy link

I also agree with you, viewBox is very important for the scaling specially for web developper. In previous versions of svgo, removeViewBox was false by default. You should reconsider setting this option to false by default.

@JohnAlbin
Copy link
Contributor

Given this discussion, I created a PR to remove this plugin from the list of default plugins. #1461

In the meantime, this is the svgo.config.js I am using on my website:

const { extendDefaultPlugins } = require('svgo');

module.exports = {
  multipass: true,
  plugins: extendDefaultPlugins([
    // viewBox is required to resize SVGs with CSS.
    // @see https://github.com/svg/svgo/issues/1128
    {
      name: 'removeViewBox',
      active: false,
    },
  ]),
};

@lzl124631x
Copy link

@GreLI viewBox is such an important field needed for scaling SVG images. I'm surprised that we remove it by default just for the sake of "minimizing number of bytes". Let's get #1461 in.

@TrySound TrySound reopened this Sep 24, 2021
@JohnAlbin
Copy link
Contributor

I've updated PR #1461 to fix a merge conflict.

Now that extendDefaultPlugins has been deprecated, this is the svgo.config.js I am using on my website:

module.exports = {
  multipass: true,
  plugins: [
    {
      name: 'preset-default',
      params: {
        overrides: {
          // viewBox is required to resize SVGs with CSS.
          // @see https://github.com/svg/svgo/issues/1128
          removeViewBox: false,
        },
      },
    },
  ],
};

@GreLI
Copy link
Member

GreLI commented Nov 25, 2021

I understand the issue, sometimes 'viewBox' is really valuable, but it's not a good idea to dictate disabling default optimization by minority of users. Most users are ok with it and they just don't write here, it's totally ok for them. Probably it's a good idea to disable the plugin in some super-safe preset.

@MoritzLost
Copy link

@GreLI You haven't answered my question though. I have a logo that I want to place on a page as an SVG. Here's a minimal example: https://codepen.io/MoritzLost/pen/wvxvbdV

I've optimized the SVG with SVGO. The first test case is the default SVGO output, for the second one I've added the viewBox attribute again. Observe what happens if you make the window smaller. Without the viewBox, the logo is cropped. With the viewBox, it scales nicely with the window size.

Have you ever put a logo in the form of an SVG on a web page? If so, was your client fine with the behaviour in the first example?

This is a super simple usage, not a special niche use-case or edge-case. Please example to me what I'm missing here.

  • Are you saying that putting a logo on a page is a special case for SVGO and should require a custom config to work properly?
  • If not, how would you solve the issue regarding the cropped logo with the default output generated by SVGO?
  • Or are you saying that the cropping is the correct behaviour?

Are you really sure that SVGs, Scalable Vector Graphics, shouldn't be allowed to scale?

@GreLI
Copy link
Member

GreLI commented Dec 21, 2022

@MoritzLost ignoring my answers and spamming “You haven't answered” wouldn't get you anywhere. Don't be lazy to think it through. Please, follow GitHub Community Code of Conduct.

@Lucienest
Copy link

@MoritzLost ignoring my answers and spamming “You haven't answered” wouldn't get you anywhere. Don't be lazy to think it through. Please, follow GitHub Community Code of Conduct.

You aren't answering our queries properly and saying others to follow the Code of Conduct?
I don't see their comments violating CoC either.

@nnmrts
Copy link

nnmrts commented Dec 21, 2022

Whoops, I think this is an upstream issue with the SVG specification. What do you think @GreLI, should we fork https://github.com/w3c/svgwg/ and rename it to NSVG (Not Scalable Vector Graphics) so we finally have a format that matches the goal you apparently have with (n)svgo?

Here is a fork I made, I renamed everything to the newer, more fitting name:
https://github.com/nnmrts/nsvgwg

Also feel free to take a look at the latest commit I pushed, changing the behavior of the viewBox attribute so it is in line with (n)svgo's philosophy and your totally acceptable way of acting like this attribute doesn't affect rendering and if it affects rendering it's the user's fault because they are using it wrong or in your setup you just don't have access to neither the affected svg files, nor the svgo config used, so you would have to fork or monkeypatch parts of your dependencies to fix "broken" svg files you naively surrounded with CSS styles to make it responsive, thinking the maintainer of a repo with 18k+ stars probably made reasonable decisions to mitigate issues with optimizing and displaying a 20+ year old format in conjunction with even older markup and stylesheet formats - oh my sweet summer child:
nnmrts/nsvgwg@f862750

@wavebeem
Copy link

this is a 3.5 year old thread, everyone. i think your effort would be better spent making a fork of svgo and trying to popularize it than posting in here.

posting snarky comments or trying to explain the situation 10 times isn't helping.

@edarioq
Copy link

edarioq commented Dec 29, 2022

For the sane developer, have your SVGO config ready, create a file that looks like:

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

To test, In the CLI run:

svgo --config svgo.config.js *.svg

Customize this file to suite your needs.

@julpat
Copy link

julpat commented Dec 31, 2022

@julpat every byte matters. It's right in the readme under “Configuration” section.

I thought to point out in the readme exactly this option removeViewBox which is ruining SVG.

@avwie
Copy link
Author

avwie commented Dec 31, 2022

@julpat every byte matters. It's right in the readme under “Configuration” section.

Maybe you should read your own readme file then:

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.

This has been my first issue filed ever on GitHub and boy.... does it keep giving.

@Lucienest
Copy link

@julpat every byte matters. It's right in the readme under “Configuration” section.

Maybe you should read your own readme file then:

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.

This has been my first issue filed ever on GitHub and boy.... does it keep giving.

The README is hypocritical, I've pointed this out before but the owner would simply ignore those.
I'm so fed up with these, it's not even a breaking change.

@MoritzLost
Copy link

@MoritzLost ignoring my answers and spamming “You haven't answered” wouldn't get you anywhere. Don't be lazy to think it through. Please, follow GitHub Community Code of Conduct.

@GreLI You haven't answered, though. I have asked some questions regarding your stance on this issue and some problems I'm seeing with it. You haven't answered a single one of those, but just repeated the same non sequitur about 'changing the rendering' you have used multiple times. I'm not too lazy to think it through. I have written multiple paragraphs to try to understand your reasoning, explain why I disagree and kindly where my reasoning is wrong. You're the one who is not willing to engage at all, instead resorting to short, intellectually dishonest replies. Pointing to the code of conduct because someone disagrees with you is laughable. Why is it so difficult for you to accept that you're wrong?

@Lucienest
Copy link

@MoritzLost ignoring my answers and spamming “You haven't answered” wouldn't get you anywhere. Don't be lazy to think it through. Please, follow GitHub Community Code of Conduct.

@GreLI You haven't answered, though. I have asked some questions regarding your stance on this issue and some problems I'm seeing with it. You haven't answered a single one of those, but just repeated the same non sequitur about 'changing the rendering' you have used multiple times. I'm not too lazy to think it through. I have written multiple paragraphs to try to understand your reasoning, explain why I disagree and kindly where my reasoning is wrong. You're the one who is not willing to engage at all, instead resorting to short, intellectually dishonest replies. Pointing to the code of conduct because someone disagrees with you is laughable. Why is it so difficult for you to accept that you're wrong?

he's doubling down instead of acknowledging it.

@Darkproduct
Copy link

Darkproduct commented Jan 8, 2023

I have nothing more to add but this was a much needed break after fiddling the first time with SVGs in a web-context coming from Latex. I tried every setting of width and height in CSS and in the <svg> tag, I read webpage after webpage of tutorials on how to get this thing, which has SCALABLE in its name to scale. Then I found this. Manually added a viewBox Attribute to all my SVG files with 0 0 [width] [height] and now everything is working fine. I used the last 2 hours to relax and to read this issue, because at this point this isn't an issue this is comedy.

@Crotalus
Copy link

Crotalus commented Jan 17, 2023

Wasted a couple of minutes on this problem today wondering why the SVGs were scaled incorrectly.

(on the other hand, it led me into this Battle Thread filled with Don Quixotes🍿)

KaylaBrady added a commit to mbta/skate that referenced this issue Jan 23, 2023
…ssue

the height and width on the raw SVG file were both set to 24 (matching the viewbox definition). Surprisngly, the image was being cut off. I found that removing the height/width properties resolved the issue. Looking further into it, the viewport was being removed by svgo - see [this](svg/svgo#1128) extensively discussed issue. Since we are re-scaling the svg with CSS to 20x20, we do need to preserve the image viewbox. Applying the solution from [this](svg/svgo#1128 (comment)) comment
@lzl124631x
Copy link

lzl124631x commented Jan 25, 2023

Ok. I commented in this thread back on Sep 10, 2021. And I have been 🍿 watching we "minority of users" complaining in this thread. It has been fun until this freaking issue kicks my ass again today. I was using SVGR to render SVG as React component and I found that the SVG file loses viewport attribute thus can't resize properly.

voice in my head -> SVGO: "SURPRISE M...F...r!"

And I have to add a stupid svgo.config.js with configs in #1128 (comment) thanks to @GreLI's ignorance and arrogance perseverance.

SVGO

@Lucienest
Copy link

Ok. I commented in this thread back on Sep 10, 2021. And I have been 🍿 watching we "minority of users" complaining in this thread. It has been fun until this freaking issue kicks my ass again today. I was using SVGR to render SVG as React component and I found that the SVG file loses viewport attribute thus can't resize properly.

voice in my head -> SVGO: "SURPRISE M...F...r!"

And I have to add a stupid svgo.config.js with configs in #1128 (comment) thanks to @GreLI's ignorance and arrogance perseverance.

SVGO

Thanks, for ignorance and arrogance

@ADTC
Copy link
Contributor

ADTC commented Jan 26, 2023

This looks like a community-maintained repository. Shouldn't this issue be put to vote?

On one hand, disabling removeViewBox seems to be a more sensible default. On the other, doing so would be a breaking change which affects all projects that didn't override it in config, if they would upgrade to a new release with the change.

We could require the plugin to be always specified in config with true or false, forcing all upgradees to decide which behavior they prefer instead of being surprised by a change in the default behavior. (I realize this isn't ideal for CLI usage.)

KaylaBrady added a commit to mbta/skate that referenced this issue Jan 26, 2023
* feat(Nav): New search map icon + page title

* fix(icon-search-map): Remove fill, height + width

* fix(IconSearchMap): Restore height/width and fix underlying viewbox issue

the height and width on the raw SVG file were both set to 24 (matching the viewbox definition). Surprisngly, the image was being cut off. I found that removing the height/width properties resolved the issue. Looking further into it, the viewport was being removed by svgo - see [this](svg/svgo#1128) extensively discussed issue. Since we are re-scaling the svg with CSS to 20x20, we do need to preserve the image viewbox. Applying the solution from [this](svg/svgo#1128 (comment)) comment

* fix(GarageIcon): correct size

Going through all other icons to see which had a  viewbox that matched the width + height  and found only two: icon-garage and icon-up-right-arrow. icon-up-right-arrow had sizing explicitly set in CSS and was not affected. icon-garage defaulted to the leaflet default marker size of 12x12. This explicitly sets the iconSize to 21x25, matching what is specified in the SVG. I noticed that the width and height SVG properties are being removed as part of [svg-inline-loader](https://github.com/webpack-contrib/svg-inline-loader/tree/4bb5529fbdd847d7809067fe11840b48ee13dde4#removesvgtagattrs-boolean). I thought about turning off that behavior as part of this PR, but it seems like it could affect more icons and I'm not really sure if it is necessary

* test(SearchMapIcon): Add rendering test

* fix(SearchMapIcon): Adjust sizing/viewbox so not cut off
@nnmrts
Copy link

nnmrts commented Jan 26, 2023

@ADTC It's not "community-maintained" in a sense that we could just "put" this issue to vote.

The most "active" maintainer nowadays is @TrySound, who seemingly abandoned this repo a couple of months ago. Seems like @GreLI also has rights to merge a PR, but we know their position...

We can't find out exactly who has admin access to this repo but there are two more @svg organization members (@rpominov and @deepsweet), both seem to not care about this repo anymore as well, otherwise a crazy person like @GreLI wouldn't have admin access to it anymore.

I'm just waiting for the moment @GreLI snaps and unpublishes the package or publishes it with some russian propaganda (something similar happened with another package in the past), so more people will notice and work on a fork.

So yeah, the only realistic option here is a fork, I'd love to work on one but don't even have time to set it up.

@deepsweet
Copy link
Member

I'm the original author as it says in the package.json. I have all the possible access both here and on NPM, but not the time and necessary context to continue working on the project. For many years.

At the same time I can't just blindly merge PRs to the project with 15M NPM downloads per week, so I've tried to look for maintainers over the years. It's way more complicated than it seems to be, starting from the observation that there are no many candidates. In fact, there are none. One thing is to merge old and stalled for months PRs, and another is to develop it all further and push forward.

The only realistic way to dramatically change this situation is to contribute and be ready to become a new maintainer.

Talk to me.

@ADTC
Copy link
Contributor

ADTC commented Jan 27, 2023

@deepsweet thank you so much for coming back to chime in! I guess the position of active maintainer is still open and unlikely to be filled any time soon.

In the meanwhile though, do you mind helping us update the Readme? I'd like to start with my own PR #1731 which organizes the content to be more readable without adding anything new.

There's a discussion in the same PR about how to inform users of quirks like this, which I think would be best resolved by adding a FAQ or Troubleshooting section in the Readme. I can add it to the same PR or open a new one.

I think changes to the Readme are much safer to merge than changes to the code. The Readme will also reflect in the NPM repository site so it is worth updating with new and important information.

@svg svg locked as too heated and limited conversation to collaborators Jan 30, 2023
@svg svg deleted a comment from nnmrts Jan 30, 2023
@svg svg deleted a comment from GreLI Jan 30, 2023
@svg svg deleted a comment from Lucienest Jan 30, 2023
SethFalco pushed a commit to JohnAlbin/svgo that referenced this issue Dec 9, 2023
SethFalco pushed a commit to JohnAlbin/svgo that referenced this issue May 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests