search

plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
426 stars 575 forks source link

Add Image for srcset and lazy loading #2103

Closed nzambello closed 9 months ago

nzambello commented 3 years ago

Features:

Added code and tests, refactored components with images to use ImageFromUrl

This should improve performances avoiding loading heavy images if not needed.

datakurre commented 3 years ago

My experiment, which passed the validator, is here https://github.com/datakurre/my-volto-app/commit/89c86ba93db2ad404f45a7dfc5fc9dde686390e6

I don't remember my use of sizes any more :see_no_evil:

But I recall that we had to hack Plone image sizes with unlimited heights, e.g.

huge 1600:65536
great 1200:65536
large 800:65536
preview 400:65536
mini 200:65536

to avoid browser getting scale, which required blocky upscaling :thinking:

sneridagh commented 3 years ago

@datakurre thanks a lot!! I searched for it, but I couldn't find it!

nzambello commented 3 years ago

Updated the PR:

nzambello commented 3 years ago

Preview of the loading with the placeholder. https://user-images.githubusercontent.com/21101435/104717825-37b84c00-572a-11eb-847f-d1dc75237bde.mov

Heavily inspired by Gatsby image

avoinea commented 3 years ago

Sizes we use for the EEA website:

print 2000:2000
panoramic 1920:1080
landscape 1370:771
portrait 771:1370
xlarge 950:950
wide 325:183
large 768:768
preview 400:400
mini 200:200
thumb 128:128
tile 64:64
icon 32:32
listing 16:16
tiberiuichim commented 3 years ago

It doesn't seem like the srcset properly responds to the images. I can see the /listing being loaded, but it's always loading the big image instead of one of the smaller sizes. Maybe the sizes attribute needs to be specified?

datakurre commented 3 years ago

I'm sorry to have not been able to try this pull yet, but reading the Smasing Magazine article and trying out the linter really helped me:

So srcset could describe all the available versions and their widths, but it may not be enough for browser to properly choose the size, because browser might load the image before it knows the eventual size of the rendered image area (and loading the largest one just in case). That's why sizes is needed to tell the browser the size of the image to help it to choose the right scale:

https://github.com/datakurre/my-volto-app/commit/89c86ba93db2ad404f45a7dfc5fc9dde686390e6#diff-b46cef1aaa7352b0e8144c7a2e58944f9640faea4b66dff62bfa86bd1f9aa8b1R48

Also, because sizing is based on widths, Plone image scaling must provide those widths or images may be upscaled on browser. That's how I ended with image sizes like large 800:65536.

tiberiuichim commented 3 years ago

Thanks @datakurre!

I ran the validator on the PR, on a page with a listing block and a hero block. Here's the things it has to say:

Screenshot 2021-01-23 124120

tiberiuichim commented 3 years ago

Isn't the @@images/image/listing size too small? 16px, as is the default, is no bigger then a small icon. Yes, it loads fast, but it's also ugly. I suggest maybe use tile (64px) or thumb (128px) as default?

datakurre commented 3 years ago

For me the validator results look like I expected: Plone image scaling is too smart for srcset and that's why widths do not match. My indefinitive heights in image scale forces Plone always to scale by width, what should make it match.That should also prevent cropping mentioned by validator. Also sizes-attribute should be set (and that's the hardest one, because its values probably depend on themes).

tiberiuichim commented 3 years ago

@datakurre No worries, I haven't yet plugged the new image scales into Plone, this was with the default image scales. I am trying to get a feeling of what this whole problem space is about. The above comment, with the screenshot, wasn't aimed at you. Just documenting things as I encounter them

tiberiuichim commented 3 years ago

@nzambello My hope for this PR is that, in the end, it provides a comprehensive solution for responsive images. One issue I have though, and maybe I have read the code wrong, but it seems to only take Plone image content types into account (so basically, only the "image" field). I think this should be handled better and allow also handling content types with either non-standard image field or multiple image fields.

datakurre commented 3 years ago

No problem. For me the key for understanding this was to acknowledge that there is no magic involved, but browser really must be able to decide the optimal scale from HTML only. On 23. Jan 2021, 13.23 +0200, Tiberiu Ichim notifications@github.com, wrote:

@nzambello My hope for this PR is that, in the end, it provides a comprehensive solution for responsive images. On issue I have though, and maybe I have read the code wrong, but it seems to only take Plone image content types into account (so basically, only the "image" field). I think this should be handled better and allow also handling content types with either non-standard image field or multiple image fields. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

tiberiuichim commented 3 years ago

Another idea: when definining image scales, I think it could be interesting to also define resizing parameters. Looking at these parameters, for example (most important, fit type and positions. Right now this PR only defines width in the image scales, thought kitconcept.volto defines square images as well (which we may want to chose how to fit):

    icon 32:32
    tile 64:64
    thumb 128:128
    mini 200:65536
    preview 400:65536
    teaser 600:65536
    large 800:65536
    larger 1000:65536
    great 1200:65536
    huge 1600:65536

The square image formats would need to be cropped.

tisto commented 3 years ago

Roadmap:

nzambello commented 2 years ago

@sneridagh we now have the correct image scale being loaded only when the image is visible and the placeholder has loaded, so we have the actual size of the image in the page. I don't fully like this behavior, but I think it's the best way we have without forcing developers to know and set the final image width. It would be an anti-responsive pattern I guess.

giuliaghisini commented 2 years ago

@nzambello it should be updated with the new way of showing images that is now used in the image block

nzambello commented 2 years ago

@plone/volto-team this is ready IMO. Fixed remaining things, tested in a production environment and updated the documentation.

netlify[bot] commented 2 years ago

Deploy Preview for volto ready!

Name Link
Latest commit a07bc4c13ea948e7aacddf47dab623b072939cd2
Latest deploy log https://app.netlify.com/sites/volto/deploys/62b31ac1a31ee00008e28eca
Deploy Preview https://deploy-preview-2103--volto.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

MrTango commented 2 years ago

I'm not sure if understand the code right, but you did not seem to define sizes right? I think it would be better, so that the browser knows the image size before it is loaded, even more so when using lazy loading. I'm currently trying to solve this for classic UI. On one hand enabling the browser to automatically select the right image scale is awesome. But i would like to also allow art direction. In the form of providing different cropped versions for smaller screens, like i could create via plone.app.imagecropping. Art direction is only possible via source tags in combination with media queries.
This is more explicit and you can force the browser to choose e special scale which contains your cropped image. With srcset and sizes the browser will choose what ever fit's in the moment, even a bigger scale, when the bigger scale is already in the cache. This way you might see the bigger scale on small screens which is fine from the quality point of view, but might not what you want. The bigger scale does not have the crop and looks bad on mobile. Here is a good example of that problem and the solution: https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/Responsive_images

The challenge now is to find the right amount of flexibility without making the configuration to complex. I would like to exchange ideas on that topic. I'll join the team in Bucharest for the Beethoven Sprint, i hope we can make some progress then.

giuliaghisini commented 2 years ago

I'm not sure if understand the code right, but you did not seem to define sizes right? I think it would be better, so that the browser knows the image size before it is loaded, even more so when using lazy loading. I'm currently trying to solve this for classic UI. On one hand enabling the browser to automatically select the right image scale is awesome. But i would like to also allow art direction. In the form of providing different cropped versions for smaller screens, like i could create via plone.app.imagecropping. Art direction is only possible via source tags in combination with media queries. This is more explicit and you can force the browser to choose e special scale which contains your cropped image. With srcset and sizes the browser will choose what ever fit's in the moment, even a bigger scale, when the bigger scale is already in the cache. This way you might see the bigger scale on small screens which is fine from the quality point of view, but might not what you want. The bigger scale does not have the crop and looks bad on mobile. Here is a good example of that problem and the solution: https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/Responsive_images

The challenge now is to find the right amount of flexibility without making the configuration to complex. I would like to exchange ideas on that topic. I'll join the team in Bucharest for the Beethoven Sprint, i hope we can make some progress then.

@MrTango image sizes are defined here https://github.com/plone/volto/blob/3660875628a9c721994a7de4a10309ad5735f1d9/src/config/ImageScales.jsx and inject in config here https://github.com/plone/volto/blob/3660875628a9c721994a7de4a10309ad5735f1d9/src/config/index.js#L117.

Browser try to determine the size of image here https://github.com/plone/volto/blob/3660875628a9c721994a7de4a10309ad5735f1d9/src/components/theme/Image/Image.jsx#L65 and loads the src-set maximum image size based on available width and pixel ratio.

In Volto we don't have yet the concept of 'cropped image' only the scaled image. I think crop will be a next feature and is not a theme to take care now on Volto.

MrTango commented 1 year ago

Do you set the sizes attribute somewhere? This is important for the browser to know how big the image will be on the screen. This will be used to calculate the needed image size by looking at the screen resolution (2x,3x aso) and the Intrinsic_Size of the image.

For example:

<picture class="image-size-large">
  <source
    srcset="https://picsum.photos/id/1011/800 800w,
            https://picsum.photos/id/1011/1000 1000w,
            https://picsum.photos/id/1011/1200 1200w,
            https://picsum.photos/id/1011/1600 1600w"
    sizes="800px"
    >
  <img src="https://picsum.photos/id/1011/800" sizes="800px">
</picture>

The image intrinsic size of the image is 800px. On a older 1x desktop screen that means the browser might prefer the image scale: https://picsum.photos/id/1011/800, but on a high dpi screen let's say 2x, the Browser would require the scale https://picsum.photos/id/1011/1600 1600w

If you don't provide the sizes attribute, the intrinsic size has to be calculated from CSS definitions, that takes more time.

PS. I'll be at the sprint in Bucharest the following days, let me know if we could sit together to discuss this and also my ideas for the srcset config in the backend. I made the default config much smaler now, but it is still valid to have it and gives us to the option to improve the frontends in time more.

giuliaghisini commented 1 year ago

Do you set the sizes attribute somewhere? This is important for the browser to know how big the image will be on the screen. This will be used to calculate the needed image size by looking at the screen resolution (2x,3x aso) and the Intrinsic_Size of the image.

For example:

<picture class="image-size-large">
  <source
    srcset="https://picsum.photos/id/1011/800 800w,
            https://picsum.photos/id/1011/1000 1000w,
            https://picsum.photos/id/1011/1200 1200w,
            https://picsum.photos/id/1011/1600 1600w"
    sizes="800px"
    >
  <img src="https://picsum.photos/id/1011/800" sizes="800px">
</picture>

The image intrinsic size of the image is 800px. On a older 1x desktop screen that means the browser might prefer the image scale: https://picsum.photos/id/1011/800, but on a high dpi screen let's say 2x, the Browser would require the scale https://picsum.photos/id/1011/1600 1600w

If you don't provide the sizes attribute, the intrinsic size has to be calculated from CSS definitions, that takes more time.

PS. I'll be at the sprint in Bucharest the following days, let me know if we could sit together to discuss this and also my ideas for the srcset config in the backend. I made the default config much smaler now, but it is still valid to have it and gives us to the option to improve the frontends in time more.

yes, for example, the html generated is this: <picture class="volto-image"><source srcset="/huge.jpg/@@images/image/listing 16w, /huge.jpg/@@images/image/icon 32w, /huge.jpg/@@images/image/tile 64w, /huge.jpg/@@images/image/thumb 128w, /huge.jpg/@@images/image/mini 200w, /huge.jpg/@@images/image/midi 300w, /huge.jpg/@@images/image/preview 400w, /huge.jpg/@@images/image/teaser 600w, /huge.jpg/@@images/image/large 800w, /huge.jpg/@@images/image/larger 1000w, /huge.jpg/@@images/image/great 1200w, /huge.jpg/@@images/image/huge 1600w"><img src="/huge.jpg/@@images/image/listing" alt="" class="full-width" role="img" loading="lazy" style="width:100%;object-fit:cover"></picture>

we didn't set the real size of image in attributes because we don't know the real size of image here, we only know the miniature size.. this is the big problem of this implementation and need to think about it..
Another problem that we have here, is that srcset is injected in page only when image meets intersection observer.. it was a way to do the lazy loading of image... but i don't think it's the best way to do it, because this causes two image loading for one image:

Any ideas?

datakurre commented 1 year ago

@giuliaghisini I'm sorry for my previous comment (I now deleted) that made no sense.

Regarding sizes, could sizes="100vw" be better than nothing? At least then browser would never choose a size greater than the window itself. (Or does browser act like that by default?)

Or does it already work properly? Just loading listing size at first and the correct scale when CSS has been calculated?

I recall that my old PoC https://github.com/datakurre/my-volto-app/commit/89c86ba93db2ad404f45a7dfc5fc9dde686390e6#diff-3f5fdd396d4f51d380bccc043229925fc08f0b5c5bffa414d8e4e1cf9325e55bR19 only loaded a single scale, but I did not try that with loading="lazy" :thinking:

giuliaghisini commented 1 year ago

@giuliaghisini I'm sorry for my previous comment (I now deleted) that made no sense.

Regarding sizes, could sizes="100vw" be better than nothing? At least then browser would never choose a size greater than the window itself. (Or does browser act like that by default?)

Or does it already work properly? Just loading listing size at first and the correct scale when CSS has been calculated?

I recall that my old PoC datakurre/my-volto-app@89c86ba#diff-3f5fdd396d4f51d380bccc043229925fc08f0b5c5bffa414d8e4e1cf9325e55bR19 only loaded a single scale, but I did not try that with loading="lazy" 🤔

yes, it could be better than nothing.. it's the sizes of the 'placeholder' slots into which the browser can insert images from the srcset.. Maybe 100vw will be too much, it will cause some 'flickers' area in page when real image is loaded.. i think.. we have to try...

Maybe sizes could be setted in applySrcSet fn where we know the real image size.. but i don't know it could be correct.. what do you think?

giuliaghisini commented 1 year ago

@datakurre @MrTango i've added sizes="100vw" as default and make some improvements. Now i think it's working much better than before.. Could you try it? for sizes.. i think we cannot calculate them.. it depends on page layout.. i've added sizes as props to pass to the Image component and setted default to '100vw'

datakurre commented 1 year ago

@giuliaghisini Did you try https://ausi.github.io/respimagelint/ already?

datakurre commented 1 year ago

I think sizes as a prop is good start. Once someone is really building responsive theme with optimal sec-sets, that someone probably gets an idea, how this should be enhanced.

MrTango commented 1 year ago

giuliaghisini

we didn't set the real size of image in attributes because we don't know the real size of image here, we only know the miniature size.. this is the big problem of this implementation and need to think about it..

while defining the srcset we give all the needed info's by appending the size like 800w of that scale. The sizes attribute, is not only useful together with media queries, if you leave them of as i did in my example, you go just with the fallback size of 800px. This size is the size the image would take on the screen, no matter of the screen resolution.

On a 1x resolution screen it will be 800px as well as on a 2x resolution screen. Think of it as the physical size of the image.

Another problem that we have here, is that srcset is injected in page only when image meets intersection observer.. it was a way to do the lazy loading of image... but i don't think it's the best way to do it, because this causes two image loading for one image:

the image set in src attribute of (/huge.jpg/@@images/image/listing)
and the needed image size from srcset.

i don't why you think you need this, if you add the loading="lazy" attribute, the browser should be able to load lazy. Right now chrome will still load to fast all images, firefox and safari will save more data and only load images when you are getting close to them. So with that in mind, if you want the optimum control and have it working on chrome you have to do it with intersection observer.

https://www.ctrl.blog/entry/lazy-loading-viewports.html

For Plone 6 classic UI we will probably stick with the browser defaults and just use loading="lazy". Chrome will eventually fix the behavior.

I guess when you add the srcset and the src attributes via the observer it should work, the browser can't load anything without either a src or a srcset.

datakurre: src-set should really just list all the available images and their real pixel width

then

sizes attributes is there for telling the view port size; it supports responsive breakpoints and CSS math

Just with these in place, browser will pick whatever size it sees best fit. I didn't even see 2x or 3x src-set items to be really required, because browser was able know its pixel density and choose the right image, as long as one was available.

not quite, the sizes attribute with out a media query defines the Intrinsic Size of the image, the same you can archive by setting the width attribute on the img tag. But it is true that the Browser is able to do the math for us, by calculating Intrinsic Size * screen resolution >> needed image size.

With these sizes in place, I know that image scaling always scaled by width, src-set was giving correct information and browser was able choose the right size.

sizes attribute, of course, is more tricky, because its optimal value really depends on your theme.

it depends if you want to change the Intrinsic Size for different viewports, most of the time that not needed. With my example above, the image is 800px on a Desktop. For mobile that resolution works too. As we need a bit less than 400px usually * screen resolution (somewhere between 2x and 4x), leaves us with needed sizes of (800px up to 1600). Together with the general definition of img {max-with: 100%;} it only make sense when the image is smaller on Desktop than on a smaller screen.

For example if our image is 250px and we want it to be almost full screen on a mobile device.
Than something like this make sense: sizes="(max-width: 768px) 90vw, 250px".

In most cases it's not worth optimizing to much.

https://css-tricks.com/a-guide-to-the-responsive-images-syntax-in-html/

MrTango commented 1 year ago

also one thing i noted while playing around with my example page is that we need to set the height when we are using lazy loading or everything jumps arround :dancers: I hope we can get together tomorrow on discord for a chat.

giuliaghisini commented 1 year ago

also one thing i noted while playing around with my example page is that we need to set the height when we are using lazy loading or everything jumps arround 👯 I hope we can get together tomorrow on discord for a chat.

sure! we could talk about this on discord. i'm not seeing some 'jumping' since yesterday afternoon improovments... but we could take a look together !

giuliaghisini commented 1 year ago

@MrTango i've investigated on 'jumping' images... and... i found that maybe it's to the miniature fault.... I'll explain.. Until real image to display is loaded, the image with the lowest scale among those configured in the config in config.settings.imageScales is loaded as placeholder. In our case the smallest scale is 'listing' which has a width of 16px. For example, my image have a listing miniature that is 16x10px and the actual image will be shown by the srcset is 833x555px. The proportions of the two images are not the same, in fact: 16x10px proportion is 1,6:1 and 833x555px proportion is 1,5:1

this is the reason for the 'jumpings' i think.. and these images are coming from plone that maybe rounded x or y dimensions when scaling...

MrTango commented 1 year ago

The jump will always happen when the browser does not have a hight to start with, before it lazy loads the image. So i think we should set width and height attributes on the img tag.

giuliaghisini commented 1 year ago

The jump will always happen when the browser does not have a hight to start with, before it lazy loads the image. So i think we should set width and height attributes on the img tag.

it could be a solution.. but we don't know the real image height..

pnicolli commented 1 year ago

Image scaling discussion notes (Beethoven sprint 2022):

tisto commented 1 year ago

@MrTango @giuliaghisini modern browsers allow you to set the width and height params without "px", so you basically just set the aspect ratio. This allows the browser to fully render the image properly (avoid jumps) and at the same time provides full flexibility of the srcset attribute:

https://www.smashingmagazine.com/2021/04/humble-img-element-core-web-vitals/#the-basics

MrTango commented 1 year ago

Today's testing showed that it does not matter much, what size we are putting into width/height attributes. As long as they have the same aspect ratio. So width="1" and height="1" will do the same as width="300" and height="300", together which some CSS to have the image width at 300px. Both "px" and "vw" work well in CSS, but "%" does not, images are jumping when using "%" for width.

So as long as we set width and height with the correct aspect ration, we can change it via CSS width and height="auto" as we want to. It event works well with max-width="100%", the Browser will recalculate continuously and load images from the provides list of options in the srcset attribute.

pnicolli commented 9 months ago

Superseded by #3337