Allow SVG uploads in widget (svg_image compatibility)

what happens when you:

• turn off the focal point widget
• upload an SVG to a (lets say) node
• turn the focal point widget back on
• edit the node and hit submit on its form

Its unclear if you ever did that last step from the original post. If you did then a focal point record would be created and I suspect you will get errors all over the place when you try to view that node anywhere.

I understand the use-case you are looking to support but I'm not sure what the "right" user experience should be here. It seems like if we want to allow other mimetypes to be uploaded we would need to detect that and not show the focal point UI and not save the focal point position, etc.

thoughts?

Focal Point is installed in the Varbase Drupal assembly and I cannot load SVG images. It is necessary to allow loading SVG bypassing the Focal Point functions as if we would load it through the standard Image field.

I'm still not convinced that this is the "right" thing to do, but I'm certainly willing to consider it. It may require a checkbox or some other configuration to let site owners affirm "yes, I want to allow SVGs and yes I understand that the focal point wont actually do anything" or some other warning... otherwise I see people submitting all sorts of bugs that focal point isnt working because they do not understand how SVGs work

Patches welcome

So far I have abandoned SVG and used PNG.

This will complicate the user interface and is likely to be an extra feature.
By default, SVG is not used. To add a new extension, we must manually add it in the field settings. Not many will face this challenge. Maybe, when loading a format other than png, gif, jpg, jpeg, make a notification that Focal Point processes only png, gif, jpg, jpeg? Now we get an error that the format is not supported, you can write a notification instead:
Focal Point only works with png, gif, jpg, jpeg. Other formats are downloaded without the participation of Focal Point.
Such a solution will be universal for any other format and will not limit users.

I am not strong in the technical part of the implementation, so my proposal may not be the most successful.

Well from a users perspective this is a quite important functionality. As you can see in svg_image module there's also confusion and demand for that: #3187356: SVG image not being accepted in the Media Library Image type

So I think there are two questions:
1. Can we improve the behaviour if there are file suffixes allowed which focal doesn't support? This would prevent people from searching for the reason why a file suffix doesn't work even if allowed in the field settings.

2. If an SVG is uploaded, wouldn't it be possible to fall back to the default widget (or hide focal functionality and show a message)?

3. Is it technically possible to set a focal point on an SVG? Then we'd need someone to do or sponsor it?

There's a quick fix here, and it's extending the focal_point form widget off of svg_image's widget instead of core's image widget. Just doing that seems to make this work pretty flawlessly, at least in my testing. However, there's a huge downside, and that's svg_image becomes a dependency of focal_point. My guess is that's a no-go, so implementing it properly properly is probably going to require not using svg_image and instead rolling your own svg support. Which makes this task a much larger lift.

In the meantime, here's a patch for the quick fix. With this applied, you can add svg as an available extension in the field settings, and then upload svgs via the focal_point widget. It even previews it with the crosshairs. Of course it doesn't actually crop the svg, since I don't think that's possible, but functionally, this allows svg uploads using the focal_point widget.

Patch #10 worked for me on core 9.2.x

Drupal: 9.1.10
Focal Point: 8.x-1.5

After applying patch from comment #10 the Form Widget Type "Image (Focal Point)" ("image_focal_point") is gone and can't selected any more. Only "Entity Browser" and "Image" are available.

Patch #10 workes for me as well without problems. @CKIDOW, do you have the contrib svg_image module installed? It is required for this patch.

ok, we realized now, that #10 is killing the "image_focal_point" widget in the form display, which kind of defying the purpose - it only happened like this recently, I'm pretty sure it worked 8 days ago...

We are now on Drupal 9.2.7

I added another patch, I think the problem is the changed field_types in the @FieldWidget anotation by @ochenk.

alright, and now a patch, that actually applies... (#12)

And a one, that applies and is working - copy paste is harder then you think ;)

@Schoenef, are you a maintainer? I can't see a commit or release.

#17 works like a charm

Patch #17 working as expected with Drupal 9.4.5, focal_point 1.5.0, and svg_image 1.16.0.

Recommend merging this patch for next release.

Thanks, @Schoenf

The existing patch would enforce a hard dependency to the svg_image module, which is completely unacceptable, so back to needs work.

I can see two options:

a) Duplicate the entire focal point widget code into a sub-module, which can have the dependency

OR

b) make a class alias so we can dynamically extend from the appropriate class, without code duplication and without the user having to configure anything:

namespace Drupal\focal_point\Plugin\Field\FieldWidget;
use Drupal\Core\Field\FieldItemListInterface;
use Drupal\Core\StringTranslation\TranslatableMarkup;
use Drupal\crop\Entity\Crop;
use Drupal\Core\Form\FormStateInterface;
// use Drupal\image\Plugin\Field\FieldWidget\ImageWidget; // remove this
use Drupal\Core\Url;

//and do this instead
if (\Drupal::service('module_handler')->moduleExists('svg_image')) {
  class_alias('PossiblySvgImageWidget', 'Drupal\svg_image\Plugin\Field\FieldWidget\SvgImageWidget')
}
else {
  class_alias('PossiblySvgImageWidget', 'Drupal\image\Plugin\Field\FieldWidget\ImageWidget')
}

/**
 * Plugin implementation of the 'image_focal_point' widget.
 *
 * @FieldWidget(
 *   id = "image_focal_point",
 *   label = @Translation("Image (Focal Point)"),
 *   field_types = {
 *     "image"
 *   }
 * )
 */
//now we dynamically extend depending on the installed modules without a hard dependency
class FocalPointImageWidget extends PossiblySvgImageWidget {
  //rest of the code is unchanged
}

I do have time budget to create a proper merge request, but need feedback from maintainer which way to go.

I think, this should be like -

• Create a base widget for focal point
• Create two widgets in focal point one for focal and one for SVG and both inherits the base widget
• In svg widget, implement ::isApplicable() method to handle if SVG module available or not etc

Changed to postponed due to planned changes in the field widget (convert to standalone form element, 2657592)

I don't think comment #24 covers the root problem: Do we need to inherit from the core image module or from the svg_image module? Creating a base widget surely is a good idea, but it just moves the same problem to the base widget.

But I think I've found the proper way to dynamically react on the existance of the svg_image module: hook_element_plugin_alter (change record, API doc)

It was explicitly designed to change the plugin class on runtime, exactly what is needed here. However, I still think we should wait for the other issue with the standalone form element.

This patch enhances the Drupal Focal Point module to handle SVG images. It introduces a dependency on the SVG Image module and modifies the FocalPointImageWidget to extend from SvgImageWidget instead of ImageWidget. Additionally, it adds a condition to check if the image is an SVG. If it is, the function returns an empty array, bypassing the focal point functionality which is not applicable to SVG images.

Unfortunately, the patch from #27 doesn't seem to be working for me, as the "sub-widget" still doesn't allow SVG.

IMO: this should be at least it's own submodule to focal_point, it not it's own project. the dependency to svg_field is heavy (also new vendor package needed to be installed), forces every site to now use another module even if they don't 'need svg' at this time.
Certainly, would be nice if Drupal Core had an SVG solution in place to work with.

Adding a re-roll of the patch in #13 for 2.1.0

The patch in #27 adds some check which is not part of this patch

Created a merge request with the patch in #31.

Re-roll of patch to add commit from #34. I can also confirm that the patch works for D11.2.10 and focal_point v2.1.2. I will note that, while it allows uploads, it does NOT show updated images in the Preview sub-modal. But, based on the comment in #27, it sounds like this bypass is intentional and expected?