Add Width and Height to UmbracoMediaVectorGraphics (SVG)

[enkelmedia]

We just had the need to provide a aspect ratio for a UmbracoMediaVectorGraphics (SVG). I solved this by adding umbracoWidth and umbracoHeight to the built in Media Type and added a MediaSavingNotificationHandler that parses the XML of the SVG to get the width and height from either width/height attributes or the viewbox.

That got me thinking that maybe this could be a nice addition to the core?

Would be happy to contribute the code in a PR if this is something that we want.

[skttl]

Yes, please!

[AndyButland]

Please feel free to bring a PR @enkelmedia, we'd be happy to consider it. The only thing I wonder is that it would need to be clearly applicable for all projects and all SVGs to make sense in core. For example I understand an SVG can use units like em or %, and not just pixels. Maybe that's OK, but those sorts of edge cases would need to make sense both in terms of the extraction logic and in interpreting the umbracoWidth and umbracoHeight parameters.

[enkelmedia]

@AndyButland nice that you think that this might make sense!

You are right in that em and % are also valid for the width/height attributes. After looking closer at this it seems like maybe the viewBox attribute is a better source for width and height.

My understanding is that in a SVG like this:

<svg width="800px" height="800px" viewBox="0 0 36 36" ..../>

The viewBox is basically the "canvas" for the icon, so the last two digits is the width and height, not necessarily in pixels because the viewBox defines the user space units.

So at the end of the day there is really no bullet proof way to get "pixels" from a SVG, but I think for the vast majority of cases something like this would be sufficient:

• Define a pixel-representation for 100% (e.g. 800px), so if width=100% and height=50%, we'd set the pixel value to 800x400.
• Define a "translation" for em. Eg. 1em = 10px (or something).
• Fallback to width/height from viewBox if width & height attributes are not present and treat width/height from viewBox as pixels.

Would this make sense?

[nul800sebastiaan]

I failed to see from the original post what your actual need was, why did you need to provide an aspect ratio?

SVGs are by definition infinitely scalable so they have no inherent height/width. I might understand that they might have an aspect ratio, could make sense, but what did you need to use it for?

As for populating width and height, that seems misleading to me, I'd rather think a set of "viewbox" properties might be more appropriate here.

I did 0 research on this, so please enlighten me if I am missing something. Just saw this discussion and now I'm interested in what is going on here 😅

[enkelmedia]

Lovely that you're getting involved @nul800sebastiaan 🚀

Our frontend-dude needed the aspect ratio to be able to ensure that it takes up the right space during rendering to avoid layout-shifts.

SVGs are by definition infinitely scalable

True

so they have no inherent height/width

Not really, they have a "canvas" that define the coordinate system, but not a "width / height" in pixels. A author can (optionally) define a width and height for "optimal rendering" but even if they do, the rendering is still infinitely scalable.

I'd rather think a set of "viewbox" properties might be more appropriate here.

Yes, it might be misleading to populate umbracoWidth and umbracoHeight - also considering that they are defined in px for uploaded images and for a SVG there is no 100% concept of "px" so it could be misleading to reuse the property names.

But, on the flip side, these are well known properties and introducing new properties might also be confusing.

At the end of the day I do not have a strong opinion reg. if it should be new properties or if we should reuse umbracoWidth and umbracoHeight.

[nul800sebastiaan]

Our frontend-dude needed the aspect ratio to be able to ensure that it takes up the right space during rendering to avoid layout-shifts.

Aaah yes, I have now done a little bit of research and can see that this is useful for preventing elements on the page from jumping around. Of course the data is available and can be extracted from the SVG using frontend magic, but that happens to late in the rendering process so stuff still keeps jumping around.

I can also see that in the property value convertor it would be useful to pre-calculate the aspect ratio that the height and width give you so that it can be plugged directly into CSS (aspect-ratio: 1.777).

Anyway, from this, it seems like umbracoWidth and umbracoHeight would be reasonable to use. I don't think we need to worry too much about people already using them as pixel values, they're not called umbracoPixelWidth and umbracoPixelHeight after all!

Scope creep (sorry!), would it be useful to also have all of the other properties of the viewBox available to work with? Not sure.

[enkelmedia]

Yeah, that the idea - so set the aspect-ratio to allow the item to "take up it's space` while the asset it loading. Another approach is of course to inline SVGs into the HTML and process while doing that but in this scenario they are lazy loaded only if needed.

Anyway, from this, it seems like umbracoWidth and umbracoHeight would be reasonable to use. I don't think we need to worry too much about people already using them as pixel values, they're not called umbracoPixelWidth and umbracoPixelHeight after all!

I agree with that, the name does not really communicate any measurement.

I would be happy to play around with this if you think it make sense :)

[nul800sebastiaan]

Yes, makes sense to me!

[enkelmedia]

I was wondering if maybe @AndyButland could help me out with some pointers so that I create the PR in the desired way.

Question 1

I'm not sure if I'm on the right path but my initial idea was to look at IImageDimensionExtractor.

It uses IImageDimensionExtractor to extract dimensions from a Stream. It seems like the system only support one unique implementation of this interface. By default the NoopImageDimensionExtractor and if ImageSharp is present ImageSharpDimensionExtractor i used.

The interface is quite simple

public interface IImageDimensionExtractor
{
    IEnumerable<string> SupportedImageFileTypes { get; }
    public Size? GetDimensions(Stream stream);
}

One approach could be to use this interface, maybe allow multiple implementations (IEnumarable<IImageDimensionExtractor>) and use the first implementation that supports the current file format (Based on SupportedImageFileTypes).

But. I'm not sure if we should think about "SVGs" as "Images", that's maybe the philosophical questions here.

What do you think? Should I approach this in another way?

Question 2

I've been thinking about when/how to add the umbracoHeight and umbracoWidth properties to the Media Type. What's the recommended approach here, should they be added during initial setup (DatabaseDataCreator?) or via a Migration?

I guess it comes down to should they only be added to fresh installs or should we append them during a upgrade?

[AndyButland]

Without diving into it too much @enkelmedia - I think a new ISvgDimensionExtractor might be best. Not so much for the fact that we need to treat this as different from images, but just so there's less churn in how the single registered instance of IImageDimensionExtractor is used.

And for question 2, seems to me these should be added in a migration as well as for new installs. They would be new properties, so I don't see there's any concern about a breaking change. Implementors could just opt into using them on their upgraded project if they wish.

[enkelmedia]

Thank you for the pointers @AndyButland!

I've worked on this for a while now and I have:

• Successfully implemented a migration that adds the umbracoWidth and umbracoHeight properties
• Added ISvgDimensionExtractor and it's implementation SvgDimensionExtractor

I have have a couple of more questions to ensure that I'm doing the right thing.

As far as I understand, the migration will run both for a new install and a existing install. I guess that this would be sufficient to ensure that them to "be added in a migration as well as for new installs"?

When looking at how the IImageDimensionExtractor is used, it's called from UploadAutoFillProperties, which is used by FileUploadMediaSavingNotificationHandler and ImageCropperPropertyEditor.

Now, I'm wondering which road to take here, should I:

• Introduce a new MediaSavingNotificationHandler for this purpose to keep everything separate.
• Add the new ISvgDimensionExtractor to UploadAutoFillProperties this would be the easier approach but it would be a breaking change as the class and ctor is public.

[enkelmedia]

I went ahead and implemented a separate Notification Handler in my first draft PR, of course happy to change based on feedback.

Here's the PR

#22244

[AndyButland]

This has been completed now via #22244, thanks @enkelmedia for the suggestion and the implementation. Will be part of 17.4.