PLIP: Retina image scales

[ghost]

Proposer : Diederik Veeze

Seconder : Maurits van Rees

Abstract

Scale images in plone.scale such that they will look good on devices with pixel ratios higher than 1.

Motivation

More and more devices these days can display images with higher resolution than the html/css resolution. Plone however often serves images that are scaled to the exact same size as html/css resolution, while a lot of devices can display twice the resolution.

Assumptions

N/A

Proposal & Implementation

I propose to add an option to the imaging control panel to set a pixel ratio for image scaling. Images generated in plone.scale will generate images whose height and width are multiplied by this pixel ratio. For instance an image in the site of 320x240 with a scaling factor of 2 should result in something like the following:
<img src="someimage-640x480.jpeg" alt="Some Image" width="320" height="240">

Deliverables

• Scaling factor in plone.scale
• Option for pixel ratio in imaging control panel
• Documentation

Risks

The image scales will get larger, but the image quality for these larger scales can be reduced while still giving a satisfactory result, for only a small increase in byte size.

Participants

@mauritsvanrees

Note

I recently signed the contributor assignment agreement and am awaiting confirmation.

[ebrehault]

That's interesting.
Two remarks:

• if I understand what you propose, if we enable the 2x option in the Image control panel, then all the images will be 2 times bigger to load, even for 1x devices (I agree the rendering will be correct because of the width and height attributes, but the network impact will be bad).

Don't you think we could use the HTML5 srcset attribute, which allows to declare several sources according the context:

<img src="images/logo-nope.png"
     srcset="images/logo-big.png 1x,
          images/logo-hd.png 2x"
     width="1824" height="499" alt="">

?
That way, any user will load the appropriate image considering her/his device, and there is no need to force a 2x scaling factor.

• your solution (but also mine) assumes the markup will contain the width and height of the image, but right now, when a user insert an image in TinyMCE, we do not have the width and height attributes on the img tag (we just get something like data-scale="tile"), and that's the same with many templates where images are inserted by plone (for instance in the navigation portlet, we get the news items image with just class="image-icon").
  So we need to change all the img markup generation in Plone.
  It could be done if we use tag() instead of tal:attributes="src blabla" in all the considered templates (plus fixing the TinyMCE image plugin).

Note: that's exactly the same problem @Gagaro is facing in this PR: plone/plone.namedfile#21 (which aims to use SVG in images, for a quite similar purpose as yours, considering SVG scale automatically).

[ghost]

if we enable the 2x option in the Image control panel, then all the images will be 2 times bigger to load

In fact images will be almost 4 times bigger. You could bring jpeg quality down however (assuming it is a jpeg) at these scales without really taking a hit in terms of how the image looks, so that images won't really be that much bigger.

we could use the HTML5 srcset attribute

Indeed, this is better. No support in IE, but fewer high res screens on devices running IE, so I don't think that's a deal breaker.

@ebrehault Thanks for the input.

[hvelarde]

yes, I prefer @ebrehault solution: HTML5 srcset attribute is the future.

take a look at collective.lazysizes and the lazysizes library to see how responsiveness works with that.

supporting HTML5 srcset attribute in the core will make that faster and easier to implement.

big +1 on this PLIP.

[ableeb]

@didrix Welcome! Your Plone Contributors Agreement has been processed. Thank you very much for your contributions.

[thet]

+1 for srcset.
For the records: srcset isn't supported by IE at all, other browsers have good support: http://caniuse.com/#search=srcset
Sounds like, we need a polyfill.
The one, @hvelarde suggested: https://afarkas.github.io/lazysizes/
Or this one, which I was using once: https://scottjehl.github.io/picturefill/

[hvelarde]

in fact there are two options: picturefill and respimg; lazysizes is not a polyfill.

[espenmn]

I have tried to find ways to do this in combination with hveralrde's collective.lazysizes.
My 'solution', unfortunate, requires a hack in alone.namedfile:
https://github.com/espenmn/collective.lazysizes/blob/master/src/collective/lazysizes/overrides/scaling.py#L110

Maybe it could be possible to 'not hardcode' which values to return
When it comes to 'scales': would it be possible to make some 'extra scales' that are 'invisible to the user', and use these for 2x etc ?

In other words: if someone adds a scale with name 'something 100x100', a scale 'something2x 200x200' is also created, but this does not show up in TinyMCE etc.
PS: This should be in an add-on

I put up a demo site here:
http://xweb14f.plana.dk:8050/Lazy/images/view
(look at the html code to see the <img tag
disable js to see 'collective.lazysizes' without the high-res images loaded

[ebrehault]

The PLIP has been approved. See https://community.plone.org/t/framework-team-meeting-minutes-2016-04-12/1943

[jensens]

The overall idea is fine, the implementation details are for discussion.

We provide content image tags by

1. image tag generation (good)
2. manual image tags in templates (bad, should be replaced by a generated tag)
3. TinyMCE generated image tags (well, may need some love also).

Given we want to use generated image tags all over the place, we have a central infrastructure to do so. With modern browsers we can use img tags srcset attribute or the picture element with a fallback to provide different images. Also if we start thinking about it we should do it right from the beginning and think about responsive images as well. Good reading here is https://responsiveimages.org/

So our image tags are generated (currently using very basic string concatenation) we can simply build something to say in a template using a (hypothetic) imagetag function::

 <img tal:replace="structure python:imagetag(context, 'fieldname', 'scalename', title='foo or generated', alt=...." />

and get out of it something like:

<img src="http://...../@@images/fieldname/scalename"
     srcset="http://...../@@images/fieldname/scalename 1x; 
             http://...../@@images/fieldname/scalename/@2 2x" 
     title='foo or generated' 
     alt=.......

or even generate <picture> <source> ..... We may want to make this configurable, at least in a second step.

The basic two ideas behind this are:

• in the page template we do not need to know about retina, responsiveness (not covered here, but we could use the same mechanism to support it in future).
• provide another traversal element in path at the end to provide the factor for the scale
• provide a simple (some kind of template based) way to produce the tag.

Other (out of scope here, but in future) ideas: Let the image tag generator support SVG and inline it this way. The tag needed for this to show a svg could then be generated easily.

[thet]

What I found very interesting is this example from SelfHTML:

 <picture>
  <source srcset="feuerwehr-1600.jpg, feuerwehr-1600hd.jpg 2x" media="(min-width: 1024px)">
  <source srcset="feuerwehr-480.jpg, feuerwehr-480hd.jpg 2x" media="(min-width: 480px)">
  <!---Fallback--->
  <img src="feuerwehr-480.jpg" srcset="feuerwehr-320.jpg, feuerwehr-320hd.jpg 2x" alt="Feuerwehrfest 2014">
</picture>

This provides responsive images AND retina image scales for each of them.

[jensens]

@thet sure, this is possible. But i think this would enhance the scope of this PLIP a lot. I'd like first implement img tag variant with srcset and then its much easier to enhance this again to support responsive images. With responsive images you need also different scales and maybe different croppings (think plone.app.imagecropping) for different device width. This is also very design dependent and so difficult to implement in a generic way. So this may need an own picturetag(...) function with a bunch of parameters for the pagetemplates. And it's also difficult to get this generic into TinyMCE. This would involve at least a configuration option to define which sources are used.

So I'd say: first retina, then look further, otherwise this one gets too complex.

[mauritsvanrees]

Thanks for the comments, interesting.
Of course with one new imagetag standard function to rule them all, the obligatory reference is xkcd...: http://xkcd.com/927/

It is tempting to create such a thing though. For one thing, it makes it easier to notice if a template is still using an old way of showing images. And if needed, this one imagetag function (probably a browser view) could transparantly support multiple image sources, like with currently have with Archetypes and Dexterity, both needing their own kinds of tags. But Archetypes can probably be forgotten at this point.

It might be that @@images can simply be improved instead.

[jensens]

In fact we have this one-for-all with the api.get_view(context, request, "@@images/fieldname").tag() already, but in a template its a but clumsy to use.

[mauritsvanrees]

Well, that is just context/@@images/tag.

[jensens]

well, for the default tag of a scale it is sufficient, if title, alt, or something else need to be passed, then it's clumsy :)

[mauritsvanrees]

Sure, but then you would just need to use this:

<img tal:define="images context/@@images;
     tal:replace="python:images.tag(...)" />

With an imagetag function it would not really be that different, probably something like this:

<img tal:define="imagetag context/@@plone/imagetag;
     tal:replace="python:imagetag(...)" />

It does not seem to matter, except that with an imagetag function like you have in mind, you can pass a different context, which could be nice.

[jensens]

You're right, our current one is not that bad. Overall it is an implementation detail and does not matter so much.

[ebrehault]

And regarding TinyMCE, I guess we have 2 solutions:

• change the image plugin so it produces the expected markup (problem: we will have 2 versions of the same thing, 1 in Python with image.tag(), and one in JS in the plugin, so it is difficult to make it configurable)
• implement an endpoint able to receive the image url + scale and to return the proper markup, and the TinyMCE image plugin would make an AJAX call to this endpoint everytime we modify/add an image.

[jensens]

regarding TinyMCE (and overall in theory beyond) it could also be an option to use transformchain to replace specific image tags, i.e. marked with a specific class or a data attribute with an other tag.

I am just not sure if this is too in-transparent.

[ebrehault]

Using transformchain would imply to ask the server to render+reload the currently edited TinyMCE content everytime an image id added/modified. Sounds more difficult.

[ghost]

For a first (crude) try I made some adaptations in plone.namedfile.scaling. The tag() function now generates a srcset attribute with the links to the appropriately scaled images. This seemed like the simplest solution and it seems the work quite well.

Todo

• Now using hard coded scales of 2x and 3x which should be made configurable via the image handling settings.
• The scales are generated regardless of whether the original file is large enough.
• Not all images are generated using the tag() function. For instance pat-plone-modal (mockup/patterns/model/pattern.js) or the icon images in the news portlet (plone.app.portlets).

Problems

Here are some hurdles I have found:

Waking objects

There is the problem of having to wake objects in order to generate tags from them. For instance in the news portlet images (plone.app.portlets) the image src's are generated using obj.getURL()+'/@@images/image/icon' to avoid waking the object (obj is actually a brain here).

TinyMCE

Tiny always scales the image as scale=large irrespective of how the user scales the image by dragging the corners. Dragging the corners only affect the width and height attributes of the img tag and not the image itself.

[ebrehault]

@didrix, it sounds as a good start!
Could you please provide a link to the branch where you made your changes?

[hvelarde]

awesome! I would leave optimizations to the end: it would be nice to compare computing time on before and after.

[mauritsvanrees]

Plip config is at https://github.com/plone/buildout.coredev/blob/5.1/plips/plip-1483-retina-image-scales.cfg

@didrix Note this recently merged pull request that may influence your work: plone/plone.namedfile#24
You may want to rebase your branch on the new master.

[gforcada]

@didrix @mauritsvanrees is a PLIP jenkins job needed?

[mauritsvanrees]

@gforcada Yes, that would be good.

[gforcada]

@didrix @mauritsvanrees the job is there: http://jenkins.plone.org/view/PLIPs/job/plip-1483-retina-image-scales/

[jensens]

ops, need to read more careful the console output, now its on GitHub

[thet]

@mauritsvanrees I just updated my review with a code-review section about adapterization of plone.namedfile.scaling.ImageScale. This is a non-blocker, but I want to raise awareness (and not loose my thoughts while reviewing) about an issue I might need an answer to sometime soon.

Second thing, re-reading this thread, I found that you already mentioned an imagetag view with some critical thoughts about it. I mentioned the same in my review. Funny thing, I thought I came up with that idea while reviewing your code :)

[mauritsvanrees]

As for testing in the wild: you can see the srcset attributes in use on the carousel of a client of Zest: Zeelandia International.

[jensens]

merged!

[jensens]

Thanks for the great work!

[fgrcon]

Phantastic / I am merging with PLIP #1734 right now. I found still some locations to upgrade:

• plone.app.contenttypes.image.pt,

furthermore

• collection portlet ,
• event portlet.
  These I will fix myself during the merge - image_scale.tag(...) makes quite easy!

How should this work with TinyMce ... ?

[mauritsvanrees]

Thanks for the reviews and the merge.
Glad to have this finally in core. Thanks to colleague @didrix who did most of the work and figuring things out. Congrats on the merge!

So, reading the reviews and final comments, it seems there may be room for some extra fixes or ideas:

• An @@imagetag view which takes a brain as context. But I don't see much that we gain from this. A view on a brain can work fine as far as I know, but I fear a small performance dip.
• Move control panel javascript to a different place. Currently at CMFPlone/static/plone.js. But adding things to a plone-legacy bundle sounds strange (it's legacy, so don't add anything), and making a pattern for something that is only needed in one control panel sounds strange to me as well.
• See if we can let images added via TinyMCE use the retina stuff too.
• Maybe register plone.namedfile.scaling.ImageScale as browser page in zcml so integrators can more easily override it.
• User documentation is missing. There should at least a small chapter about how to activate and configure retina images.
• Update plone.app.contenttypes.image.pt. This already uses the tag function, so seems good already.
• Update [collection portlet] (https://github.com/plone/plone.portlet.collection/blob/master/plone/portlet/collection/collection.pt#L32-L36)
• Update event portlet.
• Maybe replace 'Retina' by 'HiDPI' or something like that in documentation or help texts. See Term "Retina" ok in docs? documentation#812

[fgrcon]

Update plone.app.contenttypes.image.pt. This already uses the tag function, so seems good already.

sorry I uploaded too small images, works fine!"

• Update [collection portlet] done in branch plip1734thumbs (PLIP: Cleanup and enhance icon and thumb aspects #1734) (https://github.com/plone/plone.portlet.collection/blob/4e23864c9703b7f51b010c5a2d97de8f98455910/plone/portlet/collection/collection.pt#L19-L44)
• Update [event portlet] done in branch plip1734thumbs (https://github.com/plone/plone.app.event/blob/6a8ebc01da833de8b32f0249204143a4ce2ad58e/plone/app/event/portlets/portlet_events.pt#L18-L35).