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

Add Image for srcset and lazy loading #2103

Closed
wants to merge 69 commits into from
Closed

Conversation

nzambello
Copy link
Member

Features:

  • Add ImageFromUrl component which selects Plone miniatures using srcset attr and using lazy loading as in image loading lazy #2097
  • Add getSrcSet helper for srcset and sizes basing on selected miniature

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

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

Copy link
Contributor

@giuliaghisini giuliaghisini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow

Copy link
Contributor

@tiberiuichim tiberiuichim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename ImageFromUrl to ScalableImage or something along these lines?

Copy link
Member

@sneridagh sneridagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome PR! Thanks!
Only two things:

still pending, but we want to push it for the upcoming Volto integration package as default for Volto.

The rest is fine for me! Did you test it with a validator? Asko (@datakurre) was using this: https://ausi.github.io/respimagelint/ and did some research on this as well.

@datakurre
Copy link
Member

My experiment, which passed the validator, is here datakurre/my-volto-app@89c86ba

I don't remember my use of sizes any more 🙈

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 🤔

@sneridagh
Copy link
Member

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

@nzambello
Copy link
Member Author

Updated the PR:

  • Refactor of the new component and the helper method
  • Renamed both, now it's Image
  • Loaded low resolution placeholder loading afterwards the with other resolutions using the width descriptor making the browser to decide which image is the best match to load based on the actual size of the image on the page

@nzambello
Copy link
Member Author

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

@nzambello nzambello changed the title feat: add ImageFromUrl for srcset and lazy loading Add Image for srcset and lazy loading Jan 15, 2021
CHANGELOG.md Outdated Show resolved Hide resolved
@avoinea
Copy link
Member

avoinea commented Jan 19, 2021

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

@MrTango
Copy link
Contributor

MrTango commented May 3, 2022

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
Copy link
Contributor

MrTango commented May 3, 2022

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.

@giuliaghisini
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

MrTango commented May 4, 2022

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
Copy link
Contributor

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
Copy link
Contributor

pnicolli commented May 4, 2022

Image scaling discussion notes (Beethoven sprint 2022):

  • we need to render images with explicit width and height attributes on the html elements
  • we need to have scales info in volto somehow in order to do that, because we don't have that info from restapi in listings (querystring-search), for example
  • current idea is to add an expander to get scales information (urls and sizes) for the images of the items returned
  • how do we handle preview_image against image_field? We could actually move the logic to the new restapi expandable service and do that by default, and simplify the frontend
  • keep in mind that images sometimes have copyright info, so maybe add those info to the backend service, or add a different expander. This can be discussed later on.
  • remove loading="lazy" from the placeholder image https://github.com/plone/volto/blob/add_srcset_imgs/src/components/theme/Image/Image.jsx#L116
  • keep using IntersectionObserver instead of loading="lazy" because Chrome behaves badly (loads images way too soon)
  • remove the style attribute when we add the explicit width and height
  • there is a PR in plone.base (Add image srcset definition and json schema plone.base#5) to add a control panel to handle image scales properly in plone 6, discussion is happening there for that point. When that is finished, a dedicated controlpanel can be added to Volto

@tisto
Copy link
Member

tisto commented May 4, 2022

@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

@pnicolli pnicolli mentioned this pull request May 6, 2022
@MrTango
Copy link
Contributor

MrTango commented May 9, 2022

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 added a commit to RedTurtle/design-volto-theme that referenced this pull request Nov 23, 2022
pnicolli added a commit to RedTurtle/design-volto-theme that referenced this pull request Nov 23, 2022
* chore: upgraded volto to 16 alpha 45

* chore: upgrades for volto16

* chore: upgraded mrs-developer

* chore: upgrade dependencies

* refactor: upgrade razzle config for razzle 4

* feat: added layout views i18n names

* chore: removed passwordreset view backport

* refactor: removed mrs-developer, deprecated @italia

* chore: removed routes backports

* chore: re-added our custom Image component

* fix: config and sass dependencies

* chore: updated locales

* fix: styles after sass upgrade

* chore: upgrade dependencies

* chore: added test deployment config

* fix: dockerfile

* fix: cleanup rendering warnings

* fix: workaround nested bootstrap/reboot

* chore: bundlewatch removed

* chore: volto16 update actions

* fix: print styles compilation that broke reboot compilation

* fix: test config

* fix: tests

* chore: customizationPaths

* chore: bundle_size action

* chore: remove duplicated action

* chore: actions cleanup

* fix: invalid comment

* fix: view responsive image in gallery

* fix: image intersectionobserver

* fix: img responsive

* fix: intersection observer simplified

* chore: dockerfile + dockercompose

* feat!: update volto

* feat: node 16 / yarn 3

* ci: yarn 3

* ci: make

* ci: yarn 3

* ci

* ci: dockerfile

* ci: dockerfile

* ci: bundlesize

* docs: docker compose update

* feat: slick font removed

* fix: missing image styles from plone/volto#2103

* chore: upgrade to volto 16.0.0

* build: upgraded docker base image to bullseye, cleaned up env vars

* chore: removed old i18n pre-commit hook

* build: use yarn 3

* build: commit hooks dependencies

Co-authored-by: mamico <[email protected]>
@pnicolli
Copy link
Contributor

Superseded by #3337

@pnicolli pnicolli closed this Jul 18, 2023
@pnicolli pnicolli deleted the add_srcset_imgs branch July 18, 2023 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.