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

img: add width and height #351

Open
wants to merge 2 commits into
base: lib
Choose a base branch
from
Open

Conversation

aurelienpierre
Copy link

This complies with HTML5 standards and let browser plan for the area to reserve to images before they load.
If provided, it allows us to enable native browser lazy loading for performance.

This complies with HTML5 standards and let browser plan for the area to reserve to image before they load.
If provided, it allows us to enable native browser lazy loading for performance.
@aurelienpierre
Copy link
Author

I'm not sure whether a file_exists check or a try/catch approach is better here.

@michelf
Copy link
Owner

michelf commented Mar 31, 2021

I'm pretty sure the try/catch approach is better because it's one less request to make. But overall I don't think doing it this way is good.

  • There is no guaranty the parser will run in an environment where where it can look at the filesystem or issue HTTP requests in order to do its job.
  • When parsing user generated content, this could be used to inspect the local file system of the server or issue arbitrary requests from the server, which is a security risk.
  • Loading images will also make parsing slow.

In short, this needs to be 1. optional, 2. off by default, and 3. the strategy for getting this information should be customizable so that you can substitute direct filesystem checks/HTTP requests for something appropriate for other use cases. I suggest adding an optional callback to get this data like there is currently url_filter_func to filter URLs.

It could make sense for the callback to be able to return an arbitrary list of attributes. This would allow other things, for instance adding a srcset attribute with images at various resolutions.

@aurelienpierre
Copy link
Author

The remaining question is how the srcset fits in the Markdown syntax.

The need for width and height attributes in any img element is beyond discussion, it's more than a best practice now.

But Markdown has no way for the writer to specify them manually, and even if it had, it is actually cumbersome, which defeats the purpose of Markdown at all. So, all in all, that needs to be done somehow programmatically but, from what you are saying, at least a manual fallback is needed too (and perhaps the relevant Markdown syntax extension).

Before going any further in the implementation technicalities, I think that's the first design concern to address.

What about something like ![alt](url|WxH) where W and H are pixel numbers, both optional ?

@michelf
Copy link
Owner

michelf commented Apr 1, 2021

I think whether we have a syntax for expressing width and height in the text is independent of whether we have a way to make that automatic. I'm not sure why one would depend on the other, they are independent issues.

All I'm saying is the parser can't decide by itself to fetch files while parsing. This would be unexpected performance-wise and a security risk in many places PHP Markdown is used. So fetching files can't be enabled by default.

My suggestion is simply to have a configurable hook to provide the width and height information, as well as other attributes. That hook, a callback function, is not part of PHP Markdown: it's supplied by the "user" of the library and can get the attributes in whatever way the user wants.

@aurelienpierre
Copy link
Author

aurelienpierre commented Apr 1, 2021

I think whether we have a syntax for expressing width and height in the text is independent of whether we have a way to make that automatic. I'm not sure why one would depend on the other, they are independent issues.

Having an uniform workflow to handle them, if both manual and auto strategies are required, before starting implementation, is saner than jumping head-first into code, and try to retro-fit the manual fallback later. I'm too old to make that mistake again, at least.

My suggestion is simply to have a configurable hook to provide the width and height information, as well as other attributes. That hook, a callback function, is not part of PHP Markdown: it's supplied by the "user" of the library and can get the attributes in whatever way the user wants.

That's fair. But I don't understand what other attributes and how they are supposed to be found. Like, for the srcset, you would look for all img.jpg([a-z]+)$ on the server, hoping to find all the variants and sizes ?

@michelf
Copy link
Owner

michelf commented Apr 1, 2021

I don't mind discussing a syntax, but I don't see it as a prerequisite.

Thinking of it, we already have special attributes blocks in MarkdownExtra that can be used to manually set the width and height:

![alt](url){width=55 height=55}

We could comme with a more elegant syntax for this if needed. Something like ![alt](url 55x55) perhaps? I wonder if other implementations have a syntax for this we could copy to avoid further fragmentation. None have this particular syntax according to Babelmark 2 and [Babelmark 3](https://babelmark.github.io/?text=%5Bimg%5D(url+55x55).

Like, for the srcset, you would look for all img.jpg([a-z]+)$ on the server, hoping to find all the variants and sizes ?

Resized images names often follow a convention, like there could be a thumbnail with a "thumb." prefix or a double-sized version with a "-2x" suffix. Or perhaps a query to a CMS database can tell you what the size of the image is and list all its declinations without having to load any of them (obviously only applicable to URLs managed by your own CMS). Hence the callback: who writes a callback will adapt it to the situation.

@aurelienpierre
Copy link
Author

aurelienpierre commented Apr 1, 2021

I have found 3 implementations that support ![img](url =150x250) : rdiscount, Mou and Marked 2 (source). That might be the way to go.

Hence the callback: who writes a callback will adapt it to the situation.

Ok, got it. So I can give it a start if you are ok with this feature (I just don't want to invest time into a dead-end).

@michelf
Copy link
Owner

michelf commented Apr 1, 2021

showdown has this syntax too.

You can go ahead, syntax & callback. Both are conceptually approved. 👍

@taufik-nurrohman
Copy link

What about query string in URL?

erusev/parsedown#723 (comment)

@michelf
Copy link
Owner

michelf commented Apr 2, 2021

@taufik-nurrohman A query string creates the expectation the server will handle the resize, not the browser. While it's true the resize could happen server-side, in pretty much all cases it won't. I'd rather have syntax that does not create such expectations and is also nicer and less heavy.

I suppose the image size from this syntax could inform a callback hook (url_filter_func?) which could then add query parameters to the URL. Future direction, maybe.

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

Successfully merging this pull request may close these issues.

3 participants