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: https://github.com/plone/plone.namedfile/pull/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: https://github.com/plone/plone.namedfile/pull/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/

[ghost]

@ebrehault Sorry, haven't checked github in a while. The plipfile has been updated to include all te work I've done so far. Build with bin/buildout -c plips/plip-1484-retina-image-scales.cfg

Since my last comment I have added some more stuff, mainly the ability to set the scales and quality in the image handling settings. The settings page does rely on some javascript (to only show the relevant image quality fields), so you will need to rebuild the 'plone' css/js bundle in the resource registries to get the full effect.

-- EDIT -- rebuilding the 'plone' bundle currently breaks the toolbar. See https://github.com/plone/Products.CMFPlone/issues/1588 You also might need to run the "Run to51alpha1 upgrade profile." upgrade in Products.CMFPlone:plone on http://localhost:8080/Plone/portal_setup/manage_upgrades

[hvelarde]

can we maintain this work as much version agnostic as possible? I don't see any reason this can't be backported to Plone 4.x.

comments?

[ghost]

@hvelarde There have been quite a few changes in plone.namedfile.scaling, so it won't be truly version agnostic. But you are right, it could technically be backported without too much headache. I don't know if it is desirable as plone needs to move forward with plone 5 and not stay stuck in the past by adding features to older versions.

[jensens]

This can be an addon in Plone 4.3 or could go into a 4.4 (which we do not plan). But to keep on track and from semantic versioning and imo this needs to go as a feature in 5.1.

Sorry for my plone.scale/plone.namedfile changes, but this solves a conceptual problem to a certain degree. If you need help/explanations dont hesitate to ask me.

[hvelarde]

@didrix just remember that Plone 4.3 is LTS and will be here for many years and will be maintained until Plone 6 is out. I still see no reason to migrate any of my sites to Plone 5, and this feature is no exception.

@jensens an add-on is indeed a great idea.

[ebrehault]

@didrix your code looks good to me. As you are working on a lot of different repositories, if your implementation takes too long, you might have to rebase from master a lot and it can be sometimes painful. I guess the TinyMCE image plugin is probably the most tricky part, and can take longer. Maybe we should consider merging what you have done so far quite soon, and then you can focus on Tinymce image plugin. If you are ok with this approach, try to finalize your changes (add tests for instance), and make your PRs.

[thet]

... after the pull-requests are submitted, we can do a FWT review. I'm +1 on having this finished quite soon and +1 to name the reviewers at the next @plone/framework-team meeting. I could be one of the reviewers.

[ghost]

@ebrehault Just created the first pull requests: https://github.com/plone/Products.CMFPlone/pull/1611 https://github.com/plone/plone.namedfile/pull/25

[jensens]

@didrix What is the state of this PLIP? Whats done, whats missing, where is help needed?

[ghost]

@jensens About ready. There is still 1 robottest that fails (since the update of firefox in jenkins).

[ghost]

@ebrehault @jensens @mauritsvanrees . I believe this plip is ready to be merged. I just need to rebase plone.app.upgrade (and Products.CMFPlone I think), but don't want to do it until there is an indication that we can merge soon, otherwise I just stay busy rebasing.

[mauritsvanrees]

I did some more rebases or merges and fixes. It's being tested again in the plip job: http://jenkins.plone.org/view/PLIPs/job/plip-1483-retina-image-scales/21/

Note: @didrix does not have much time to work on this anymore, as he needs to focus on studies, but I am a colleague and can do some of the work if needed.

[mauritsvanrees]

The PLIP job is green again. :-)

[mauritsvanrees]

Any update? Do some @plone/framework-team members still need to vote?

[jensens]

This PLIP is in state "Ready for Review". @thet and I are put ourself in the role of a reviewer, but both had not enough time. Sorry for that! I put this on top of my review list.

[mauritsvanrees]

No problem. Everyone is busy. I am just curious. Thanks!

[mauritsvanrees]

Most branches were in need of a rebase, so I did that. I did some minor fixes as well. I added upgrade documentation too.

I would very much like to land this in the next 5.1 alpha or beta. Final reviews are very welcome.

[mauritsvanrees]

The PLIP job is successful: http://jenkins.plone.org/view/PLIPs/job/plip-1483-retina-image-scales/

(Well, the past two jobs were successful, the current one is still running, but that one should be fine too.)

[mauritsvanrees]

I noticed that we were not using the new portal/@@image_scale in a few places, but were still using getObject and then calling @@images on that. I have updated the plone.app.portlets and plone.app.contenttypes branches to fix that.

[thet]

Just to let you know - I reviewed all but plone.namedfile and it looks pretty good to me (so long quite straight forward...). A nice side effect is, that all image tag replacements are more concise. I'll finish the review ASAP...

[jensens]

TODO List Merge

• [x] review by @thet
• [x] review by @jensens
• [x] discuss at FTW meeting and declare as ready
• [x] check if all rebased are needed meanwhile and run plip-job tests
• merges needed for, but independent from PLIP (fixes/enhancements)
  • [x] plone/plone.app.caching#25
  • [x] plone/plone.app.linkintegrity#39
  • [x] plone/mockup#679
• merge together (core PLIP feature)
  • [x] plone/Products.CMFPlone#1611
  • [x] plone/plone.namedfile#25
  • [x] plone/plone.app.upgrade#76
  • [x] plone/plone.app.portlets#72
  • [x] plone/plone.app.contenttypes#350
  • [x] plone/documentation#781

@mauritsvanrees Anything's missing?

[jensens]

After Plone 5.1.b2 was released (even if still pending) plone.app.upgrade needs minor work. The current upgrade step is to_beta1 and need to be moved to to_beta3 (or to_rc1?).

[mauritsvanrees]

The TODO list contains everything as far as I see. I have updated plone.app.upgrade, and have increased the metadata.xml version in CMFPlone. I have started the Jenkins PLIP job.