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

Responsive images support #5027

Closed
szymonkups opened this issue Oct 10, 2016 · 28 comments · Fixed by ckeditor/ckeditor5-image#120
Closed

Responsive images support #5027

szymonkups opened this issue Oct 10, 2016 · 28 comments · Fixed by ckeditor/ckeditor5-image#120
Assignees
Labels
package:image type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@szymonkups
Copy link
Contributor

Should image feature be responsible for supporting responsive images? Should this be an editor responsibility?

@szymonkups szymonkups self-assigned this Oct 10, 2016
@Reinmar
Copy link
Member

Reinmar commented Oct 10, 2016

cc @fredck

@Reinmar
Copy link
Member

Reinmar commented Oct 10, 2016

I think that the image feature must somehow consider that. Perhaps it's enough that this is an anticipated and possible extension. Because I don't see how an end-user could specify various sizes through the image dialog.

@fredck
Copy link
Contributor

fredck commented Oct 11, 2016

Exactly. By default, we must take in consideration that the user will have access to one single image size. We may leave it open to have an API that allows for several different sizes.

In the other hand, responsiveness is not about image size, which instead is related to bandwidth optimization. Responsiveness is actually about having the image adapting to different situations. Therefore, if we're able to produce responsive markup (+ CSS) that makes such images responsive, then it would be great.

The CSS thing is the tricky part. The CSS must be in the page delivering the content. It's not a CKEditor feature. In any case, alignment features must be CSS driven as well, so I would not be too worried about it, other than providing a good template in the documentation.

@szymonkups
Copy link
Contributor Author

szymonkups commented Jun 16, 2017

Responsive images are using basically two attributes srcset and sizes.

srcset attribute

The srcset attribute can be provided in two forms:

Containing pixel density descriptor

srcset="image.jpg 1x,
        image-2x.jpg 2x"

This allows to define different image sizes for screens with different density. This is a good solution when image has a fixed width on the page.

Containing width descriptor

srcset="image-480.jpg 480w,
        image-640.jpg 640w"

This allows to define different image sizes and let the browser choose which image should be loaded - this is strictly connected with sizes attribute.

sizes attribute

The sizes attribute is required when srcset containing width descriptor is provided. It can look like this:

sizes="(max-width: 480px) 100vw,
       (max-width 900px) 33vw,
       200px"

This are some tips to the browser about the page layout, helping to decide which image should be loaded. In above example:

  • for viewport sizes up to 480px image will take all the viewport width,
  • for viewport sizes from 481px to 900px it will take 1/3 of the viewport width,
  • for other viewport sizes it will have 200px.

Problem (tldr)

Image responsiveness is clearly connected with its final presentation. We need to know the context: page layout and how the content will be presented there, to properly decide which approach to take and how to eventually generate sizes attribute.
I have some doubts how this should be supported in most generic way possible and what part of this should be an editor responsibility and what not.

@fredck @oleq @Comandeer

@pjasiun
Copy link

pjasiun commented Jun 16, 2017

If there is nothing more we can do, we should take the viewport width. This way we will prevent loading images wider than the viewport width. It's not a perfect solution, but still better than loading always a full-width image.

@Reinmar
Copy link
Member

Reinmar commented Jun 16, 2017

To rephrase a bit the problem – if we set the sizes to a generic 100vw (to indicate that the image always takes 100% of the viewport size) then if someone will have a fullscreen browser window (let's say 1900px) the browser will always load big images. However, most websites use a fixed-width content column (~1000px), so as a result, an unnecessarily big image will be loaded.

From that perspective, sizes is pretty useless for us. So I wonder if we shouldn't try to use the setting with pixel density, but I don't know how we could do that. From what I understand, we'd need to set width of an image and browser would pick the right size based on that, but here, again, we don't know the size the image will have.

Doooh.

@pjasiun
Copy link

pjasiun commented Jun 16, 2017

The problem is that we don't know what is the ratio: width of the editor/width of the image. I think that to be safe we should assume that it can be 100%.

@pjasiun
Copy link

pjasiun commented Jun 16, 2017

Or we can make a config option: editor.image.widthOfTheEditorToWidthOfTheImageRatio. :D

@Reinmar
Copy link
Member

Reinmar commented Jun 16, 2017

A config option may be a go. As ridiculous as it sounds, it seems that this information just has to be hardcoded with the data, so an option to configure this may be the only optimal option.

@Reinmar
Copy link
Member

Reinmar commented Jun 16, 2017

This, ofc, means that the content will be less portable and will be dependent on the current layout of the target webpage. Aaaaaa 😨 😨 😨 😨 😨 😨 😨 😨

So, for some small set of cases a config option may be a solution – it'd allow adjusting the content to the current shape of the website. But it means that if the website's layout is changed, someone may need to reprocess the content in the database.

Therefore, our approach could be different – the default behaviour of the editor would be to output inoptimal but safe 100vw and if anyone wants to optimise this (by aligning the content to the current layout) then they can process the content before serving the website. This solution would be simple for us and would require some work from the other developers if they want to optimise page load times.

@szymonkups
Copy link
Contributor Author

Another doubt from f2f talk with Reinmar is what to do with pasted images. They can have different setup of srcset and sizes, totally not matching user's needs. Should we remove them in such situations?

@fredck
Copy link
Contributor

fredck commented Jun 19, 2017

Another doubt from f2f talk with Reinmar is what to do with pasted images. They can have different setup of srcset and sizes, totally not matching user's needs. Should we remove them in such situations?

For pasted images, we take the biggest available image and upload it, ignoring the rest. The uploading service will then return the proper list of sizes.

@Reinmar
Copy link
Member

Reinmar commented Jun 19, 2017

We can't remove these attributes from all pasted images, because some of these images might've been created in the same editor. We don't have the support yet differentiation between external/internal content yet, so we can't quickly implement it.

But in general, srcset and sizes are kinda stylistic information which we can't align to the current editor needs easily, so stripping it may need to be necessary.

PS. I wonder if browsers put srcset and sizes to the clipboard.

@fredck
Copy link
Contributor

fredck commented Jun 19, 2017

.... and yes, forget about sizes... we play with srcset only... that's all we can do.

@fredck
Copy link
Contributor

fredck commented Jun 19, 2017

But in general, srcset and sizes are kinda stylistic information which we can't align to the current editor needs easily, so stripping it may need to be necessary.

Actually, this is true about sizes. srcset just tell the browser what's available and then it's up to it to decide what to do with that. So, in fact, we don't need to cleanup srcset never.

@Reinmar
Copy link
Member

Reinmar commented Jun 19, 2017

I don't think you're right. sizes is required if you used w in srcset. So this is not that simple.

@pjasiun
Copy link

pjasiun commented Jun 19, 2017

.... and yes, forget about sizes... we play with srcset only... that's all we can do.

I don't think we can do it. sizes should be always.

Actually, this is true about sizes. srcset just tell the browser what's available and then it's up to it to decide what to do with that. So, in fact, we don't need to cleanup srcset never.

I also thought it works this way. Instead, the browser needs to decide which size should be taken before the styles are loaded. The browser needs sizes to decide which size should be taken.

As I said before I think that to be safe we should assume that it can be not wider than 100% viewport width since we don't what percent of the viewport width image takes.

I think that clipboard is a separate issue.

@fredck
Copy link
Contributor

fredck commented Jun 19, 2017

As I said before I think that to be safe we should assume that it can be not wider than 100% viewport width since we don't what percent of the viewport width image takes.

That's the point... "100vw" is the assumed default for sizes so we don't have to specify it. And, this is a presentational attribute, which we should definitely avoid and let the browser decided what to do then.

@Reinmar
Copy link
Member

Reinmar commented Jun 19, 2017

It is the assumed value but it needs to be specified anyway:

image

sizes parsing algorithm (defaults to 100vw): https://html.spec.whatwg.org/#parsing-a-sizes-attribute

Plus:

If the srcset attribute is present and has any image candidate strings using a width descriptor, the sizes attribute must also be present, and is a sizes attribute. The sizes attribute contributes the source size to the source set (if no source element was selected).

@szymonkups
Copy link
Contributor Author

I guess we can keep srcset values in the model and convert it to two attributes in the view, where sizes will be taken from configuration (with 100vw as defalut). If I assume everything correctly, this way we will always remove some custom sizes and always provide pre-configured value.

@Reinmar
Copy link
Member

Reinmar commented Jun 19, 2017

👍 I wanted to propose the same. It's ok for now.

Later, we may think what to do if someone loaded into the editor a different sizes value than the default/configured one. It's a bigger task to handle such values (there are valid cases where the loaded value should be maintained by the editor) because we need to be handled on copy&paste.

@wwalc
Copy link
Member

wwalc commented Jun 19, 2017

This, ofc, means that the content will be less portable and will be dependent on the current layout of the target webpage. Aaaaaa 😨 😨 😨 😨 😨 😨 😨 😨

So, for some small set of cases a config option may be a solution – it'd allow adjusting the content to the current shape of the website. But it means that if the website's layout is changed, someone may need to reprocess the content in the database.

Sounds like an interesting idea. Hardcoding anything which at some point will become invalid may sound stupid, but you can always write a script to update your image tags in your database.
At the same at a cheap cost you may protect yourself against loading extremely big images instantly.

Therefore, our approach could be different – the default behaviour of the editor would be to output inoptimal but safe 100vw and if anyone wants to optimise this (by aligning the content to the current layout) then they can process the content before serving the website. This solution would be simple for us and would require some work from the other developers if they want to optimise page load times.

Looks like the way to go as well, it can be a "recommended practice" and an OOTB solution which we could provide e.g. in Drupal. Suppose that some day we provide a module Drupal that allows for inserting responsive images. What we could do there is introducing an easy to understand "runtime" configuration option that can be set at any moment.
E.g. the maximum width of responsive images (with an explanation what it actually means).

The value of this config option would adjust "on the fly" the sizes attribute inside rendered <img> tags, whenever the content is rendered by the Drupal engine.
If the administrator changes website layout, all he'd need to do, would be adjusting that runtime config option. No need to fix anything in the database to "fix" existing content.

@fredck
Copy link
Contributor

fredck commented Jun 19, 2017

Loved this idea ;)

@pjasiun
Copy link

pjasiun commented Jun 19, 2017

I also agree that it should be not optimized 100vw or let it to the integration to overwrite sized when it's rendered.

At the same time, I don't think that making sizes configurable is a good idea. It's not a proper solution from the beginning if it is the editor who set sizes (using this option) then they will be broken when the layout changes. The proper way to set sizes is when the page is rendered not created, the config option would be misleading. Also, we may bring more options like maximum or default width, handle densities. We may need to think then how the sized config option should work with them.

.g. the maximum width of responsive images (with an explanation what it actually means).

Note that these may be the config option of the upload adapter, not the image feature.

@Reinmar
Copy link
Member

Reinmar commented Jun 19, 2017

At the same time, I don't think that making sizes configurable is a good idea. It's not a proper solution from the beginning if it is the editor who set sizes (using this option) then they will be broken when the layout changes.

While I agree that the config option is not needed now as anyone can process the content and fix the value when rendering the website, let's not forget about simple integration cases and simple websites where it's really just a lot easier to configure the editor than to introduce some HTML processor on page render.

Of course, one could also do a simple /sizes="100vw"/.../g replacement. Therefore, it came to my mind that instead of making the value configurable to any arbitrary string we could make it configurable to be either the default 100vw or some easy-to-search placeholder (like sizes="RESPONSIVE_IMAGE_SIZES") which would indicate that this is to be replaced by the system. OTOH, searching for that placeholder is as good as searching for sizes="100vw" so I guess we'll never need that option anyway (the only unsafe situations are technical websites where someone might really type such a string).

@pjasiun
Copy link

pjasiun commented Jun 19, 2017

Therefore, it came to my mind that instead of making the value configurable to any arbitrary string we could make it configurable to be either the default 100vw or some easy-to-search placeholder (like sizes="RESPONSIVE_IMAGE_SIZES") which would indicate that this is to be replaced by the system.

Yea, I was also thinking about some this kind of the placeholder.

OTOH, searching for that placeholder is as good as searching for sizes="100vw" so I guess we'll never need that option anyway (the only unsafe situations are technical websites where someone might really type such a string).

Well, not exactly. We can not promise that it will be always 100vw, but we can promise that it will be always [RESPONSIVE_IMAGE_SIZES] if you turn it on.

@fredck
Copy link
Contributor

fredck commented Jun 19, 2017

KISS in the beginning, please... no configurations and srcset only being output will make the deal for now.

Reinmar referenced this issue in ckeditor/ckeditor5-image Jun 23, 2017
Feature: Introduced support for responsive image's `srcset` attribute. Closes #2.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-image Oct 9, 2019
@mlewand mlewand added this to the iteration 11 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:image labels Oct 9, 2019
@Isaacn123
Copy link

am getting this how can i solve this <figure class="image"><img></figure>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:image type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants