mdn / yari

The platform code behind MDN Web Docs
Mozilla Public License 2.0
1.19k stars 509 forks source link

Styling stretches inline images instead of scaling them #1798

Closed peterbe closed 4 years ago

peterbe commented 4 years ago

This comes from https://github.com/mdn/yari/issues/1734#issuecomment-731337332

If you have

<p>
<img alt="The default styles used by a browser" src="https://mdn.mozillademos.org/files/16493/html-example.png" style="border: 1px solid #cccccc">
</p>

It looks good:

Screen Shot 2020-11-20 at 1 29 54 PM

on http://localhost:3000/en-US/docs/Learn/CSS/First_steps/What_is_CSS

But if you correctly set the image width and height attributes:

<p>
<img height="594" width="1385"
alt="The default styles used by a browser" src="https://mdn.mozillademos.org/files/16493/html-example.png" style="border: 1px solid #cccccc"
>
</p>

then this happens:

Screen Shot 2020-11-20 at 1 31 14 PM

Having the height set is good because it means the browser doesn't need to shift all the stuff below it as the image loads. This causes CLS (Cumulative Layout Shift) which is bad for our Web Perf metrics which could affect our SEO. And just generally make people less happy consuming the content.

Is there not a way to do the CSS so that images are allowed to have their correct size set but "scale" it so it displays as well as it can?

chrisdavidmills commented 4 years ago

Looks like the dimensions are not being set correctly, so that the aspect ratio is incorrect.

If you only set the width, the height is usually adjusted proportionately so the aspect ratio is maintained, and I think it still gives you a box to help avoid CLS. Would that work?

peterbe commented 4 years ago

No, the dimensions are set correct in terms of their ratio.

Screen Shot 2020-11-20 at 1 49 50 PM

It says you should set both width and height to optimize for CLS.

ghost commented 4 years ago

Hello, sorry for the intrusion but this is a problem that I happened to face. Personally after some investigation I would have come to the conclusion that the smartest way is to set the height and width on the Backend. Ideally a microservice for the image that returns an object in different dimensions and the same dimensions in node that can be bound on the template. Instead in the case of SSR and without services you could use Node to request the image (or possibly only the HEAD if this information is in the HEAD?), obtain the dimensions and perform the calculation. How to get 100% width? Also fix the dimensions of the column? 🤔

Does what I said have anything to do with the state of the art of the project? I do not know then the question of scalability as it is impacted .... I have mine.

Ideally (as WordPress does) images should be processed in both upload and download in a known and defined set of sizes.

https://developer.wordpress.org/reference/functions/the_post_thumbnail/ https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/Responsive_images https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/Image

Js solution

https://stackoverflow.com/q/11442712/3579536

I'm not sure that setting dimensions on Front End solve the CLS metrics because i guess that the first parsing is without dimensions so the attributes have to be setted at first parsing...

Where Is dimensions?

The most solid solution seems to be find this data in the image file... https://stackoverflow.com/a/2610307/3579536 Depending on image extension i guess...

schalkneethling commented 4 years ago

I agree with @chrisdavidmills. Set the width, let the browser maintain the aspect ratio as the viewport resizes.

ghost commented 4 years ago

Something (even PHP) for the concept.

other not good solution

Wrap images in a fixed dimensions flex container...

schalkneethling commented 4 years ago

Hey all,

I commented on this without really digging in. I'm ashamed 😞

I now did. So, to fix this and make it so this is a non-issue in terms of the visual display takes one line of CSS. Currently, our reset does this:

img {
  display: block;
  max-width: 100%;
}

With only that, having for example the following in HTML will result in an image that is not proportionately scaled:

<img alt="The default styles used by a browser" src="https://mdn.mozillademos.org/files/16493/html-example.png" width="1385" height="594">

Simply adding height: auto to the previously mentioned rule resolves the problem. This will be added in the next release of minimalist. In terms of best practice, I would say:

  1. The absolute ideal is to set both the width and height
  2. If for whatever reason we can only set one, set the width

Phew, no I feel better 😄

ghost commented 4 years ago

The thing is CLS: until the image is loaded if auto the height is 0.

prevent CLS?

Maybe picture should have fixed height.

picture {
 width: 100%;
 height: 128px;
}
Ryuno-Ki commented 4 years ago

I wonder, whether an img could use a min-height? But then again, what value should be chosen until the image is loaded …

ghost commented 4 years ago

https://github.com/mdn/sprints/issues/3925

schalkneethling commented 4 years ago

With https://github.com/mdn/yari/pull/1860, images that set their width and/or height attributes will be scaled appropriately. If an image uses inline-style to set these values, that is a flaw and we should allow it to look weird so it will be fixed.

If we can do this in an automated fashion, that is great. If not, this will be resolved over time and should be prevented from happening in the future. As mentioned in https://github.com/mdn/yari/issues/1799#issuecomment-734794068, inline styles will override whatever we do unless we use !important and we are not going to do that.

peterbe commented 4 years ago

I love it! Thanks to everyone involved! Now we have the ideal solution. If you set height="594" width="1385" on the <img tag there, you get this:

Screen Shot 2020-11-30 at 10 38 26 AM

It's hard to tell but thanks to the height tag on the <img the browser doesn't do any layout shift whilst waiting for the image.

Now I need to write that piece of code that opens the image, gets its width and height and uses the Cheerio instance to set it based on the binary image size (there's a node lib for that).