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

Follow up: Complement SVG image support in image library and elsewhere #6497

Open
4 tasks
indigoxela opened this issue May 1, 2024 · 5 comments
Open
4 tasks

Comments

@indigoxela
Copy link
Member

indigoxela commented May 1, 2024

Description of the task

As of #5541 core now supports using SVG files as image. 🚀

Some details are still pending discussion and implementation. Im collecting here, what comes to my mind. It might be, that we split this into several issues. We'll see.

  • Validator settings to make them stricter or less strict (override in settings.php or config)
    Do we want to provide such a setting, for example on /admin/structure/file-types/settings or some sort of hook?
  • Consider viewBox attribute as an additional option to determine dimensions
    We currently only rely on width/height attributes in the main <svg> tag in function image_get_svg_dimensions(), but viewBox is another (relatively easy to parse) option. That's the attribute browsers seem to use as a fallback to consider dimensions for rendering.
  • CKEditor drag-and-drop upload
    Currently it's easy to select a previously uploaded svg in the editor, but drag-and-drop upload doesn't seem to work yet. Do we want to add that? Should we discuss that in a separate issue?
  • Consider pre-existing (potentially broken) SVG files
    Or that they don't have dimensions set via image_file_presave().
    Also if site is upgraded from Drupal - do we have to run an update hook? (It might work out of the box)

Additional information

  • Backdrop CMS version: 1.28+
@indigoxela
Copy link
Member Author

indigoxela commented May 1, 2024

Here's a PR to consider viewBox as (additional) fallback. That seemed to be the easiest of the four tasks.

Handy to test: our brand new iconset in core (/core/misc/icons), as none of them has width/height, but all of them have viewBox set. 😉

Edit: PR removed (as not planned)

@indigoxela
Copy link
Member Author

indigoxela commented May 1, 2024

Interesting finding, why image_get_info doesn't work for newly uploaded svg files:

The first check is to ask the image toolkit, if this is something usable - SVG is not, so there's a second check. That check, down the road, relies on the file extension. And what we have at that point of uploading is something like /tmp/phpABCXYZ without any extension.

I belief, that's a bug. @docwilmot what do you think? Or was it @herbdool, who struggled with that?

I guess, this is relevant for CKEditor, as it uses file_validate_is_image() as upload validator for drag-and-drop upload.

@herbdool
Copy link

herbdool commented May 1, 2024

Regarding viewBox, I'm not certain yet if we want to use it for setting the width and height as a fallback. The aspect ratio I can understand, but I'm looking at https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/viewBox and trying to wrap my head around the three example svg with appear the same size but have different viewBox's. The viewBox could be dramatically different, depending on the relationship to the sizes and units of the paths and shapes within it. At least that's how I understand it. Do we want one to be set at width="10" and another at width="100"?

@indigoxela
Copy link
Member Author

@herbdool re viewBox: I absolutely understand, that you hesitate. Things look even more complicated, when switching to the official spec. We could postpone that viewBox parsing, until someone comes up with a request...

Any opinion re the other three subtasks?

@herbdool
Copy link

herbdool commented May 2, 2024

Sounds like the ckeditor drag and drop is important enough to be it's own issue. I don't feel strongly about the others.

I'm guessing pre existing SVG files aren't a big issue. If people were actively uploading bunches of them they were probably installing the svg_image module already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants