search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Add support for SVG images. #5541

Closed yorkshire-pudding closed 5 months ago

yorkshire-pudding commented 2 years ago

Description of the need

At present, if you have uploaded SVG files to use on site, these show as though the file can't be found: image

As a content author I want to be able to preview SVG images when I go to add an image to a content item through the "Select from Image Library dialog" So that I can check I'm inserting the right image

As a content author I want to see light coloured SVG images against a dark background and vice versa So that I can easily see what the SVG will look like

Proposed solution

When viewing the Select from Image Library dialog, SVG files are rendered against a light or dark background depending on the dominant colour, or user could have an option to switch background colour.

Alternatives that have been considered

If you go to manage files, you can click on view and see them displayed as an entity.

Is there a Drupal or Backdrop contributed module that accomplishes this? Not aware of any. The SVG Image Support module adds support for fields but doesn't have any impact here.

Additional information

Add any other information or screenshots that would help.

Draft of feature description for Press Release (1 paragraph at most)

Backdrop now has improved support for SVG files. SVG files (Structured Vector Graphics) have the advantage of being able to scale to any size without distortion or needing a larger file size. SVGs have been added to the list of default allowed extensions for new sites and you can preview your SVG both in the field where it is added and the Image Library where you can select previously uploaded images.

indigoxela commented 2 years ago

The main problem with SVG: it's not an image format, but SVG's are documents. Possibly they even contain scripts - which is a security problem.

The second problem: the GD library can't handle SVG. GD is the default image handling library in Backdrop (and other php based CMS).

indigoxela commented 2 years ago

Out of curiosity I played around a bit with svg. All necessary support seems to be in core already. The culprit here is the way, the image library view handles things (hardcodes to thumbnail style directory, but there's no "image"). That one would need a complete rewrite - which is probably not backwards compatible.

My experiments:

  1. Create a custom file type SVG with "image/svg+xml"
  2. Configure the display for that file entity type
  3. Allow the upload for the svg extension
  4. Add such a file
  5. Create a view of files (entities)
  6. Use the "preview" style of the rendered entity

What's still not working: the file display mode needs some extra file template to add the necessary attributes, so I didn't work that out. My conclusion: in theory it's possible with only core stuff. In practice ... it's probably still a contrib candidate.

yorkshire-pudding commented 2 years ago

For the first problem, I'm going to take a look at https://www.drupal.org/project/svg_sanitizer

Thanks for looking. I've been using SVGs for logos without too much difficulty. I spotted this while testing something else on that view and thought it's worth looking at.

I can't see a problem with this being contrib as its probably not something that the majority will try to do. Added label

schoenid commented 2 years ago

Even in the IMCE preview, a SVG preview is not displayed. Seems to be the same problem. There you have also the possibility to create thumbnails. If you try it with a SVG, you will get the message: <filename>.svg is not an image.

Anyway it can be selected and inserted properly and will then be displayed on the websites page.

By the way: Thumbnails for small SVGs makes no sense, because of it's size the file itself should be used as thumbnail !!!

yorkshire-pudding commented 1 year ago

Just adding a thought here that @schoenid inspired. If as part of #5966 we allowed upload of SVGs, perhaps the solution to getting these in the image browser is to create a small thumbnail in a compatible format when the file is uploaded?

docwilmot commented 1 year ago

Quick POC based on https://www.drupal.org/project/svg_image.

yorkshire-pudding commented 1 year ago

Thanks @docwilmot . I gave this a quick test and it works well 😃 - the SVG shows in the image library. On the file view page I do get these warnings:

Warning: Undefined array key "svg_settings" in image_field_formatter_view() (line 830 of /app/docroot/core/modules/image/image.field.inc).

Warning: foreach() argument must be of type array|object, null given in image_field_formatter_view() (line 832 of /app/docroot/core/modules/image/image.field.inc).

These don't affect functionality though, but probably need handling.

docwilmot commented 1 year ago

Warning: Undefined array key "svg_settings" in image_field_formatter_view() (line 830 of /app/docroot/core/modules/image/image.field.inc).

Ooh our sandboxes run PHP 8.2.8! Didnt realise this. Will need to update my local.

docwilmot commented 1 year ago

Quick note that this PR doesnt just add SVG to the Image browser, but adds support to Image fields as well. So you can add svg to the supported extensions in the content type field settings, then you can upload SVG through image fields and also insert existing through fields, and also insert SVG in CKEditor.

Tests all pass except PHP 8, which is unsurprising given the error above, but should be fixable.

docwilmot commented 1 year ago

We discussed this in the Thursday weekly meetings and the concern of security came up (details today's recording). Noting here quickly to followup on that and for any comments.

klonos commented 1 year ago

@docwilmot thank you for working on this 🙏🏼 ❤️

Re the previous comment, rehashing what @yorkshire-pudding said in an earlier comment about having a look at https://www.drupal.org/project/svg_sanitizer. The module seems to be implementing a separate formatter, but I'm thinking that if this is to get in core, then we should take the safest option and sanitize things by default in our existing image formatters (but make it possible for contrib to override that or add an option in the UI for it). ...having said that, watching https://www.youtube.com/watch?v=Nb9W9-Ufq0g makes me think that the default options may be doing a bit too much stripping out of certain tags and attributes, which may result in broken svgs - and having a UI to customize which tags/attributes should be allowed doesn't seem like a thing that should be in core 🤔

I also noticed that the svg is being rendered using an <img> HTML tag and referencing the .svg file "externally" instead of adding the svg file "inline" (as an actual <svg> HTML tag): <img src="https://pr4539-6dcl7rmc9im9zkg3pcuzussnfvayhalt.tugboatqa.com/files/field/image/android.svg" alt="">

...I am not in favor for doing it one way or the other, but if done that way, the ability to manipulate the svg via CSS (or via JavaScript, or be able to animate it) is apparently lost (see the "A Note About SVG Security" section in https://www.codeenigma.com/blog/allowing-svg-images-be-used-media-drupal-9, point 4 in the issue summary in https://www.drupal.org/project/drupal/issues/2433761 etc.). We should discuss that and decide which option is best to go with, or if it should be a formatter setting, which is what the contrib modules are doing (also worth noting that all of them have the "safest" option as the default):

klonos commented 1 year ago

FTR, I should mention that out of the 3 contrib implementations in Drupal-land, I'm in favor of svg_image (which is what the PR is also based on 👍🏼 ), which makes the required changes to allow the existing image field to support svg files.

I don't like the approach that svg_image_field is taking to introduce a separate, dedicated field type specifically for svg files by "locking" the extensions that can be used with it, as I find it too limiting and the addition of yet another field to be complicating things.

I don't like the approach of svg_formatter either, because it "locks" the svg support to the file field type instead of the image field. I understand that technically svg files aren't (raster) images, but the end result or the perception by non-technical people seems to be that these are images or at least "graphics" or media assets - something that resembles or behaves as an image.

yorkshire-pudding commented 1 year ago

Thank you for sharing thoughts.

I've mostly used SVGs as file reference in image tag but I agree that it should be a choice. I haven't explored all the potential of SVGs, but I think if inline then you can use CSS to change individual sub components of the image (e.g. colour) which opens lots of cool possibilities.

makes me think that the default options may be doing a bit too much stripping out of certain tags and attributes, which may result in broken SVGs - and having a UI to customize which tags/attributes should be allowed doesn't seem like a thing that should be in core

I agree. One use case I have thought about quite a bit is the ability of SVGs to have individual components have a link, so you could click on the desk you want to book and it would take you to a booking form and likewise you could colour the regions to indicate availability. There are lots of possibilities so we should avoid locking down too far. I can't think of any genuine use cases for scripts but there might be as long as someone knows what they are doing.

docwilmot commented 1 year ago

Updated with Sanitizer support and optional display as <img tag. Considering whether it makes sense to have a separate upload SVGs permission.

klonos commented 1 year ago

Updated with Sanitizer support and optional display as <img tag.

❤️ 🙏🏼

Considering whether it makes sense to have a separate upload SVGs permission.

I would normally consider this (adding a separate permission for a very specific file type/extension) a bit excessive, however in this case, and given the very real security considerations/risks, I think that security-conscious organizations would definitely appreciate the possibility to restrict this to specific user roles (think only the marketing department for instance). The added security/restrictions would help reassure that we are giving people the means to take all precautions possible.

yorkshire-pudding commented 1 year ago

I agree that a separate permission would make sense here. I'll try and do some further testing.

docwilmot commented 1 year ago

Added and implemented upload svg images permission. Users without that permission can still use and insert SVG images from the library.

I'll need to add tests next (since I cant think of anything else outstanding left to add), but will pause until its clear this is OK as is.

docwilmot commented 1 year ago

Reminder if anyone got time to test or review this please.

indigoxela commented 1 year ago

Reminder if anyone got time to test or review this please.

I planned to chime in earlier, but... :shrug:

@docwilmot great work so far! :+1: Works like a charm for image fields.

However, while testing, I ran into UX WTF's:

  1. Selecting via editor dialog works, but the editor doesn't display the svg in content. After saving, it's there.
  2. Uploading via editor dialog doesn't work, yet (no way to configure extensions for the editor upload)
  3. Uploading via File form on /file/add doesn't work, yet (workaround: edit file.settings.json to add extension)

noupload-dialog

noupload-file

indigoxela commented 1 year ago

Re the SVG not displayed in the editor content: ouch, that's gonna be tricky.

It's the lack of dimensions. If I add dimensions manually (in source mode, width/height), the SVG gets displayed properly.

The problem is, that CKEditor doesn't automatically add dimensions for SVGs like it does for regular images, and the Backdrop dialog doesn't set any value on insert (other issue in the queue).

A-HA, some more findings: whether an SVG get's displayed in the editor depends a lot on the SVG itself. A simple SVG (no layers, dimensions set in px,...) works. So files might or might not be visible in the editor, depending on with which software they got created, it seems.

indigoxela commented 1 year ago

The editor again: drag-and-drop upload of SVG also doesn't work (these extensions are hardcoded in js/plugins/backdropimage/plugin.js in function clipboardIntegration).

Another finding: extraction of svg dimensions (attribute width and height) doesn't seem to work. All get the default dimensions.

docwilmot commented 1 year ago

Excellent, thanks for the review. Will get on to fixing those ASAP

indigoxela commented 1 year ago

I'm trying to think over the extra permission per extension (.svg in this case), and honestly, I'm not convinced it's a great idea.

It complicates things - in code and for admins.

UX first: If admins choose to add the extensions, they also have to think of (first of all, have to know about) also setting the permission accordingly. Admins usually are privileged, so they see the ability to upload svg after setting the extension in the field, but then editors complain, that they can't upload svg and the admin has a perfect puzzle. ("Huh? I see it, what's going on?") Having to configure one feature in two places, to get it working is something I'd like to avoid. And that's only for image fields. There are several ways to upload images

All of these would be required to add permission validation for the 'upload svg images' perm. Do we really want that? Especially, as some are contrib, which means, we put the burden on them without providing a standardized API for the extension list to both, display available extensions (dynamically) in the form and validate accordingly.

Another consideration: we add a complete (and not even small) library to core to sanitize SVGs - and then obviously we don't trust it, to do a proper job? I mean: if it's good enough, why can't we simply rely on it and skip the additional permission per extension? If it's not good enough and its output isn't trustworthy, why do we include it, anyway?

And I really don't want to derail this issue. I think, handling SVG as images, consistently throughout core, consistently for contrib, seems like a nice thing to advertise with 1.27. :wink:

indigoxela commented 1 year ago

Small complement re contrib projects having to implement checks for upload svg images: keep in mind, that they'd also have to check for the core version, because they can't rely on Backdrop being the latest version. Yet another complication.

indigoxela commented 1 year ago

One concern re the current library location in 'core/modules/image/libraries' and the image_autoload_info() function:

That means, that the functionality to sanitize SVG is only available, if the image (field) module is enabled. That's for sure usually the case, but the module's not required, so we should consider, that it might be off.

docwilmot commented 1 year ago

Moving from the PR comment:

@indigoxela said: I do have some questions re the current implementation:

Currently the library location is 'core/modules/image/libraries', and hook_autoload_info() is also in the image module. That means, that the functionality to sanitize SVG is only available, if the image (field) module is enabled. That's for sure usually the case, but the module's not required. Should we consider that?

Besides that I wonder, if hook_autoload_info is actually necessary in this case. And I also wonder, why there's an abstraction layer for the sanitizer class. (Is someone supposed to come up with an alternative sanitizer class?) And then, still all (sub-)classes of this sanitizer would get loaded on every page call (because of hook_autoload_info). Even if it wouldn't ever be needed, for instance if no field allows svg's.

indigoxela commented 1 year ago

Looking at my recent comments ... hm, I did derail the issue a bit, I'm afraid. :disappointed:

What if we separated out some topics to their own issues (we already have a meta issue), and in a first step concentrate on the great enhancement to the image formatter?

What I imagine:

@docwilmot what do you think, would that help? @yorkshire-pudding what's your opinion on splitting topics out? @klonos (and other people involved in this thread) would that be OK for you?

yorkshire-pudding commented 1 year ago

@indigoxela - derailing issues late in the day is normally a @klonos thing to do 😉

I suppose splitting it out reflects that the original issue was about supporting SVGs in the image browser so I'm fine with that.

I personally don't like having images added within text fields using the editor; I prefer my images to be in distinct image fields so the editor is a nice to have.

You make some valid points about UX which may be simpler to be split out but I'm not familiar with the code base to know if that will be simpler or not.

indigoxela commented 1 year ago

I prefer my images to be in distinct image fields so the editor is a nice to have.

Good point, the first step should already include the ability to pick svg from image browser in image fields (already possible, but (without patch) currently causes EntityStorageException). :+1: Uploading's currently (without patch) possible via /file/add, if one adds "svg" to file.settings.json (which is feasible via config manager).

(And, hey, I derailed issues before, I'd consider myself a pro re that. :laughing:)

docwilmot commented 1 year ago

Selecting via editor dialog works, but the editor doesn't display the svg in content. After saving, it's there.

Can you send me a copy of an SVG that doesnt show? All mine work fine.

I'm trying to think over the extra permission per extension (.svg in this case), and honestly, I'm not convinced it's a great idea.

You make some very good points. I'll leave the code in for now, but I'm in agreement that it probably isn't worth it.

That means, that the functionality to sanitize SVG is only available, if the image (field) module is enabled. That's for sure usually the case, but the module's not required, so we should consider, that it might be off.

I think I also might agree with this and had initially recommended putting the library in core/includes, but it was suggested to put it where it is now. But I'm not sure if there is a use case for an SVG sanitizer if image module is off though?

And then, still all (sub-)classes of this sanitizer would get loaded on every page call (because of hook_autoload_info).

My understanding of that hook is it registers the classes and they are only (lazy) loaded when an instance of the class is created. (It uses spl_autoload_register() in the background). Not an expert, though.

And I also wonder, why there's an abstraction layer for the sanitizer class.

There were comments somewhere as to all the things this sanitizer does might not be always desirable. I figured if we allow users to specify a sanitizer class, then they can extend this (or any other sanitizer really if the future brings better ones) and use any of the class methods (it has a reasonable list of options, setAllowedTags() etc). Otherwise there is no reasonable way to influence what this sanitizer does.

Thanks again for all the input.

indigoxela commented 1 year ago

but it was suggested to put it where it is now. But I'm not sure if there is a use case for an SVG sanitizer if image module is off

This is a valid point, but my understanding is, that the File module still allows to upload SVG, even if the Image module's off. So it should be responsible for sanitizing (note that the File module is a required one). Personally, I'd agree, that core/includes doesn't seem to be the right place.

There were comments somewhere as to all the things this sanitizer does might not be always desirable.

I see. I wonder, though if there isn't a simpler approach for that. A setting or a hook might be an easier option.

Re a sample SVG, that won't show up (no dimensions set) initially (in the editor): try the "noodles-bowl.svg" in the sandbox. If you need more SVGs to test, grab some from openclipart.org

Again, sorry, that I joined here late and derailed things a bit. I'd love to have this feature in core.

docwilmot commented 1 year ago

Iwouldnt say its derailed, grateful for the comments. FYI the sanitizer only sanitizes on display, so uploaded SVGs would be in original state until viewed.

I see. I wonder, though if there isn't a simpler approach for that. A setting or a hook might be an easier option.

Not sure what you mean.

indigoxela commented 1 year ago

the sanitizer only sanitizes on display, so uploaded SVGs would be in original state until viewed

Ah, I see, I didn't actually test what gets displayed on /file/FID, when the image module's off. :thinking: If sanitizing is a display thing, it might make sense, that the sanitizer is part of the image module. :+1:

... hook or seeeing ... Not sure what you mean.

Currently, there's a setting Render SVG images in an <img> tag (which IMO should always be the case, anyway :shrug:). How about an additional setting (per field display? global?) to sanitize SVG? Or eventually a new hook inside image_field_formatter_view? Didn't try that out.

Another UX detail, that irritates me a bit: the image field display summary states "SVG image attributes: 200 x 200 pixels", but that's not an actual setting in that display, as long as an image style is selected. And this setting only has effect, if the display is either toggled to "original image" or "upscaling" is turned off for the image style in use. But even if upscaling is off, the image dimensions get overridden (so scaling still happens). Why not using original dimensions if available?

docwilmot commented 1 year ago

Cant fix drag and drop because of this bug reported here, which was only fixed in CKEditor 4.19.0 (but we're still at 4.18.0). https://github.com/ckeditor/ckeditor4/issues/5095

docwilmot commented 1 year ago

How about an additional setting (per field display? global?) to sanitize SVG? Or eventually a new hook inside image_field_formatter_view? Didn't try that out.

I dont see a good reason to make sanitizing a per-field display though. I think it should be globally on or off. Either you know for sure all possible SVGs are safe (youre creating them yourself, and only you uploading them) or youre not sure. And I think sanitize by default, and not make it easy to disable either.

Another UX detail, that irritates me a bit: the image field display summary states "SVG image attributes: 200 x 200 pixels"

Will fix that. The concept of SVG size is a bit slippery, and I think I changed the code halfway and broke the whole idea.

indigoxela commented 1 year ago

Cant fix drag and drop because of this bug reported here, which was only fixed in CKEditor 4.19.0

They... fixed that? :astonished: Wow. Maybe we should open a new issue to update to the last available OpenSource CKE4 (v4.22.1 | 30-06-2023)?

I dont see a good reason to make sanitizing a per-field display though. I think it should be globally on or off.

Whatever you prefer.

And I think sanitize by default, and not make it easy to disable either.

That sounds like a plan. At least, it shouldn't get turned off "by accident".

Many thanks for all your work on this. :+1:

docwilmot commented 1 year ago

I'm having a rethink about the overall approach. This PR is mostly as @indigoxela implies leveraging image fields formatter to add SVG support. I'm wondering if it may be better to focus on adding SVG support to theme_image() instead, which would cover everywhere images (and thus SVGs) are displayed. It would mean putting most of the *_svg functions in image.inc. The doubt about this approach is that SVGs are not really images, and would this lead to complications or confusions. @indigoxela what do you think?

klonos commented 1 year ago

FYI the sanitizer only sanitizes on display, so uploaded SVGs would be in original state until viewed.

That doesn't seem right from a security perspective 🤔 ...I would be less concerned if we sanitized the file that ends up on the server (since we cannot know for sure that it will be displayed using the expected methods). Perhaps sanitize on upload and throw an info message to let people know that sanitization has happened for security reasons (link to documentation that explains the possible risks), and that certain potentially unsafe attributes might have been stripped from the file?

The doubt about this approach is that SVGs are not really images...

My take on this is the following: less technical people consider SVGs to be images - only developers will understand that they are technically XML files and expect them to be treated as such in code. So we can keep in-code whatever seems sensible code-wise (treat SVG files as XML/code/unsafe/whatever), but from a UX perspective we should not be exposing these technicalities in the UI (not too much at least - only the necessary up to a point when things do not become confusing). For example, yes we can add the code where it makes more sense for developers and make the choices that simplify the technical implementation, however we should be treating SVGs as images in the UI (where possible - if might not be feasible for image styles for example).

indigoxela commented 1 year ago

I'm wondering if it may be better to focus on adding SVG support to theme_image()

@docwilmot Interesting point... However, isn't that the display mode "original image", which already works fine for SVG, anyway? To my understanding it's theme_image_style, that currently fails to properly display SVGs - the part that should get fixed, as it prevents SVG (as image) to properly show up in the library, or in fields with an image style set for display.

The doubt about this approach is that SVGs are not really images

I think, if we're using them in image fields, we should treat them as images (using img tags). One problem is, though, that all files that are SVG (extension) will show up in the image browser. No matter, if they're supposed to be documents or images. (Correct me if I'm wrong.)

indigoxela commented 1 year ago

I would be less concerned if we sanitized the file that ends up on the server...

A bit tricky, though. As we might possibly mangle SVG's that aren't supposed to be used as image. Or am I missing something?

There are several examples of letting people input potentially unsafe stuff, but sanitize on display in Backdrop. For example node titles, or filtered text fields.

@klonos which example would fit best as a model for SVG handling, what do you think?

Personally, I don't have strong opinions re sanitize on upload vs. on display.

klonos commented 1 year ago

...we might possibly mangle SVG's that aren't supposed to be used as image.

Good point 👍🏼 ...then I would like us to have the default be to sanitize on upload, with an option to not do so (explaining why people might wanna disable it, and a warning of the risks that this option has).

There are several examples of letting people input potentially unsafe stuff, but sanitize on display in Backdrop. For example node titles, or filtered text fields.

Also good point 👍🏼 ...but I had CKEditor-enabled fields on my mind. My understanding is that by default things are being filtered out on save (for the "Filtered HTML" text format provided OOTB, which has the "Limit allowed HTML tags" filter on by default). It seems to me that with the SVG sanitizer we should be doing something very similar, trying to keep people safe.

So in a similar fashion, perhaps we should be allowing .svg extensions for the default "Image" file type that is provided OOTB, but have those be sanitized. So if SVGs are not supposed to be used as an image on a specific site, then you'd need to specify a separate file type, where you can decide whether to sanitize those files on upload or not.

@klonos which example would fit best as a model for SVG handling, what do you think?

Are you referring to the question @docwilmot asked with regards to leveraging image fields formatter to add SVG support vs. adding SVG support to theme_image() instead and putting most of the *_svg functions in image.inc? ...if so, then I think the latter makes sense to me 🤔 ...I would have to see a list of pros and cons outlined in order to make a better-informed decision though.

indigoxela commented 1 year ago

...but I had CKEditor-enabled fields on my mind. My understanding is that by default things are being filtered out on save

Not with SVG. Both, CKEditor and TinyMCE use img tags when svg's get inserted/dropped (as long as "svg" is allowed per config). Which means, the xml contents aren't touched at all, but the browser won't do anything with active content, like for example "onclick".

To have something to play with, here's a nasty svg:

<svg width="200" height="200" version="1.1" viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg"><path onclick="alert('You clicked')" d="m20 20h160v160h-160z" fill="#72c5c5"/></svg>

It's a blue square. If used like that:

<img alt="" data-file-id="6" height="119" src="/files/maximize.svg" width="119" />some content<img alt="" data-file-id="7" height="200" src="/files/blue-square.svg" width="200" />

... the "onclick" is ignored.

If using svg directly, the Filter module will do its magic ... to completely break the tag. :stuck_out_tongue_winking_eye:

If the filter is off (raw HTML with no filtering at all) in that text field, and SVG is used as tag, the onclick event fires. :point_left: And that's what we try to prevent.

docwilmot commented 12 months ago

OK, again, as per @indigoxela post I've left things as is, so now all SVGs will display as <img> tags wherever theyre in use, except in fields where the view mode is specifically set to not display as <img>. This should mitigate security concerns except where the user chooses, and even then we sanitize those SVGs, so less risk.

Further, I've changed the formatter settings form to allow setting max SVG dimensions only where the SVG is <img> and a image style is not set. If not, then the SVG displays at its native dimensions, or at 100% if it doesnt have dimensions. And the formatter summary now reflects that.

Side note though, re this piece of code in image_field_formatter_view() with some explanation and a question:

    if (!$is_svg || $display['settings']['image_svg_display']) {
      // If its not an SVG or, we chose to display as <img>, display in <img> tags.
      // Use the view mode settings to determine the displayed height and width.
      ...
    }
    else {
      // If its an SVG and we dont want <img> tags.

      $svg_path = file_create_url($item['uri']);
      $svg = file_get_contents($svg_path);
      $svg_clean = image_svg_sanitizer_sanitize($svg);
      etc
      ...
      // Because its displaying as an SVG though, we're just using #markup to show it, so no height and width is set unless the 
      SVG file has a built in height and width set, so the view mode settings wont affect anything here.
     // BUT here we could use simplexml_load_string() or Domdocument etc and actually alter the SVGs natural width and height and apply the view 
     mode height and width. However this might lead to issues with SVG Viewbox, and also not sure about performance

    }
indigoxela commented 12 months ago

Hm, the Tubgoats rising again, anyway, testing locally.

I'm struggling to upload an SVG to an image field directly. Selecting from library's no problem. I can edit file.settings.json to also allow svg in "default_allowed_extensions" and then upload svg via /file/add, but for whatever reason the image field doesn't allow svg, although the extension's included in the list ("Allowed file extensions" form item).

So I'm testing with that upload. :shrug:

The formatter still works fine, but wait, image style dimensions get ignored for svg (with display as img tag). Is that by intention? The display summary shows "SVG image attributes: 200 x 200 pixels", although that setting's not visible when opening the form. That's still quite confusing.

Are SVG, when displayed as img tag, supposed to follow image style dimensions, anyway?

Next I played with svg display in the image field, then selected my "nasty" svg with click event, saved, clicked on the svg (now as markup) ... and the event fires. :-1: If that's all the "sanitizer" achieves, then it's not worth embedding, I guess.

Out of curiosity I also checked the File display setting (admin/structure/file-types/manage/image/file-display), but got:

Notice: Undefined index: settings in image_field_formatter_settings_form() (line 658 of ...core/modules/image/image.field.inc).
Notice: Trying to access array offset on value of type null in image_field_formatter_settings_form() (line 658 of ...core/modules/image/image.field.inc).
indigoxela commented 12 months ago

@docwilmot re the questions you raised in comments re image_field_formatter_view()... SVG displayed as-is and image dimensions.

TBH, I don't know, either. Personally, I don't think, I'll ever use direct markup display when using SVG in an image field. Weird idea: a wrapping div with inline style set to width/height? Too odd, I guess. Maybe someone else has a good idea.

docwilmot commented 12 months ago

@indigoxela can you try the sandbox again, seems to be working fine for me.

indigoxela commented 12 months ago

@docwilmot I think, I know why I had trouble to get svg working locally - the "image_svg_support" config item wasn't set.

That means, that svg upload will only be available for fresh installs, or for people who manage to figure out, which json file to patch?

Anyway, confirmed, uploading svg to image fields works in the sandbox. :+1: And the sanitizer also does its job, it seems. Hm, that means, that sanitizing also depends on a hidden config item (image.settings.json), which will only be available on fresh installs?

docwilmot commented 12 months ago

Yes, I'll do an update routine as part of the PR.

indigoxela commented 12 months ago

Yes, I'll do an update routine as part of the PR.

That great :+1:, don't forget it. :wink:

I've left some comments on your PR.

BTW, I'm still not a fan of the extra permission per extension, but... :shrug:

And a reminder, that the File display setting still nags on PHP 8.2 (/admin/structure/file-types/manage/image/file-display):

    Warning: Undefined array key "settings" in image_field_formatter_settings_form() (line 658 of /var/lib/tugboat/core/modules/image/image.field.inc).
    Warning: Trying to access array offset on value of type null in image_field_formatter_settings_form() (line 658 of /var/lib/tugboat/core/modules/image/image.field.inc).
    Deprecated function: strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated in image_field_formatter_settings_form() (line 658 of /var/lib/tugboat/core/modules/image/image.field.inc).
izmeez commented 9 months ago

The PR tugboat has expired. However, the diff applies as a patch to the bd-1.27.0 release without problems. I haven't tested it yet, but will give it a go. Thanks.