Improve DX of responsive images; convert theme functions to new #type element

@wim thanks for the extensive explanation

The easy ones
3/ This is ported like that from D7 version, and was used as a fallback mechanism, but you're right it should be removed
5/ Also a remain of the D7 version, not anymore needed in D8, comment needs updating
6/ Known issue should be fixed and backported to D7 version

This leaves 1, 2 and 4. If I understand correctly we better use '#type' => 'picture' and instead of passing breakpoints around we need to pass the mapping_id?

New d.o is stripping that.

First stab at a patch following #0. Should fix all points made there.

Fails seem unrelated to this patch...

6: drupal8.picture.2123251-6.patch queued for re-testing.

Patch no longer applies.

Patch #6 rerolled.

FYI: picture is renamed to responsive_image so this needs a reroll once #2124377-74: Rename "Picture" module to "Responsive Image" module is committed

Less yelly title :)

less mangled title.

Oh yeah, D7 d.o--

Adding a very related issue that overlaps/maybe clashes with this. This will also need a reroll as mentioned by @attiks.

Reroll of #11. I didn't test it yet (I will later today or tomorrow) but at least it applies cleanly.

There's one thing I wasn't sure about: #2211831 removed the alt and title attributes from the picture element in theme_responsive_image(), while this patch uses $output[] = '<picture' . new Attribute($variables['attributes']) . '>'; which implicitly allows alt and title attributes again.

Patch in 18 contains an error, this one is better.

In response to #18, alt and title should not be output on the picture element.

This issue is also relevant to solving that: #2220865: Add empty img element inside picture element.

Re #25: I see. Currently, there is just one attributes array, which is applied to the picture element as well as the img element. The question remains we should be able to add other attributes to the picture element. I think we should - there's no way we can be sure it will never be needed. I guess there are two options: 1) add the attributes to the picture element except 'title' and 'alt', or 2) provide a separate attributes array for the picture element.

I did a simple experiment just to see if the patch in #20 makes it easier to leverage the responsive_image module from custom code. I'm attaching the example module, "custom_respimg", to this comment. The important part is the render array in the page controller:

    $output['custom_responsive_image'] = array(
      '#type' => 'responsive_image',
      '#fallback_image_style' => 'large',
      '#mapping_id' => 'default',
      '#uri' => 'public://ddd-logo-l.png',
      '#attributes' => array(
        'alt' => 'Drupal Dev Days logo',
      ),
    );

This code assumes that:
- there is a file called "ddd-logo-l.png" (file included in the tar.gz) in the public:// files directory;
- there is an image style called "large";
- that there is a responsive image mapping called "default".

If everything is OK, the module should display a responsive image at /custom_responsive_image.

Fixing a stupid mistake, tests should pass now.

So, my patch no longer applies because in the mean time [#8611447-36] was committed. That patch defines a ResponsiveImageMappingInterface::getMappings() method which makes rerolling this patch more difficult.

It may be wise to postpone this issue on #2219329: ResponsiveImageMapping::mappings and ResponsiveImageMapping::breakpointGroup properties have inconsistent return structure and type respectively, which aims to make the format of breakpoint groups and responsive image mappings more consistent.

#2260061: Responsive image module does not support sizes/picture polyfill 2.2 contains a bit rewrite, so it might be better to merge the remaining parts into that issue, or postpone it till that is committed

#32 I think we need to update the IS, because most of it is no longer relevant

Since the responsive image module code has been completely overhauled, it's highly like that much of this issue isn't relevant anymore. But somebody should take a look to see if any of this is still relevant.

From the issue summary:
1. is mute because of the points made in 2. Also there is now ResponsiveImageStyle::getImageStyleMappings() and ResponsiveImageStyle::getKeyedImageStyleMappings()

2. We now use #responsive_image_style_id and no longer use #breakpoints. All other points are still valid. Except we use #url in stead of #path (which might still not be ideal?).

3. & 4. These points are also still pretty valid. Except we switched theme_picture() to template_preprocess_responsive_image() and responsive-image.html.twig.

5. Docs are ok now, but naming isn't (see 2. in this comment)

6. I believe there is a separate issue for this. Let me track it down... Jup here it is: #2421317: Do #attributes/$variables['attributes'] to go the <picture> or the fallback <img>?

Trying to put together a to-do list, as it's pretty complicated right now to understand what needs to be done.

Non-field-formatter use cases
The big issue is how can responsive images be used outside of a field formatter context. Field formatters are great, but the last D7 project I worked on had numerous situations where a field formatter wasn't available. I ended up using the theme_picture function but had to feed in the fallback style and breakpoints: it was really awkward, and I'm hoping this can be done better in D8. We may or may not be able to make those improvements at this point, but we can try.

Set fallback style in mapping
If we can avoid passing in the fallback style, and let the mapping take care of that, we'd be better off. #2349461: Move fallback image style into the responsive image style entity does just that.

#type
By changing this to a #type, the Picturefill library could be attached automatically. Preventing people from setting that manually would be a big win.

Change #path to #link_path
Make it clearer that this is for when the image should link to something.

Fallback for no responsive mapping ID unnecessary?
Particularly if we move the fallback style into the mapping, then I have a hard time seeing why a formatter would be set without a mapping being set, where we'd need to fall back to a simple image. Can that fallback be removed?

Clarify URI format in docs and variable name
I definitely ran into confusion over this. If only a stream-wrapped URI is an acceptable value, that should be clear, and the variable name should reflect that.

Picture element attributes
There is very brief discussion about that in #2421317: Do #attributes/$variables['attributes'] to go the <picture> or the fallback <img>?. The global attributes are allowed on picture, although in almost all cases it is probably better to set things like class on the controlling img inside picture. Anyhow, here are the global attributes: https://html.spec.whatwg.org/multipage/dom.html#global-attributes The challenge is how to single out what attributes should be for picture vs img.

So here are the big issues that look like they still need to be solved that aren't being handled elsewhere:

1. Clarify how this can be used outside of the field formatter use case and make fixes as appropriate
2. Decide if this should be converted to #type
3. Possible remove the fallback for when there is no mapping ID
4. Change path to link_path
5. Clarify URI format

Four and five are pretty straightforward. The first three items may need discussion and decisions. We may want to split some of these out to separate issues.

Thoughts?

Oh, also, #1898442: responsive_image.module - Convert theme_ functions to Twig will convert theme_responsive_image_formatter to template_preprocess_responsive_image_formatter. There may or may not be overlap with this issue.

My 2 cents:
#36.1: If we do #36.2, this wil become way easier.
#36.2: I think we should, would be a huge DX win.
#36.3: Yes, I would remove it, I don't see any use for it anymore.
#36.4: ++
#36.5: ++

Since #2349461: Move fallback image style into the responsive image style entity is green, let's wait till it gets committed.

I'll ask Jelle if he feels like working on this

I feel like it ;-)

Great. I'll be happy to test your patch and see if it improves my X as a D :-)

Here's the new patch. Since A LOT has changed to Responsive Image since the last patch an interdiff isn't of any use here.

This is a patch on top of #2349461-27: Move fallback image style into the responsive image style entity (hence the do-not-test), since that patch makes writing this patch a bit easier and will probably soon get committed anyway.
All relevant tests were green on my machine, but I couldn't run all tests.

So to recap from #37:

1. A picture tag can now be rendered by using
   

     $content['hero_image_responsive'] = array(
       '#type' => 'responsive_image',
       '#uri' => 'public://hero-image.png',
       '#attributes' => array(
         'alt' => t('Hero image'),
         'title' => t('Hero image title'),
       ),
       '#responsive_image_style_id' => 'hero',
       '#link_path' => 'http://www.drupal.org/',
     );
     

2. See 1., '#theme' => 'responsive_image' has been changed to '#type' => 'responsive_image'
3. Fallback removed. In fact theme_responsive_image_formatter has been completely removed, the formatter now uses '#type' => 'responsive_image' directly, so the conversion to twig is also no longer necessary.
4. Done.
5. Done.

1.

+++ b/core/modules/responsive_image/responsive_image.module
@@ -71,94 +71,48 @@ function responsive_image_menu() {
 /**
+ * Implements hook_element_info().
+ */
+function responsive_image_element_info() {
+  $types = array();
+  $types['responsive_image'] = array(
+    '#theme' => 'responsive_image',
+    '#attached' => array(
+      'library' => array('core/picturefill'),
+    ),
+  );
+  return $types;
+}
+

hook_element_info() is replaced by annotated classes, see https://www.drupal.org/node/2320115

also you can attach libraries in twig templates directly now, see https://www.drupal.org/node/2456753
2.

and consequently

+++ b/core/modules/responsive_image/responsive_image.module
@@ -71,94 +71,48 @@ function responsive_image_menu() {
 function responsive_image_theme() {
   return array(
     'responsive_image' => array(
       'variables' => array(
+        'link_path' => NULL,
         'uri' => NULL,
-        'width' => NULL,
-        'height' => NULL,
-        'alt' => '',
-        'title' => NULL,
         'attributes' => array(),
         'responsive_image_style_id' => array(),
       ),
     ),

shouldn't this be

    'responsive_image' => array(
      'render element' => 'responsive_image',
    ),

?

I'm working on this at the Drupal Dev Days.

Patch attached should take care of #46.1. I ran all simpletests in the responsive image group locally, they all were green.

I updated my test module which shows that this actually works. The DX has indeed improved a lot; it's pretty easy now to output a responsive image element. Of course a responsive image style must be defined, but it was pretty easy to define it, export it to a yml file and include it in the module.

If you want to test it yourself:

• Start with a standard installation with the Bartik theme
• Download the archive and extract it into the modules directory (you may need to remove the underscore in the .tar_.gz extension first)
• Place the file ddd-logo.png in your files folder
• Enable the module
• Open the /custom-responsive-image page
• Resize the page to see it work

I couldn't figure out if it's possible to serve the image from the module's directory (or from an external URL) instead of the files directory. Being able to do that would make it even more flexible for custom code.

By the way I don't think it's necessary to convert the theme hook from 'variables' to 'render element'. After all the information passed to the theme layer is not a render array, but just a bunch of variables that needs to be processed in template_preprocess_responsive_image().

@Wim did you mean to refer to #50? Because #51 only says we do not have to change the hook_theme implementation, so there's no work needed there.

RE #52: So we need to be able to serve the image from the module's directory (or from an external URL) instead of the files directory?

Module's directory will probably work (given that the file has the right permissions), but external URL is difficult I'm afraid, since we need to know the dimensions of the given image (unless D8 Core can handle that?)

I'll start work on the points made in #54.

New patch with points made in #54.

@Jelle It would be very nice if images can also be loaded from the module directory or from an external URL. I think I've read somewhere that regular image styles in D8 would support external images too, but I couldn't find that post so maybe I am mistaken.

If we can add support for module-directory files (and possibly external files) to this patch with not too much effort, let's do it. If it would take hundreds of lines of code, and several weeks to review and test, then I propose to get this patch committed now and create a follow-up issue.

According to the current documentation of template_preprocess_image_style image styles do not support anything other than stream wrappers (URIs), meaning we don't either since responsive image styles use image styles to output the <source> attributes. So IMHO this should be fixed/added to image styles and my best guess is (with a bit of luck, but I'm not sure) we'll support it out of the box when or if that happens.

EDIT:
There's #1308152: Add stream wrappers to access extension files as well, which would probably help a lot.

EDIT2:
#1358896: Flexible scheme and URI for image derivatives is another related one.

Re-uploaded patch without do-not-test since #2349461: Move fallback image style into the responsive image style entity got committed (woohoo!).

Yeah sorry it's the template. We could do that, but in my personal taste the template looks better / easier to read with the whitespace:

{% if link_path %}
<a href="{{ link_path }}">
{% endif %}
<picture>
  {% if sources %}
    {#
    Internet Explorer 9 doesn't recognise source elements that are wrapped in
    picture tags. See http://scottjehl.github.io/picturefill/#ie9
    #}
    <!--[if IE 9]><video style="display: none;"><![endif]-->
    {% for source_attributes in sources %}
      <source{{ source_attributes }}/>
    {% endfor %}
    <!--[if IE 9]></video><![endif]-->
  {% endif %}
  {# The controlling image, with the fallback image in srcset. #}
  {{ img_element }}
</picture>
{% if link_path %}
</a>
{% endif %}

vs

{% if link_path %}<a href="{{ link_path }}">{% endif %}<picture>
  {% if sources %}
    {#
    Internet Explorer 9 doesn't recognise source elements that are wrapped in
    picture tags. See http://scottjehl.github.io/picturefill/#ie9
    #}
    <!--[if IE 9]><video style="display: none;"><![endif]-->
    {% for source_attributes in sources %}
      <source{{ source_attributes }}/>
    {% endfor %}
    <!--[if IE 9]></video><![endif]-->
  {% endif %}
  {# The controlling image, with the fallback image in srcset. #}
  {{ img_element }}
</picture>{% if link_path %}</a>{% endif %}

Oh nice! I'll change it then.

New patch, added {% spaceless %}

Forgot to update one test with the new missing whitespaces.

Regarding external/module files: I think #1308152: Add stream wrappers to access extension files looks like a very nice solution, much better and more generic than what we could achieve in this issue. Let's leave that for now.

Thanks Wim for the {% spaceless %} tip, I didn't know that one yet.

I'll test the new patch in combination with my proof-of-concept module asap.

FYI, if you still see whitepsace that you don't want use the other whitespace modifiers. {{- -}} or {%- -%} because spaceless doesn't like to go into other control structures deep.

Here's what I'm babbling about:
http://twigfiddle.com/gcenh6

It looks to me like the responsive image formatter is doing some extra things to extract the image's height and width that isn't being done if somebody is using responsive images outside of a field formatter. It would be great if it were just as easy to use responsive images outside of a field formatter, so if that height and width is necessary, can that be done somewhere that benefits both use cases?

Setting to needs work to address my questions about height and width, as well Joel's suggestions for how to handle whitespace.

Not a suggestion more an FYI for when you ask "why you no work like I expect spacesless!"

RE #70: In responsive_image_build_source_attributes we default to the image width and height if they aren't given, which is basically what the formatter does as well. Since this is double code and might confuse some people (they will probably look at the formatter for an example on how to output a <picture> tag). I think removing this part of the code is within the scope of this issue as it improves DX of responsive images, so I did. Relevant tests were still green on my local machine.

Oops, didn't remove enough code :-).

This looks great, thanks all for working on this.

Let's get a beta evaluation for this please in the issue summary, since it's changing APIs but not fixing a critical issue.

I added the beta evaluation to the issue summary.

IS update looks good, does it also need a CR?

Yes, need a CR since this changes how to work with responsive images outside of field formatters.

Done: https://www.drupal.org/node/2475903

Very nice, had another look at the patch and it looks like a nice DX improvement to not have to deal with the library attachment!

My €0.02 regarding the beta change evaluation:

IMHO this is an API addition rather than an API change. At this moment, there is hardly anything that I would call an API if you want to render a responsive image in custom code. Maybe it's currently possible to trick Drupal into thinking that you're rendering an actual field, but that's not an API, that's a hack.

Even if this is considered an API change (admitted: the theme hooks have changed) then I'm sure that impact > disturbance. Looking at the definition of a disruptive change, I think none of the 6 points apply in this case.

Whoa that's like $0.03 CAD:P I agree, any chance you can massage those points a bit to your perspective?

The new render element has me confused. I guess I expected basically a parity with the existing image.html.twig element and what it supports, so that with minimal changes I could change from '#type' => 'image' to '#type' => 'response_image'. Why does this support linking to something when the output should be wrapped in a separate link element itself? I'm used to theme_image vs theme_image_formatter, I guess I expected the same, and now having two different ways of outputting a basic image vs responsive image, seems like worse DX in my mind.

Oh that's a great point @Dave Reid, going to bump it back for some discussion here.

Maybe we can extend #type=>image to get some parity/consistency? (Element class extending kinda/sorta works but pain to merge callbacks), there are examples with some of the form element's extending base elements.

#85 If I understand you correctly, you propose to provide two twig templates:

- image-formatter.html.twig
- image.html.twig

And keep all variables as close as possible?

ok, so #type => responsive_image_formatter or #theme => responsive_image_formatter (second one I guess?)

I closed #1898442: responsive_image.module - Convert theme_ functions to Twig since it will be part of this patch, so we can commit both twig templates at the same time.

re #89, I think there is no need to turn the formatter itself into a render element, so that would be #theme, not #type.

But... How would image-formatter.html.twig be different from responsive-image-formatter.html.twig? I mean, if the responsive image module provides a render element and a field formatter, couldn't the field formatter simply use '#theme' => 'image_formatter' to wrap the responsive image element in a link?

My understanding is that the responsive_image #type is more akin to an image style than to an image. Does that help, @dave-reid?

I'm also unclear why we're talking about going back from #type to #theme. My understanding is we switched to #type in order to be able to attach the Picturefill library.

@mdrummond, we're not talking about going back from #type to #theme. We keep '#type' => 'responsive_image', but we should bring back '#theme' => 'responsive_image_formatter'.

In the current HEAD, there are two theme hooks, 'responsive_image' and 'responsive_image_formatter'. The patch in #44 (and all patches after that) merged the tasks of those two theme functions into a single render element. If we want a clear separation of concerns, then the responsive_image render element should not be responsible for wrapping the responsive image in a link. Wrapping it in a link should be the job of the formatter, because it's the formatter that provides a setting to link the image. Image module follows the same pattern: it separates '#theme' => 'image' (which is not a #type, because it doesn't need to add js by default) from '#theme' => 'image_formatter'.

@marcvangend After my last comment, I was looking at the image module to try to grok what was being proposed. I think I'm almost getting it, and your comment helped clarify further. Not entirely there yet, but then again, I have a bad cold.

As long as we're keeping the benefits of the #type conversion, makes sense to me to restrict the image link to the formatter, as those two don't need to be glommed together if you're calling this through preprocess.

My local machine's tests were being weird. Checking if it's my local setup or a general problem with the code.

I tried to mimic the image formatter as much as possible so everything should be pretty similar. Let's see what the testbot thinks.

Thanks Jelle, it looks pretty much like what I expected. Some comments on the code, and one question:

1. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -74,70 +74,55 @@ function responsive_image_theme() {
   +      'variables' => array('item' => NULL, 'item_attributes' => NULL, 'url' => NULL, 'responsive_image_style_id' => NULL),
   

   I know Image module does this too, but can we break the array up in multiple lines please?

2. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -74,70 +74,55 @@ function responsive_image_theme() {
   + *   - item_attributes: An optional associative array of html attributes to be
   

   s/html/HTML/

3. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -74,70 +74,55 @@ function responsive_image_theme() {
   + *   - responsive_image_style_id: An responsive image style.
   

   s/An/A/

4. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -74,70 +74,55 @@ function responsive_image_theme() {
   +function template_preprocess_responsive_image_formatter(&$variables) {
   +  $variables['responsive_image'] = array(
   +    '#type' => 'responsive_image',
        '#responsive_image_style_id' => $variables['responsive_image_style_id'],
      );
   -  if (isset($item->uri)) {
   -    $responsive_image['#uri'] = $item->uri;
   -  }
   -  elseif ($entity = $item->entity) {
   -    $responsive_image['#uri'] = $entity->getFileUri();
   -    $responsive_image['#entity'] = $entity;
   -  }
   -  $responsive_image['#alt'] = $item->alt;
   +  $item = $variables['item'];
   +  $attributes = array();
   +  // Do not output an empty 'title' attribute.
      if (Unicode::strlen($item->title) != 0) {
   -    $responsive_image['#title'] = $item->title;
   +    $attributes['title'] = $item->title;
      }
   -  if (isset($variables['url'])) {
   -    return \Drupal::l($responsive_image, $variables['url']);
   +  $attributes['alt'] = $item->alt;
   +  $attributes += $variables['item_attributes'];
   +  if (($entity = $item->entity) && empty($item->uri)) {
   +    $variables['responsive_image']['#uri'] = $entity->getFileUri();
   +  }
   +  else {
   +    $variables['responsive_image']['#uri'] = $item->uri;
   

   No criticism, but just for my understanding: why does all this processing happen in template_preprocess_responsive_image_formatter() and not in ResponsiveImageFormatter::viewElements() ? Can someone enlighten me?

5. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -147,13 +132,9 @@ function theme_responsive_image_formatter($variables) {
   - *   - uri: Either the path of the image file (relative to base_path()) or a
   - *     full URL.
   + *   - uri: The URI of the image.
   

   Why did this change? Was the previous doc incorrect, or did we lose the ability to use a full URL somewhere?

6. +++ b/core/modules/responsive_image/templates/responsive-image-formatter.html.twig
   @@ -0,0 +1,19 @@
   + * Responsive theme implementation to display a formatted image field.
   

   This doesn't seem right. Given that image-formatter.html.twig says "Default theme implementation to display a formatted image field.", I guess this should be "Default theme implementation to display a formatted responsive image field". It's not the theme that has become responsive; it's the image.

#96.4: This is basically copy paste from the image module (aside from the obvious changes). I guess we could do all that in ResponsiveImageFormatter::viewElements(), but I thought it would be better being consistent with the image module.

I'll address your other points.

EDIT:
#96.5: The doc was incorrect (see the issue summary).

New patch, only comment & code style changes so should still be green.

Thanks Jelle.

Re #96.4: I understand that you have followed the structure of template_preprocess_image_formatter() and ImageFormatter::viewElements(), and I agree that that's the best approach for this patch. I'm just interested in the rationale behind this structure. If there is no logical explanation, we could create a follow-up issue to clean it up in both modules.

#99: Yeah, I don't understand the rationale behind it either. I don't know who wrote that code, but it would be interesting to hear from them about their reasoning behind it.

#99 and #100: no particular rationale for D8, just a minimal change from D7 code when converting to Twig. Twig conversion approach was always to minimise code changes, and let optimisation for follow ups.

Thanks for the info, @mondrake.

The patch in #98 looks really good to me. It solves the 5 points from #36 and addresses the concerns raised by Dave in #85. The tests are green, my proof-of-concept module shows that it works in real life... I see no reason why this wouldn't be RTBC.

This looks good to me as well. Greatly simplifies how responsive images are used.

I think if there are concerns that preprocess can be simplified, both for images and responsive images, then let's file a follow-up issue for that. The preprocess code doesn't seem like a deal-breaker to me, but if there's a better place to take care of those checks, sure.

Thanks for everybody's hard work on this. Feels like responsive images is getting into a much better place now. After this, only a couple really important issues that still need to get in for responsive images in D8 from my point of view.

Tweaking the UI to allow sizes to be configured is pretty important: #2334387: UI changes to support current responsive image standards.

Could also some review on source attribute streamlining: #2423743: streamline responsive_image_build_source_attributes().

Feel free to start taking a look at those while we wait for core committer reviews on this.

Am I correct in thinking that if a site has not done any custom theming of responsive image this change is not disruptive at all?

True

+++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
@@ -15,6 +15,7 @@
+use Drupal\Component\Utility\Unicode;

Not used.

As to whether or not this is eligible for beta inclusion - I'm inclined to agree that this is followup to the recetn critical and bringing responsive image inline with the rest of core. The work to bring this in line with @Dave Reid's suggestion in #85 is encouraging.

Line removed

Concern in #106 resolved. Moving back to RTBC.

I'm committing this under the reduce fragility (image and responsive_image working the same makes lots of sense) and the followup provision made in the beta evaluation policy. I think the risks of making this change are extremely minor because responsive_image is not enabled by default and if a site had installed it they would have to have do some custom theming to be affected.

Committed 9451f8a and pushed to 8.0.x. Thanks!

Great to see this get in! Thanks alexpott!

I found a small omission that could use a follow-up #2478667: Remove link_path from responsive_image theme. I think we intended to remove link_path from responsive_image to better match image, but we missed that. One line change, so hopefully we can get that fixed quick. I updated the change notice at https://www.drupal.org/node/2475903 to reflect that link_path shouldn't be used.

Can we get that change notice updated to reference this issue please?

Linked this issue and the follow-up in the change notice. Thanks for the suggestion, Cottser.