nhoizey/jekyll-cloudinary

Don’t add <img> `height` & `width` attributes

eeeps opened this issue · 6 comments

eeeps commented

On my test page, the following Liquid tag:

{% cloudinary /assets/test.png alt="beautiful!" %}

Produced an image with width and height attributes. These don't make sense for a couple of reasons.

  1. They represent the dimensions of the full-size image; often we will be loading one of the smaller-downscaled versions in srcset.
  2. The intrinsic width is over-ridden by the standard max-width: 100%; display rule. Without the height, the image would be scaled proportionately, but with it, it is only scaled horizontally, and distorted to fit both the 100% width, and the 1012 height.

height and width are often more trouble than they're worth on responsive layouts. Get rid of them?

The aim was to let the browser know the image ratio so that it can reserve the necessary space, even before the image is loaded, which would limit content "jump".

It looks like the browser don't do it, after all, so you're right, I should remove them.

For your distortion issue, you should anyway always put these two together:

max-width: 100%;
height: auto;

I now remember why I did this.

If the image is small, c_scale will enlarge it, so setting width and height attributes show it at the right size.

Which means it can be an enlarged image (with ugly artefact) shown at a reduced size. Not really efficient.

I just read the image resizing documentation once again, and it seems I should use c_limit instead of c_scale.

I could even remove rmagick dependency, letting Cloudinary do its magic even more… ;-)

@eeeps any advice?

Parts of what I said in previous comment are false.

Yes, c_scale will enlarge the image, while c_limit won't, so the later might be safer. But I still need rmagick anyway, because I try to prevent using superfluous Cloudinary URLs, like the ones where the required width is larger than the natural one. If I succeed, c_limit should not be required.

The main objective of width and height attributes was to show an image at its natural size if it is smaller than its container, because —as @eeeps says— "naturalWidth will always report the evaluated sizes value".

I see two options to show the image at its true natural size when the container is larger:

  1. keep <width> and <height> attributes on the image, easy.
  2. add a breakpoint to sizes for before and after the image is exactly the width of the container. Really more difficult, even maybe impossible, because it means the sizes attribute must be different for each image, but it depends on the design, which is the reason the user currently has to set it manually in_config.yml.
eeeps commented

::just spent a half hour testing things and being surprised::

OK so I agree with you, leave these attributes!

I thought that width= set the intrinsic width, and was trumped by sizes. It doesn’t! It sets the layout width, and it + sizes are independent. CSS layout width: trumps HTML layout width= for layout... but the HTML width=, and not the intrinsic width (as I thought), provides the upper limit for max-width:.

What a horrible and confusing paragraph. Too many “widths!” Anyways, the system works as you describe, and my mental model was all wrong.

As you say, if I just set the CSS height: to auto for all of my responsive images, the height= attribute wouldn't distort my image. And, as you say, it turns out that while space isn't actually reserved on the layout until the image is loaded, the spec has an example that tells you that it is, advocating for exactly the pattern that you’re using... so who knows maybe that'll work some day too.

TL;DR if you don’t want to upscale,* max-width: and height: auto + width= and height= is the best pattern. Use it!

* I’m weird, and in the habit of setting width: instead of max-width: on my images and treating layout width and resource width totally independently...but that's unusual I think

Awesome, I'll put the width and height back… ;-)

Maybe there's an interesting example to build, with both large and small images, and discuss with @respimg people?

After reading the discussion, I am still uncertain what is proposed here. Having width and height attributes screws up my rendering and I can't see a way of removing them.

Adding height: auto to my CSS fixed the rendering issues, though.