[v4] Icon extensions viewBox inconsistent

[rasteiner]

I guess this is more an enhancement than a bug, but I think it's a missed opportunity when switching to a free icon set.

Description

With the new icon set of alpha.7, the viewBox of the included icons has changed from the 16x16px format of Nucleo to the 24x24 of remix icon.
The "icons" extension of plugins however is still putting them into a 16x16 viewBox, cutting a great portion of the remix icons off.

Expected behavior
Either change the viewBox of plugin icons to 24x24, so that we can use the remix icons, too (without having to manually resize them in something like Illustrator), or offer an option to specify the viewBox when declaring icons.

Screenshots

To reproduce

1. Download an svg from remixicon.com
2. Copy its <path/>
3. Put it into an icon plugin:

   panel.plugin('rasteiner/writer-tables', {
     icons: {
       'split-cell': `<path d="M20 3c.5523 0 1 .44772 1 1v16c0 .5523-.4477 1-1 1H4c-.55228 0-1-.4477-1-1V4c0-.55228.44772-1 1-1h16Zm-9 2H5v14h6v-4h2v4h6V5h-6v4h-2V5Zm4 4 3 3-3 3v-2H9v2l-3-3 3-3v2h6V9Z"/>`,
     },

4. Use it

Your setup

Kirby Version
V4 - alpha 7

Additional context

What about a new interface so to avoid a breaking change?

{
  icons: {
    'split-cell': {
       viewBox: '0 0 24 24',
       content: `<path d="M20 3c.5523 0 1 .44772 1 1v16c0 .5523-.4477 1-1 1H4c-.55228 0-1-.4477-1-1V4c0-.55228.44772-1 1-1h16Zm-9 2H5v14h6v-4h2v4h6V5h-6v4h-2V5Zm4 4 3 3-3 3v-2H9v2l-3-3 3-3v2h6V9Z"/>`,
    }
  },
}

[lukasbestle]

We could also try to auto-detect the viewBox.

If I put the example SVG from above into the DOM and call getBBox() on the <svg> element, I get:

SVGRect {x: 3, y: 3, width: 18, height: 18}

So if we assume that the "intended" viewBox has an x and y of 0, we can calculate the viewBox as:

0 0 width+x*2 height+y*2

// so in the example above:
0 0 18+3*2 18+3*2
0 0 24 24

Maybe a bit of a hack, but for icons that are centered in a viewBox with 0 0 as start coordinates, it will work fine. For all other cases where auto-detection doesn't work, we could still introduce a new prop in a later release if needed. But if everyone either uses 0 0 16 16 icons that were the only supported ones before or 0 0 24 24 Remix Icons, we would get away without the prop.

I've tried it with a few other Remix Icons as well, including very tall or very wide ones (like git-commit and netflix). It worked for all of them if the resulting numbers are rounded to integers.

[rasteiner]

Interesting approach, imo a bit too smart.
"Centered" doesn't always mean that the bounding box is centered, think "optical corrections". For example the bounding box of the "music" icon of remix icon isn't centered.

If we don't want a new attribute, but consider parsing the string, what about letting us pass the string of a complete SVG? Kirby would then just need to replace the SVG tag with a Symbol tag, or let us pass a Symbol and Kirby "just" sets the id

[lukasbestle]

You are right, I had feared weird edge cases. Good catch! :)

I like the idea to support full <svg>/<symbol> elements. Then we could do this:

• If string starts with <symbol, import into DOM as is and attach the ID
• If string starts with <svg, create DOM element, copy child elements and attributes into a <symbol> and attach the ID
• Otherwise behave like before for backwards-compatibility.

Using DOM methods would allow us to avoid any custom parsing.

[distantnative]

But wouldn't that leave the default at 16px?

I think with switching to remix icons, we must make the default (the option with the least markup) correspond to those / 24px.

[distantnative]

Or we make the viewing 24px and scale icons up with the math @lukasbestle suggested above. I think old non-24px icons that get a little wrongly centered would be the least issue.

[lukasbestle]

We could also use the math to sort the icons into two buckets. Either it's roughly 16 px and gets the 0 0 16 16 viewBox or it's roughly 24 pixels and gets the 0 0 24 24 viewBox. Sizes in between or outside of these values would not be supported, but there is currently no use case for that if we only have legacy icons and new Remix icons.

In practice we could use a calculation like this:

const bbox = svg.getBBox();
const width = bbox.width + bbox.x * 2;
const height = bbox.height + bbox.y * 2;
const average = (width + height) / 2;
const distanceTo16 = Math.abs(average - 16);
const distanceTo24 = Math.abs(average - 24);

const viewBox = distanceTo16 < distanceTo24 ? "0 0 16 16" : "0 0 24 24";

[rasteiner]

I still think that having it explicit would cause the least amount of problems or surprises, with really no real downside other than a few bytes of easily compressed overhead. As well as just permitting a designer to use their icons in random 192x136 format, maybe the same assets they also use on the frontend, without much thought.

Being able to just give it the contents of an SVG would also make automation way easier. Like I could imagine an easy build step passing a bunch of SVG files through svgo and then into a JSON string which the plugin can import. A pipeline which is made harder when you also need to parse the contents (in node, so without native Dom functions) to remove <svg ...> and </svg>.

[lukasbestle]

We could switch to full svg or symbol tags and deprecate the old syntax. So 16 px would still be the default for backwards-compatibility, but deprecated.

[distantnative]

This can be an additional supported way but we should not add so much overhead for the most common use case especially now that people will much more use the one single remix library: and that is adding another 24px viewbox icon from remix.

So I'm up for having all the suggested different ways, explicit viewbox option, supporting passing svg element... but the default one should clearly be the short definition as it has been with 24px viewbox.

[lukasbestle]

TBH I'm not even sure the short definition is the best way. If you download or copy a SVG from https://remixicon.com, you get a full SVG:

For the short definition you would have to specifically remove the wrapping <svg> element. As Roman wrote, this is not easily possible if you store the SVG in a separate file and inline it during the build process (like I do in the Roomle plugin – I had to manually remove the wrapping tag in the imported file). And even manually, I don't really see an advantage of the short definition. It only creates problems with the implicit viewBox. What happens if we ever switch to yet another icon set that uses a different viewBox again?

[distantnative]

All arguments that make sense to also allow <svg> et.c markups additionally, but not to take away the shorter version that developers are used to by now.

[lukasbestle]

But if we keep the shorter version in the long run, we need to switch to 24 px, which is a breaking change. Is there a way to avoid the breaking change?

[distantnative]

Yes, but as I said I think it's a totally acceptable breaking change to have 16px icons rendered in 24px a bit off-centered. They'll still be visible. And if we'd add the math you mentioned above, we could fix most of this even more. I don't think the costs of trying to avoid this outweighs the benefit of trying not to have any breaking change.

[lukasbestle]

If you have an old 16 px icon and place it in a 24 px viewbox, the right and bottom thirds will be empty. It's not just a bit off-centered. So we would need the detection logic, which would introduce a lot of magic. If we don't deprecate 16 px icons, we would also have to carry this solution with us for a long time.

Maybe as a compromise, what about this:

v4

• We use the "two buckets" detection logic from [v4] Icon extensions viewBox inconsistent #5492 (comment) to support both 16 px and 24 px icons with the short definition.
• We deprecate 16 px icons with the short definition.
• We introduce support for full <svg> or <symbol> icons based on [v4] Icon extensions viewBox inconsistent #5492 (comment) for those who want to make the icon size explicit.

v5

• We remove the detection logic (making every short definition icon use a 24 px viewBox and dropping support for 16 px short definition icons).

[rasteiner]

Yeah, so... As bnomei found out on discord, you can actually put <svg viewBox... into the icons extension 😱

Obviously this results in a:

<svg>
  <symbol viewBox="0 0 16 16">
    <svg viewBox="0 0 48 48">
      ...
    </svg>
  </symbol>
</svg>

I just imagined that this would be invalid SVG, but it turns out it's actually not. Symbols can contain "structural elements", and <svg> is one of those.

I don't know where this leaves this issue.

[distantnative]

@rasteiner and in this case the 48px view box works even if nested inside a 16/24px view box?

[distantnative]

I've created a PR as suggestion based on how I understood our discussion: #5510

[rasteiner]

@rasteiner and in this case the 48px view box works even if nested inside a 16/24px view box?

I haven't tried it and don't have a computer handy, but bnomei says so.
Probably since the embedded SVG doesn't have an explicit width and height, but only a viewBox expressing an aspect ratio, it's automatically resized to fill the symbol's available space. Similarly to how an SVG without dimensions loaded directly in the browser fills the screen. But this depends on the SVG not declaring a natural size: an SVG with width and height attributes would probably be clipped by the symbol.