Responsive image module does not support sizes/picture polyfill 2.2

FYI, two excellent articles explaining the new specification of the picture element, in particular its srcset and sizes attributes:
- Srcset and sizes, http://ericportis.com/posts/2014/srcset-sizes/
- The new srcset and sizes explained, http://martinwolf.org/2014/05/07/the-new-srcset-and-sizes-explained/

One of the most important issues we need to solve in order to make this possible is: #2220865. We're hung up on getting some tests written.

There are a whole bunch of other issues related to responsive images and breakpoints that could use some love. Here's a quick glance:

https://drupal.org/project/issues/search/drupal?project_issue_followers=...

This is a good meta being used to track these as well: #2203447: [Meta] Followup Responsive_image(Picture element) and Breakpoints in core

The other issue we need to get started is adding a breakpoint UI to core. With recent changes, the breakpoint config files in a theme are only used when you install a theme. If you want to edit breakpoints once a theme is installed, you need to export that config, edit it, then import it again. That's a pretty awful experience for themers deciding upon what breakpoints they need for their responsive images.

I've talked to webchick about this in IRC, and probably our best bet is to come up with something similar to the UI in D7 Breakpoints. It's basic, and it isn't flashy, but it's better than exporting and importing config files. I know there were ideas for some cool visual tools for breakpoints, but I think we're a lot less likely to get that in than something simple that allows us to enter the media queries for a breakpoint group, clone a breakpoint group, edit a breakpoint group, delete a breakpoint group, etc.

In regards to sizes, I think it probably makes the most sense to associate sizes with breakpoints in the breakpoints config/UI. Add a width field alongside the MQ declaration for each breakpoint.

In the responsive images module, you'd then want two methods of associating image styles with breakpoints. One where you assign one image style to each breakpoint (and potentially a high-res style with srcset) and thus each source. And another method where you assign a set of image styles to a breakpoint/sizes group that would be associated with one source.

I think sizes is going to be the best way to support responsive images at present, so I'd make that most prominent. One MQ per source is most useful for art direction, and we don't really have a lot built into core for that use case. For most people the viewport sizing use case with sizes is going to be much more important.

The other feature of picture we might want to support is type. That allows you to associate a particular image format that isn't always supported with a source element. So you could have a source element with WebP for example. If the browser supports WebP, it uses the images in WP. If not, it moves on to the next source. That could also be useful for SVG, as you could assign SVG as a type, and then have PNGs on another source as a fallback.

There are a lot of possibilities really. But the first step is to get that empty img element issue fixed. Without that, we don't have valid output to work with Picturefill 2.0.

Re #4, I'm not sure if we need a Breakpoints UI in core (or even at all).

Breakpoints are hard-coded into the theme's CSS (unless a contrib (base)theme provides a fancy gui to configure the responsive layout, but let's leave that out of scope). As Eric Portis points out in his article on srcset and sizes, "the breakpoints in sizes should mirror your page’s breakpoints exactly". In other words, it isn't even desirable to let anyone edit the breakpoints for the picture element independently of the theme's breakpoints. The way I see it, the breakpoints can simply be defined in the theme's CSS, and mirrored in the theme's .info.yml file so Drupal modules can retrieve the information when they need it. Or am I missing something?

I've started to work on this but I ran in to a small issue:
Picturefill 2.0 loops through the sources in the picture element and returns the first source that has a matching media query.
The current output of the picture element (using the breakpoints provided by Bartik) looks something like this:

<picture>
  <source media="(min-width: 0px)" srcset="img_mobile.jpg 1x" width="100" height="67">
  <source media="all and (min-width: 560px) and (max-width: 850px)" srcset="img_narrow.jpg 1x" width="220" height="147">
  <source media="all and (min-width: 851px)" srcset="img_wide.jpg 1x" width="480" height="320">
  <img src="fallback_img.jpg" alt="alt" title="title" >
</picture>

The first media query (min-width: 0px) will always match. So even on large screens, the image for the "mobile" breakpoint is shown. This is because in bartik, the mobile, narrow and wide breakpoints have weights 0, 1 and 2 respectively.

We could just do

array_reverse($variables['breakpoints']);

in theme_responsive_image() (which works), but I'm not sure that's the cleanest way to go.

I'd like some input on this issue.

#4 - yep, agreed, a simple UI is the best win here.

#7 - the current TX is terrible (very slow and tedious), thats the main thing here. Also it's not our place to tell themers/site builders what they should be doing, but provide efficient, flexible tools only. How they use them is up to them.

OK, I don't know I unassigned Jelle_S but I did, how odd, sorry dude, that was not supposed to happen!

No problem ;-)

I just ran into an other issue where the only supported mime types in Picturefill for the type attribute are image/svg+xml and image/webp

I created an issue upstream: https://github.com/scottjehl/picturefill/issues/224

Re #9: I'm not saying slow and tedious is good - if we can improve TX, we should. However creating a gui for something doesn't magically turn slow and tedious into quick and exciting. I've seen several people struggle with the D7 Breakpoints module and its hard-to-grasp, error-prone ui. For good DX/TX/UX, we should make it difficult to make bad decisions and easy to do the right thing. Given that "the breakpoints in sizes should mirror your page’s breakpoints exactly", we should make it easy to make Drupal aware of the breakpoints defined in your CSS. I am concerned that a breakpoint ui would make it too easy to make bad decisions, ie. define breakpoints that differ from the ones in your CSS. The breakpoint settings should only be changed if your CSS changes. IMHO, anyone who is capable of changing the CSS should also be capable of changing a .info.yml file.

There's no magic way to scan CSS, decipher how the layout works, and then create a set of appropriate breakpoints.

Some themes have a layout already. But a lot of people use base themes with no predefined layout. They define the layout after installing the theme, so a UI would be really useful, rather than exporting and importing config files (which is where breakpoints are currently defined for s theme, not info.yml).

One other thing for the to do lost to get the markup matching the spec. We need to remove the setting of width and height on source elements.

To answer my own question in #8 (from http://picture.responsiveimages.org/#the-source-element):

When a source element is a child of a picture element and has a following sibling source element or img element with a srcset attribute specified, it must have at least one of the following:

• A media attribute specified with a value that is not defined to always evaluate to true for all devices (e.g. all or variations thereof). [MEDIAQ]
• A type attribute specified.

[Emphasis mine.]

So according to the spec, a source element with media query that eveluates to true for all devices (e.g. (min-width: 0px) can not precede a source element with a srcset specified. This means that

<picture>
  <source media="(min-width: 0px)" srcset="img_mobile.jpg 1x" width="100" height="67">
  <source media="all and (min-width: 560px) and (max-width: 850px)" srcset="img_narrow.jpg 1x" width="220" height="147">
  <source media="all and (min-width: 851px)" srcset="img_wide.jpg 1x" width="480" height="320">
  <img src="fallback_img.jpg" alt="alt" title="title" >
</picture>

is in fact invalid HTML. While reversing the array would solve that particular problem, I would still like some other opinions on the matter.

Can we move the discussion about a breakpoint UI into a separate issue, please? It's severely out of scope here.

This change here has to happen very fast - before the beta release, because it's a major API change on various layers. Therefore, let's remove any unnecessary distractions, please.

@Jelle_S: Sounds like we need a (PHP) media attribute value parser, in order to sort the elements correctly?

@sun: I don't think that'll work, it's as good as impossible to start comparing(min-width: 700px) to (min-width: 45em). Currently breakpoints have weights, so we could use those. I'm not sure if they're used anywhere else, but this way developers can decide on the order of the breakpoints. If there ever is a UI, we can use tabledrag (or something similar, but that's a discussion for an other issue) to order the breakpoints.

#4: like marcvangend in #7, I doubt we need a Breakpoints UI. At the very least, that's off-topic to this issue. Let's not derail this issue with that.
Having read #14, it might be good to open an issue to propose theme breakpoints to be defined in the theme's *.info.yml file rather than config *.yml files.

#12: wow, wtf! Nice find!

#17/#8: I have only one suggestion for now, but keep in mind I don't know anything about media queries best practices. An alternative would be to to use max-width instead.

@Jelle_S & attiks: Thanks for working on this!

I'll continue working on this tomorrow. Current patch is not yet testable so it's no use uploading it now.

#19: Moving breakpoint declarations into the info.yml file might do the trick. My understanding is that updates to info.yml can be made even after the theme has been installed. If that's the case, then there's your answer. My concern is just that breakpoints can be edited once a theme is installed, as it would be almost impossible to work with responsive images otherwise. This would remove the need to have a UI.

Using max-width is usually not a best practice. Best advice is to use min-width for media queries and start from smallest then work your way up. For that particular example, max-width would probably be okay.

Important that the max-width can be declared in em as well as px. As far as ordering, would be fine to multiply em by 16, as ems in media queries are tied to the default font size of the device/browser, and have nothing to do with font sizes set in stylesheets.

As far as ordering, would be fine to multiply em by 16

I'm not totally clear on what is meant by this, but it would lead to issues if the user has a default browser font size that is not 16px, say 32px instead - then the order could be wrong (if the themer is blending em and px breakpoints, which I personally don't do and it seems rather odd that you would want to do this).

#24: I'm just talking about sorting source elements into the proper markup order. It's definitely true that a default browser might have 32px as a font size, but that wouldn't matter as far as markup, as the markup would be constructed without knowledge of any individual's browser settings.

I'm just saying that if you're trying to sort source elements based on a width setting in the media query, and some MQs have px units and others have em, then multiplying the em amount by 16 is a decent proxy to match up with the px units. It's not a perfect proxy for the reasons you cited.

For the record, because of that, it's a really bad practice to have some source elements that use px in MQ width declarations and others with em, as the order won't necessarily be correct if the the default font size isn't 16 (or whatever other assumption was used to set up the order of the source elements). So I'd look at comparing px and em as an edge case if we sort source elements. It's not something that people should do, but if they do, this is how we could handle it.

Changes made in this patch:

• Replaced picturefill.js with version 2.0
• Sources now support the "type", "sizes", "media" and "srcset" attributes
• In PictureMappings a choice between ImageStyles and the sizes attribute can be made
• Weight of breakpoints changed to output them in the correct order.
• BreakpointGroup::getBreakpoints changed so breakpoints are sorted.
• Added an empty default breakpoint to the breakpoint module so other modules can use it.
• Responsive Image module now uses the empty breakpoint as extra option to use in its mappings (mainly for use in combination with the "sizes" attribute).
• This patch incorporates the changes from #2262863: Add srcset to template_preprocess_image so Responsive Image can use srcset for the fallback image (preventing unnecessary image preloads)

Oh, and the tests were updated as well ;-)

Wow, awesome work!

0 bytes. Something went wrong with the upload.

Go testbot go!

New patch. In stead of hard-coding the empty breakpoint to all mappings, we allow modules or themes to choose whether or not an empty breakpoint makes sense in the provided breakpoint group.

Took a quick look at the patch. At first glance this looks great. Appears to have support for sizes and type in addition to srcset.

We'll eventually need to convert to Twig, but the model we've been using for that is to have no markup changes in Twig conversions, so we can tackle Twig after this gets in.

I'll try to apply the patch tonight and look at the resulting code to see if I have additional questions. Thanks for the great work on this!

1. +++ b/core/assets/vendor/picturefill/picturefill.js
   @@ -1,126 +1,506 @@
   +/*! Picturefill - Responsive Images that work today.
   

   Let's not forget to update the library definition in core.libraries.yml for the new version accordingly.

2. +++ b/core/modules/breakpoint/config/install/breakpoint.breakpoint.module.breakpoint._none.yml
   @@ -0,0 +1,11 @@
   +id: module.breakpoint._none
   ...
   +label: None (empty media query)
   ...
   +weight: 99
   +multipliers:
   +  1x: 1x
   
   +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/Breakpoint.php
   @@ -178,7 +178,11 @@ public function isValid() {
   +    // Skip the empty media query provided by the breakpoint module.
   +    if ($this->id != 'module.breakpoint._none') {
   
   +++ b/core/themes/bartik/config/install/breakpoint.breakpoint_group.theme.bartik.bartik.yml
   @@ -5,6 +5,7 @@ breakpoint_ids:
   +  - module.breakpoint._none
   

   This appears to be a fallback default value.

   Fallback values are not configuration. Unless you expect users to be able to edit the fallback configuration to no longer be none/empty — because the whole point of configuration is to be editable.

   A simple replacement for that would be to create a new entity (in-memory) ad-hoc when collecting/loading available breakpoints.

   Even better would be to avoid the situation altogether — this definition essentially says: If nothing matched, or if there are no breakpoints, then fall back to no breakpoint. That case should be handled (much) earlier, before even starting to process any breakpoints, because it doesn't make sense to process breakpoints if there are none (or no matching).

3. +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/Breakpoint.php
   @@ -178,7 +178,11 @@ public function isValid() {
   +      return $this::isValidMediaQuery($this->mediaQuery);
   

   static::

4. +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/Breakpoint.php
   @@ -178,7 +178,11 @@ public function isValid() {
   +    return TRUE;
   

   TRUE? Why TRUE?

5. +++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/DesaturateImageEffect.php
   @@ -30,6 +30,12 @@ public function transformDimensions(array &$dimensions) {
   +  public function transformMimeType(&$mime_type) {
   

   It's not clear why all image effects suddenly have to implement this method…?

6. +++ b/core/modules/responsive_image/lib/Drupal/responsive_image/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
   @@ -142,21 +143,24 @@ public function viewElements(FieldItemListInterface $items) {
   +              if (!$responsive_image_mapping::isEmptyMappingDefinition($mapping_definition)) {
   

   This should be a method on the mapping definition instead of a static method on the image mapper...?

7. +++ b/core/modules/responsive_image/lib/Drupal/responsive_image/ResponsiveImageMappingForm.php
   @@ -73,16 +74,58 @@ public function form(array $form, array &$form_state) {
   +          '#options' => array(
   +            '' => t('Do not use this breakpoint'),
   

   An empty string is not a good default value. It's better to use an explicit 'none'.

New patch based on #46:

1. Updated core.libraries.yml
2. Breakpoint with empty media query is no longer provided by the Breakpoint module. In stead, the empty breakpoint can just be defined by the module/theme itself.
3. Breakpoint::isValidMediaQuery() now allows empty media queries
4. Bartik has an empty breakpoint defined.
5. $this::isValidMediaQuery($this->mediaQuery); => static::isValidMediaQuery($this->mediaQuery);
6. Default mapping value was an empty string, changed to '_none'

RE: #46-6:
$mapping_definition is an array, not an object (see changes in /core/modules/responsive_image/config/schema/responsive_image.schema.yml):

--- a/core/modules/responsive_image/config/schema/responsive_image.schema.yml
+++ b/core/modules/responsive_image/config/schema/responsive_image.schema.yml
@@ -26,8 +26,24 @@ responsive_image.mappings.*:
                 - type: sequence
                   label: 'Machine name'
                   sequence:
-                    - type: string
-                      label: 'Image style'
+                    - type: mapping
+                      label: 'Mapping definition'
+                      mapping:
+                        mapping_type:
+                          type: string
+                          label: 'Type'
+                        image_style:
+                          type: string
+                          label: 'Image style'
+                        sizes:
+                          type: string
+                          label: 'Sizes'
+                        sizes_image_styles:
+                          type: sequence
+                          label: 'Image styles'
+                          sequence:
+                            - type: string
+                              label: 'Image style'

Note on the interdiff:
Except for the first 4 lines, all changes in isValidMediaQuery is basically just an "if" that is removed and the indentation that has changed because of it.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.php
@@ -197,6 +197,7 @@ public function getBreakpoints() {
+    uasort($this->breakpoints, array('\Drupal\breakpoint\Entity\Breakpoint', 'sort'));

Just making sure I understand correctly: I think this calls ConfigEntityBase::sort, so it's only sorting on weights and labels? Following #27 etc, there is no attempt to sort mediaqueries, right?

@marcvangend: Indeed, just weight and labels. We figured the one creating the media queries would know best which ones should come first. (S)he can decide the weight each breakpoint should get.

Thanks. I had seen #27 and I completely agree - just wanted to make sure this patch didn't attempt to sort MQ's after all.

New patch because of changes in #2262863-19: Add srcset to template_preprocess_image

New patch because of changes in #2262863-23: Add srcset to template_preprocess_image

For those interested, I set up a new issue for discussing a basic UI for breakpoints configuration that we could get in place for Drupal 8.0 (#2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level), ideally based on the UI from the Drupal 7 Breakpoints module. The discussion in (#1701112: Advanced responsive images UI/UX for breakpoints) is quite old and starts out with discussion of a visual configuration for the Breakpoints module that was developed as part of the Responsive Layout Builder, which won't be available for Drupal 8.0.

For what it's worth, I looked into having settings for breakpoints in a theme's .info.yml file. That won't work with the configuration system. We need a UI.

Doing my own picturefill integration for a client site (non-imagecache based resizing flow, thus picture module's theme_picture() doesn't fly), I bumped into the following subtelty :

The picture spec (and thus the picturefill 2.0 script) says values within a 'sizes' attributes must be :

<source-size> = <media-condition> <source-size-value>

Thus, a full fledged <media-query> is not allowed, only a <media-condition> -thus, excluding a possible <media-type> part.
(see the grammar for those:

<media-query> = <media-condition>?
             | [ not | only ]? <media-type> [ and <media-condition> ]?

)

In human speak :
- "(max-width: 60em) 300px" is allowed in the 'sizes' attributes
- but "only screen and (max-width: 60em) 300px" isn't recognized (and is just ignored in with the current picturefill 2.0)

Meaning, we cannot put the breakpoints defined by themes / modules "as is" in the 'sizes' attributes : the breakpoints specify media-queries (thus possibly with a media-type part), and we need media-conditions there.

It's easy with a simple regexp to remove the "media-type" if present and just keep the "media-condition" part of a media-query. Just pointing that it has to be done manually.

Disclaimer : As I said above, my use case excludes the current imagecache-based theme_picture(), so I didn't delve into the (somewhat complex) field formatter code and check how the "sizes" actually get populated. If they in fact don't use the theme-defined "breakpoints" and use other stuff instead that is guaranteed to be a kosher "media-condition", well, sorry for the noise, just ignore me :-)

@attiks : well, picture lets you do
sizes="(media cond.) (size), (media cond.) (size),... , (fallback size)"

You're saying that the field formatter only ever outputs
sizes="(fallback size)" ?

In that case, right, no need to worry about #57. Sorry for the noise.

@attiks : Ah right, sorry, I misread your explanation. Thanks !

I still think that the media conditions, in most cases, will be inherently tied to a theme's layout, so I wonder if there is value in defining them in the theme's info file, rather than in the UI. Media conditions not tied to the layout are likely to be a fallback.

The thing to keep in mind is that sizes will probably be the primary way that responsive images are used. If there was a great system for setting up art direction cropping for different source element breakpoints, maybe that wouldn't be the case. But right now there isn't a great solution for that, so what people are going to want to do is just provide different sized images at different breakpoints, and for that sizes makes the most sense.

Since you really only need one source element to use sizes and srcset, and you don't really need to define a breakpoint, that means for the vast majority of the site theme out there, the themes' info files will just have an empty breakpoint defined in it.

I'm not clear on how the picture mappings are set up—if you can have multiple mappings per breakpoint group or just one mapping per breakpoint group. If the latter, that means each theme would need to define multiple breakpoint groups, each with an empty breakpoint, in order to have multiple kinds of responsive images on a site (banner image, sidebar image, product image, etc.) Even if those can all be based on one single breakpoint group with an empty breakpoint definition, it seems inefficient.

I think it makes a lot more sense to store the sizes definitions in a theme's info file, closer to where the layout settings are defined. Maybe that makes things a little more complex, as you could define either a breakpoint with a full media query or a sizes definition with just a series media conditions. I think it would make sense to define those on a separate lines in the info file, then have them bundled up into a comma-separated list.

If it's not possible to do that, then it would be good to at least remove the requirement of an empty breakpoint definition in a theme file when there's only one source element with no media query.

I'm glad to continue the discussion in that issue. I had tried to do so, but hadn't gotten a response from you there about this. So just replying here, where you discussed it. We're all busy with DrupalCon week I'm sure, so no worries. Just want to figure out the best way to do this.

New patch with the IE9 workaround.

I did some manual testing to make sure the merge with the PSR-4 commit didn't break anything.

Do we want to get 2.1?

I'll comment in the other issue, but I'm going to let the sizes issue go. We need to get this in so we can move forward with triage and cleanup of other responsive image issues.

What do we need to do to get this committed? Who needs to review it? Any additional tests needed? Profiling?

So this is postponed on #2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level. Let's mark it as such.

Version 2.1 of Picturefill is out. We should update to the latest version in this patch.

https://github.com/scottjehl/picturefill/tree/2.1.0

Maybe jumping the gun a bit, but here's a patch created with the patch from #2271529-97: Move breakpoint settings to theme and module *.breakpoints.yml files at root level already applied.
This way this issue can move forward a bit faster, once #2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level gets committed.

So once the other patch is committed, we can put this issue on 'Needs Review' and see what the testbot has to say about it.

Picturefill 2.1 is the current version and what is in the current patch. Updating the issue information accordingly.

First things first. Let's get this rerolled now that #2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level is in.

Rerolled patch. All relevant tests were green on my local machine. Let's see what the testbot thinks!

Forgot to update a unit test.

Ok let's try that again...

This shoud fix the 1 fail from #83

Partial review. Lots of code style issues.

1. +++ b/core/core.libraries.yml
   @@ -776,9 +776,7 @@ normalize:
        url: https://github.com/scottjehl/picturefill/blob/master/LICENSE
   

   Should be updated to https://github.com/scottjehl/picturefill/blob/2.1.0/LICENSE.

2. +++ b/core/includes/theme.inc
   @@ -1077,9 +1077,31 @@ function template_preprocess_links(&$variables) {
   +  if (!empty($variables['srcset'])) {
   

   Perhaps add a comment saying something like "Generate a srcset attribute conforming to the spec at [URL]."?

3. +++ b/core/includes/theme.inc
   @@ -1077,9 +1077,31 @@ function template_preprocess_links(&$variables) {
   +      // URI is mandatory.
   +      if (isset($srcset['uri']) && !empty($srcset['uri'])) {
   

   If it's mandatory, then why check if it exists?

   Let's remove this if-test and fail explicitly.

4. +++ b/core/modules/breakpoint/src/Tests/BreakpointDiscoveryTest.php
   @@ -33,54 +33,54 @@ protected function setUp() {
   +        'mediaQuery' => 'only screen and (min-width: 3456px)',
   

   This is … a very peculiar number?

5. +++ b/core/modules/image/src/ImageEffectInterface.php
   @@ -44,6 +44,14 @@ public function applyEffect(ImageInterface $image);
   +   * Determines the mime type of the styled image.
   

   This is an image effect, not an image style.

6. +++ b/core/modules/image/src/ImageEffectInterface.php
   @@ -44,6 +44,14 @@ public function applyEffect(ImageInterface $image);
   +   *   Mime type to be modified.
   
   +++ b/core/modules/image/src/ImageStyleInterface.php
   @@ -131,6 +131,14 @@ public function createDerivative($original_uri, $derivative_uri);
   +   * Determines the mime type of the styled image.
   ...
   +   *   Mime type to be modified.
   

   s/(m|M)ime/MIME/

   s/styled image/image style/

7. +++ b/core/modules/image/src/Tests/ImageStyleTest.php
   @@ -0,0 +1,115 @@
   +use \Drupal\image\Entity\ImageStyle;
   

   Please remove the leading backslash.

8. +++ b/core/modules/image/src/Tests/ImageStyleTest.php
   @@ -0,0 +1,115 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function getInfo() {
   +    return array(
   +      'description' => '',
   +      'name' => '\Drupal\image\Entity\ImageStyle unit test',
   +      'group' => 'Image',
   +    );
   +  }
   

   This method is no longer necessary. All info lives in the test class's annotation. :)

9. +++ b/core/modules/image/src/Tests/ImageStyleTest.php
   @@ -0,0 +1,115 @@
   +    $this->entityType->expects($this->any())
   +                     ->method('getProvider')
   +                     ->will($this->returnValue($this->provider));
   +    $this->entityManager = $this->getMock('\Drupal\Core\Entity\EntityManagerInterface');
   +    $this->entityManager->expects($this->any())
   +                        ->method('getDefinition')
   +                        ->with($this->entityTypeId)
   +                        ->will($this->returnValue($this->entityType));
   +    $this->effectManager = $this->getMockBuilder('\Drupal\image\ImageEffectManager')
   +        ->disableOriginalConstructor()
   +        ->getMock();
   

   This is not how we indent chained method calls.

I changed 4 & 5, but:
4: That number is only used for testing purposes, so I don't think it really matters. However, I do agree it looks a bit cleaner with a realistic number.

5: That comment was basically copy-paste from ImageEffectInterface::transformDimensions(), so maybe we should consider changing that one as well (in a separate issue)?

Forgot a few s/(m|M)ime/MIME/

Here's the final round of high-level feedback. The next step will be for me to actually dive into the code.

I will actually have to step through this with a debugger to start making sense of the code: "why is X happening?", "why is Y happening in this particular way?", and so on. Perhaps it's necessary due to inherent complexity. Perhaps it can be simplified. I strongly suspect this patch could benefit from more comments, particularly in the pieces of code where there's a lot of nesting and looping going on. If you could add such comments, then this patch will definitely get to RTBC faster :)

1. +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -307,6 +307,15 @@ public function transformDimensions(array &$dimensions) {
   +  public function transformMimeType(&$mime_type) {
   

   Why do we want this new API method to modify a string by reference, rather than allowing the method to *return* a new MIME type?

   It's okay if there's a good reason, but in general Drupal avoids using references in APIs AFAIK.

2. +++ b/core/modules/image/src/ImageEffectInterface.php
   @@ -44,6 +44,14 @@ public function applyEffect(ImageInterface $image);
   +   * Determines the MIME type of the image effect.
   ...
   +   *   MIME type to be modified.
   ...
   +  public function transformMimeType(&$mime_type);
   
   +++ b/core/modules/image/src/ImageStyleInterface.php
   @@ -131,6 +131,14 @@ public function createDerivative($original_uri, $derivative_uri);
   +   * Determines the MIME type of the image style.
   ...
   +   *   MIME type to be modified.
   ...
   +  public function transformMimeType(&$mime_type);
   

   "determine" vs. "modify" vs. "transform"

   I think we should make this consistent.

3. +++ b/core/modules/image/src/Tests/ImageStyleTest.php
   @@ -0,0 +1,104 @@
   +    $container = new ContainerBuilder();
   +    \Drupal::setContainer($container);
   

   A unit test should never be mocking the container! What's going on here?

4. +++ b/core/modules/image/src/Tests/ImageStyleTest.php
   @@ -0,0 +1,104 @@
   +  }
   +}
   

   Missing newline. We always put a newline between the beginning of a class and the first method, and the last method's closing brace and the class's closing brace.

5. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -196,68 +196,78 @@ function theme_responsive_image($variables) {
   +      // Only add MIME type if it is unique.
   +      $derivative_mime_types = array_unique($derivative_mime_types);
   ...
   +        '#mime_type' => count($derivative_mime_types) == 1 ? reset($derivative_mime_types) : '',
   

   This is confusing. I think this can be simplified to:

   // Only set a MIME type if all derivative images have the same MIME type.
   '#mime_type' => count(array_unique($derivative_mime_types)) == 1 ? $derivative_mime_types[0] : '',
   

   No need

6. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -196,68 +196,78 @@ function theme_responsive_image($variables) {
   +    $output[] = '<!--[if IE 9]><video style="display: none;"><![endif]-->';
   +    $output = array_merge($output, array_map('drupal_render', $sources));
   +    $output[] = '<!--[if IE 9]></video><![endif]-->';
   

   Eh? :P

   This could use a comment! :)

7. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -196,68 +196,78 @@ function theme_responsive_image($variables) {
   +    $output[] = drupal_render($fallback_image);
   

   All calls to drupal_render($x) in the theme layer should set $is_recursive = TRUE, i.e. drupal_render($x, TRUE).

8. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -309,27 +316,41 @@ function theme_responsive_image_source($variables) {
   +    entity_load('image_style', $image_style_name)->transformDimensions($dimensions);
   ...
   +  entity_load('image_style', $image_style_name)->transformMimeType($mime_type);
   

   All of these should become ImageStyle::load(…).

9. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -309,27 +316,41 @@ function theme_responsive_image_source($variables) {
   +    return 'image/gif';
   

   Why would we want the empty image to be a GIF?

   A PNG has a lower smallest possible filesize.

10. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
    @@ -88,18 +88,19 @@ public function __construct(array $values, $entity_type_id = 'responsive_image_m
    +  public function addMapping($breakpoint_id, $multiplier, $mapping_definition) {
    

    Can't this be typehinted to array?

11. +++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
    @@ -142,20 +142,27 @@ public function viewElements(FieldItemListInterface $items) {
    +      $fallback_image_style = end($image_styles_to_load);
    

    Why the last one? Can use a comment.

12. +++ b/core/modules/responsive_image/src/ResponsiveImageMappingForm.php
    @@ -105,11 +105,53 @@ public function form(array $form, FormStateInterface $form_state) {
    +          '#title' => t('Type'),
    ...
    +            '_none' => t('Do not use this breakpoint'),
    ...
    +            'sizes' => t('Use the sizes attribute'),
    

    Shouldn't all of this be $this->t(…)?

13. +++ b/core/modules/responsive_image/src/ResponsiveImageMappingInterface.php
    @@ -62,17 +71,35 @@ public function setBreakpointGroup($breakpoint_group);
    +   *   The mapping definition. Null if the mapping does not exist.
    

    s/Null/NULL/

14. +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageAdminUITest.php
    @@ -69,30 +69,71 @@ public function testResponsiveImageAdmin() {
    +    $this->assertFieldByName('keyed_mappings[responsive_image_test_module.mobile][1x][mapping_type]', '');
    +    $this->assertFieldByName('keyed_mappings[responsive_image_test_module.mobile][2x][mapping_type]', '');
    +    $this->assertFieldByName('keyed_mappings[responsive_image_test_module.narrow][1x][mapping_type]', '');
    +    $this->assertFieldByName('keyed_mappings[responsive_image_test_module.narrow][2x][mapping_type]', '');
    +    $this->assertFieldByName('keyed_mappings[responsive_image_test_module.wide][1x][mapping_type]', '');
    +    $this->assertFieldByName('keyed_mappings[responsive_image_test_module.wide][2x][mapping_type]', '');
    

    All of these assertions, and the many dozens below, would be a lot clearer if it looked like this:

    $cases = [
      ['mobile', '1x'],
      ['mobile', '2x'],
      ['narrow', '1x'],
      …
    ];
    foreach ($cases as $case) {
      $this->assertFieldByName('keyed_mappings['responsive_image_test_module.' . $case[0] . '][' . $case[1] . '][mapping_type]', '');
    }
    

    Then PHP does the parsing rather than us humans :)

15. +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
    @@ -146,35 +164,50 @@ public function _testResponsiveImageFieldFormatters($scheme) {
    +    // Create a derivative so at least one MIME-type will be known.
    

    s/MIME-type/MIME type/

1. Ok :)
6. Yes, that much is obvious, but it's not clear why it's necessary. A link to an MSDN article for example would be helpful.
8. Since a month or so, we have <EntityType>::load(). All patches that go in now should use that.
9. I don't know, that's just the general rule. Feel free to ignore that, but then we should at least document why we chose this GIF image. For future code readers, it will otherwise seem like an arbitrary choice.

@Wim

3. I just looked in to this again. Apparently it is needed because of this code in ImageStyle.php:

/**
 * {@inheritdoc}
 */
 public function getEffects() {
   if (!$this->effectsBag) {
     $this->effectsBag = new ImageEffectBag(\Drupal::service('plugin.manager.image.effect'), $this->effects);
     $this->effectsBag->sort();
   }
   return $this->effectsBag;
 }

Not sure how else to solve it. Do you have any ideas?

Change ImageStyle to have a getPluginManager() method, then mock ImageStyle in the test, overriding getPluginManager() to return a mocked plugin manager that contains the needed effect plugins.

9. I looked in to this a bit more:

The smallest PNG possible would be 67 bytes: http://garethrees.org/2007/11/14/pngcrush/

The smallest GIF possible would be 26 bytes: http://probablyprogramming.com/2009/03/15/the-tiniest-gif-ever
But then the color table is removed, which makes it display in different colors in different browsers.
So the smallest stable and transparent gif possible would be 43 bytes.

Our current solution seems to come from http://css-tricks.com/snippets/html/base64-encode-of-1x1px-transparent-gif/
Chrome even claims it is 42bytes (not sure why that is).

We could also use data:image/gif;base64,R0lGODlhAQABAIAAAP///wAAACH5BAEAAAAALAAAAAABAAEAAAICRAEAOw== (source: http://proger.i-forge.net/The_smallest_transparent_pixel/eBQ) which Chrome confirms is 43 bytes.

#89.2: This docblock is as good as a copy-paste from transformDimensions:

 /**
   * Determines the dimensions of the styled image.
   *
   * @param array $dimensions
   *   Dimensions to be modified - an array with components width and height, in
   *   pixels.
   */
  public function transformDimensions(array &$dimensions);

Shouldn't we go for consistency here?

#94: great — now please just document your research in code comments by linking to that URL that explains the smallest possible GIF :)

(I confused the general WPO rule which indeed says "never use GIFs, always use PNGs, because they will always be smaller" with the search for the smallest possible image —which apparently is a GIF.)

#95: ugh, ok. Then indeed let's be consistent with what exists, and fix that in another issue.

Ok, I'm on it :-)

New patch with remarks from #89 addressed.
I also added comments where I thought they where helpful/necessary.

Wonderful, thank you! I'll try to review this again tomorrow.

I think all of the sizes stuff should be removed from this patch. That's a feature addition. We can update to Picturefill 2.1 without using sizes. Therefore it should be split off to a separate issue. That will make this patch much easier to review.
(And complexity aside, the UI allows me to use "sizes" for one breakpoint and "image style" for another. Can I really mix as I want? Worse, when I select "sizes", I still have to choose an image style anyway, and I have to repeat most of the media query that's actually already in the breakpoint. In other words: the current UI is *extremely* confusing — I know something about this subject, but I literally have no idea how to use this UI.)

Similar for the MIME type transformations: that's at least not cluttering the affected code, but why is that in this patch? Why wasn't it there already? Can't it be done in a follow-up? If you'd split the MIME type transformations into a separate patch, we could get that committed very fast, very easily. Making it part of this patch only complicates this patch.

I think applying the two above … breakpoints for the patch (see what I did there/ :P /me runs away from his lame joke) to this patch will make it an order of magnitude easier to review.

+++ b/core/modules/responsive_image/config/schema/responsive_image.schema.yml
@@ -20,6 +20,9 @@ responsive_image.mappings.*:
+            mapping_type:
+              type: string
+              label: 'Type'

What is a mapping type?

Besides, it's very confusing to see

mapping:
  mapping_type:
    …

in YAML — mapping is a keyword, so mapping_type also feels like a piece of configuration for that keyword.
What about image_mapping_type?

Besides that, the label ("Type") is similarly vague and hence confusing.

I have mixed feelings. I would really like to be able to use sizes with responsive images, and this does technically allow me to do that. However, I agree with Wim that the interface for sizes is pretty confusing.

I went into detail about this in previous comments, but I'll reiterate here. I believe that for the vast majority of use cases, the combination of sizes and srcset on an img element will work... and that img will not necessarily be within a picture element.

Using the picture element with multiple sources primarily makes sense when you are doing art direction: when there are different crops for an image at different breakpoints. However, there's no really easy way to provide different crops at different breakpoints. You could do automatic cropping with image styles, but I would bet that in most cases you would want to do multiple cropping.

The way it's been explained to me for how sizes would work with this patch is that you would have a breakpoint with min-width 0, then you'd manually write out your breakpoints in the sizes field, and attach all of the image styles for srcset. That seems very confusing to me, and I have been closely following responsive images for years now.

Manually writing out breakpoints for use in sizes also defeats the purpose of having a breakpoint module in the first place. Why allow themers to create a set of breakpoints in a YML file in theory so they can be used in Drupal, and then force themers to completely rewrite every single breakpoint they want to use in a text field?

There has to be a better way to make this work.

That said, I am nervous that if we ditch sizes from this, and we can't agree on a sensible user interface and way to configure sizes, then there will be no way to use sizes in Drupal 8. Instead we'll have the terribly confusing method of assigning a single breakpoint and image style to each source element. I haven't even checked if you can use srcset with that method in order to provide retina images.

So I'm inclined to support breaking off sizes, but I'd feel more comfortable suggesting that if we had agreement that yes, we will try to get sizes into D8 along with some idea of how a UI would work.

I believe that for the vast majority of use cases, the combination of sizes and srcset on an img element will work... and that img will not necessarily be within a picture element.

I think this is correct also. But at the same time: in Drupal, there's no need to prefer a simpler/the simplest approach over a more advanced/the most complex approach, because Drupal will generate the markup. We just have to make sure that our abstractions are solid and that the UI is simple. Then it's fine to always use <picture> under the hood.

Using the picture element with multiple sources primarily makes sense when you are doing art direction: when there are different crops for an image at different breakpoints. However, there's no really easy way to provide different crops at different breakpoints. You could do automatic cropping with image styles, but I would bet that in most cases you would want to do multiple cropping.

But with https://www.drupal.org/project/imagefield_focus, https://www.drupal.org/project/image_focus, https://drupal.org/project/focal_point and likely better ones in D8, it will actually be very possible that there's at least "automated art direction", and partially manual art direction by means of allowing the author/editor to define a focus point on the image, after which image styles will handle the cropping, or even fully manual art direction.

Again: as long as we don't make the UI more complex, I don't think there's a reason why we should prefer sizes over picture.

[…] That seems very confusing to me, and I have been closely following responsive images for years now.

Manually writing out breakpoints for use in sizes also defeats the purpose of having a breakpoint module in the first place. Why allow themers to create a set of breakpoints in a YML file in theory so they can be used in Drupal, and then force themers to completely rewrite every single breakpoint they want to use in a text field?

+1!

That said, I am nervous that if we ditch sizes from this […]

It sounds like this implies that <picture> is worse than sizes in some way besides markup… therefore: the above being said, I don't remember if there are downsides to <picture>. If sizes or srcset is "better" in some way, but is only worse in that it doesn't support art direction, then I'm fine with going with one of the others. <picture> support would simply be deferred to a Art-directed responsive image formatter D8 contrib module, probably along with a widget that makes art direction easier.

Indeed, you can do "art direction with several source tags in a picture tag" by applying different image styles to the same source image - granted, that's a limited kind of art direction, but that's still very valid.

Also, since the "sizes" are supposed to be hints for the browser about what actual dimensions the image will have in the various layout variants once the CSS is applied, it does seem to make sense that the breakpoints used in "sizes" are the ones defined by the theme and (if the theme does things right) used in the CSS ?
(although, providing those dimensions requires some intimate knowledge of the theme's CSS, and is not easy to figure out for a site admin...)

It takes a few minutes to read, but this is an excellent, excellent overview of the wide variety of use cases for responsive images.

https://dev.opera.com/articles/responsive-images/

The short version is that yes, there are use cases where it makes sense to use sizes + srcset on multiple sources.

If there was a better UI for sizes selection than just a text field, that would be a big step forward.

For sizes, I think the UI needs to allow people to select a breakpoint defined in the Breakpoint module and then have a text field to select the actual size associated with that breakpoint. Or a numeric text field plus a select field for the type of unit. You'd also need the option to have a size like 100vw without a breakpoint associated with it.

It may be very complicated to have this sizes mapping on the same page as the responsive image mapping source selection page.

In an ideal world, I'd also like for people to be able just use a single sizes/srcset with a responsive image mapping without needing to set up a breakpoint of min-width: 0 first, which just seems overly complicated.

Forgot about this issue: https://www.drupal.org/node/2260061#comment-8831579

Breakpoint module allows breakpoints to be defined as Media Type + Media Condition. Sizes only uses Media Conditions. That means we couldn't use Breakpoint-defined breakpoints in a sizes mapping. Unless we somehow changed how Breakpoint defines breakpoints to separate out the media type and media condition. But for complicated media queries that chain together multiple media type+condition declarations with OR or AND, that would be very complicated.

Ugh.

I am not suggesting that we define the sizes attribute within a *.breakpoints.yml file.

breakpoints.yml contains a series of named media queries.

A media query can include both a media type and a media condition.

screen and (min-width: 700px)

Media type: screen
Media condition: (min-width: 700px)

In addition, a full media query can chain together multiple combinations of media type + media condition

(min-width: 700px), handheld and (orientation: landscape)

Really good docs on this here: https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Media_queries

So the trouble is that breakpoints.yml can contain these complex media queries. However, the sizes attribute can only contain the media conditions portions of a media query, much less a complex media query that changes together multiple types and conditions.

sizes="(min-width: 640px) 60vw, 100vw"

That says that when the viewport is at least 640px wide, the image should fit into a space that is 60% the total width of the viewport. Below 640px, the image takes up the fill width of the viewport.

srcset="opera-200.jpg 200w,
				opera-400.jpg 400w,
				opera-800.jpg 800w,
				opera-1200.jpg 1200w,
				opera-1600.jpg 1600w,
				opera-2000.jpg 2000w"

srcset then provides a list of image files at various widths (200px, 400px, 800px, 1200px, 1600px, 2000px in this case), and the browser can determine which source files best fit within the space defined by the sizes attribute.

So sizes + srcset means the browser does a whole lot of math rather than the person setting things up. That is great.

In terms of Drupal, srcset means selecting a series of image styles.

For sizes, it would be nice to be able to enter a series of sizes an image is supposed to fit within, each of which might be associated with a particular media condition (in most situations, all but one would have a media condition).

While it might in theory be nice to grab those media conditions from breakpoints.yml, a full media query is not the same thing as a media condition, so that wouldn't work.

If you wanted to avoid just having a text field where sizes are entered based on a code-based configuration, you'd almost need an entirely new module/plugin for a sizes.yml definition.

Obviously, that is way, way out of scope for this issue.

The combination of sizes + srcset is a very powerful and easy way to make responsive images work for most use cases. So I share Peter's concerns that if we split off sizes, it never gets solved, and then D8 doesn't have sizes support.

Maybe it's best to just keep proceeding as is? I don't know.

New patch without UI.

Patches from #2262863: Add srcset to template_preprocess_image and #2330899: Allow image effects to change the MIME type + extension, add a "convert" image effect included (I know they'll be committed separately but this way we can keep this issue moving forward as well.

Needs reroll due to #2260121: PHPUnit Tests namespace of modules is ambiguous with regular runtime namespace (+ Simpletest tests).

Needs another reroll.

This is just a straight reroll for testing purposes.

#2262863: Add srcset to template_preprocess_image, #2330899: Allow image effects to change the MIME type + extension, add a "convert" image effect and #2333395: Add sizes to template_preprocess_image should get committed first, then we can start working seriously on this patch again.

Rerolled patch after #2262863: Add srcset to template_preprocess_image and #2333395: Add sizes to template_preprocess_image got committed. Patch assumes #2330899: Allow image effects to change the MIME type + extension, add a "convert" image effect will get committed as well and should work once it is (hence the do-not-test) .

Since srcset is now an array of arrays, the array_unique call didn't work as expected anymore.

In the fallback image the width descriptor was added to the srcset, which would mean safari wouldn't support it (safari does not support width descriptors, only multipliers). Since the width descriptor in the fallback image was useless anyway, it is removed now and the test was adjusted accordingly.
In the formatter the link to content setting didn't work. Fixed it and added tests.

Ok, hopefully this helps move this along.

Just retested. Needs reroll.

Rerolled patch.

Apparently it needs more than just a reroll.

Let's try this again.

So sad there hasn't been more progress by the browsers http://caniuse.com/#search=picture

Still, hopefully this polyfill can work.

What's the best way to test this?

I applied the patch nicely on SimplyTest.me, uploaded an image, but don't see how it is different:
http://s21e7ee391110709.s3.simplytest.me/node/1

I suspect I just don't know what I'm looking to verify.

I'm actually incredibly encouraged by browser support.

Keep in mind, a year ago, there was no viable spec for responsive images, and it looked like all was lost. The revised spec was created last December into early January. For us to get from that point to here where Chrome and Opera supports the responsive image spec, and Firefox is close behind, that's incredibly awesome. Since the Chrome codebase is so similar to Safari, I'd imagine we may well see Safari support before too long. IE might be a bit further behind, but I don't think they're opposed to implementing this.

This is a spec created based on the needs of web developers and was led by web developers, so to see this become a reality in so short a time is very, very good for the future of the web.

And for the browsers where picture isn't supported natively yet, that's what this polyfill is for. This is looking really close now. Thanks so much to everyone for all their work!

I'm actually incredibly encouraged by browser support.

Likewise!

The issue summary does not suggest we're moving away from <picture>, but the patch does. Please update the issue summary (this will be necessary for a core committer to review this also/anyway).

I appreciate all the hard work that's been done here, and I know this is an incredibly difficult issue — because no clear best practices exist yet "in the wild", there are multiple ways of doing responsive images, and so we're all trying our best to find the best solution possible.
I'm reviewing this with fresh eyes since I haven't looked at this in a long, long time. I'm reviewing it WITHOUT reading the entire backlog again, precisely so that I'll look at it without assumptions. Reading through the code SHOULD NOT be sufficient to understand all there is to know about responsive images, but it SHOULD be sufficient to understand WHY it is working the way it is. Otherwise, we have no hope of other people understanding this besides the few (heroes!) who worked on this issue.

So please know that if there's any harshness, it's directed at the code, and directed at the general frustrating insanity that is responsive images, it's not directed at you :)

Also: this is just a first, relatively high-level review. I will try to review this patch every few days from now on.

1. +++ b/core/modules/responsive_image/config/schema/responsive_image.schema.yml
   @@ -20,6 +20,9 @@ responsive_image.mappings.*:
   +            image_mapping_type:
   +              type: string
   +              label: 'Type'
   

   Label is too vague. Didn't find a clear explanation elsewhere either.

2. +++ b/core/modules/responsive_image/config/schema/responsive_image.schema.yml
   @@ -29,6 +32,15 @@ responsive_image.mappings.*:
   +            sizes:
   +              type: string
   +              label: 'Sizes'
   

   Again too vague. Again no explanation elsewhere.

3. +++ b/core/modules/responsive_image/config/schema/responsive_image.schema.yml
   @@ -29,6 +32,15 @@ responsive_image.mappings.*:
   +            sizes_image_styles:
   +              type: sequence
   +              label: 'Image styles'
   

   These are about image styles when using [sizes]?

4. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -189,68 +190,90 @@ function theme_responsive_image($variables) {
   +        switch ($mapping_definition['image_mapping_type']) {
   

   Can we have different image mapping types depending the breakpoint?

   Perhaps a nonsensical question, but it just feels weird and scary that in a nested for-loop we have a switch-statement… while we're rendering the markup for responsive images, all of which I'd expect to use the same image mapping type within one <picture> element, no?

5. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -189,68 +190,90 @@ function theme_responsive_image($variables) {
   +          // Create a source tag with the sizes attribute.
   

   s/source//
   s/sizes/'sizes'/

6. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -189,68 +190,90 @@ function theme_responsive_image($variables) {
   +              // it. Because of this the image styles for all multipliers of
   

   s/this the/this, the/

7. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -189,68 +190,90 @@ function theme_responsive_image($variables) {
   +              // this breakpoint should be merged in to one srcset and the sizes
   

   s/in to/into/

8. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -189,68 +190,90 @@ function theme_responsive_image($variables) {
   +              // http://www.w3.org/html/wg/drafts/html/master/embedded-content.html#image-candidate-string
   

   The above is nigh impossible to understand without reading the spec or being very, very familiar with the syntax.

   It'd be great if a concrete example would be added.

9. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -189,68 +190,90 @@ function theme_responsive_image($variables) {
   +      $sources[] = array(
   +        '#theme' => 'responsive_image_source',
   +        '#srcset' => array_map('unserialize', array_unique(array_map('serialize', $srcset))),
   +        '#media' => $breakpoint->getMediaQuery(),
   +        // Only set a MIME type if all derivative images have the same MIME type.
   +        '#mime_type' => count(array_unique($derivative_mime_types)) == 1 ? $derivative_mime_types[0] : '',
   +        '#sizes' => implode(',', array_unique($sizes)),
   +      );
        }
      }
    
      if (!empty($sources)) {
   

   Aha! So all of the above really was just about generating the <source> tags to be used within <picture>!

   Could we maybe split that building of $sources into a helper function? Then that helper function can get a fairly long docblock explaining what its purpose is, what the various things are that it does, and why it does them.

   That should greatly help understandability.

10. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -189,68 +190,90 @@ function theme_responsive_image($variables) {
    +        // Only set a MIME type if all derivative images have the same MIME type.
    

    If it's optional in one case, then why can't we always omit this?
    Is it for another front-end performance optimization?

11. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -262,39 +285,45 @@ function theme_responsive_image($variables) {
     function theme_responsive_image_source($variables) {
    

    I wonder if this would be clearer in general as a twig template? I think it might be.

    Perhaps I'm talking nonsense and this is not possible?

12. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -262,39 +285,45 @@ function theme_responsive_image($variables) {
    +    // URI is mandatory.
    

    Let's throw an exception if it's missing then.

    throw new \LogicException('…');

13. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -302,34 +331,50 @@ function theme_responsive_image_source($variables) {
    +function responsive_image_get_image_dimensions($image_style_name, $dimensions) {
    

    AFAIK $dimensions must be an array. If that's right, let's typehint it as such.

14. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -302,34 +331,50 @@ function theme_responsive_image_source($variables) {
    + * Determines the MIME type of an image.
    ...
    +  // The mime type guesser needs a full path, not just an extension, but the
    ...
    +  return Drupal::service('file.mime_type.guesser.extension')->guess($fake_path);
    

    "determine" implies certainty, whereas "guesser" does not. So which is it?

    If it's not certain, then why is that okay?

15. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -302,34 +331,50 @@ function theme_responsive_image_source($variables) {
    +    // http://probablyprogramming.com/2009/03/15/the-tiniest-gif-ever
    

    :)

16. +++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
    @@ -193,26 +195,42 @@ public function viewElements(FieldItemListInterface $items) {
    +      // Link the picture element to the original file.
    

    s/picture/
    /

17. +++ b/core/modules/responsive_image/src/ResponsiveImageMappingInterface.php
    @@ -27,7 +27,13 @@ public function hasMappings();
    +   *     - image_mapping_type: One of image_style or sizes.
    
    @@ -62,17 +71,35 @@ public function setBreakpointGroup($breakpoint_group);
    +   *     - image_mapping_type: One of image_style or sizes.
    

    Either 'image_style' or 'sizes'.

18. +++ b/core/modules/responsive_image/src/ResponsiveImageMappingInterface.php
    @@ -27,7 +27,13 @@ public function hasMappings();
    +   *     - image_style: If image_mapping_type is image_style, the image style ID.
    +   *     - sizes: If image_mapping_type is sizes, the value for the sizes attribute.
    
    @@ -62,17 +71,35 @@ public function setBreakpointGroup($breakpoint_group);
    +   *     - image_style: If image_mapping_type is image_style, the image style ID.
    +   *     - sizes: If image_mapping_type is sizes, the value for the sizes attribute.
    

    80 col rule violations.

19. +++ b/core/modules/responsive_image/src/ResponsiveImageMappingInterface.php
    @@ -62,17 +71,35 @@ public function setBreakpointGroup($breakpoint_group);
    +   *   The mapping definition. NULL if the mapping does not exist.
    ...
    +   *     - breakpoint_id: The breakpoint ID for this mapping.
    +   *     - multiplier: The multiplier for this mapping.
    ...
    +   * Checks if there is at least one mapping defined.
    ...
    +   * @param array $mapping_definition
    +   *   The mapping definition array.
    ...
    +  public static function isEmptyMappingDefinition(array $mapping_definition);
    

    "mapping definition" vs. "mapping"

10.

+++ b/core/modules/responsive_image/responsive_image.module
@@ -189,68 +190,90 @@ function theme_responsive_image($variables) {
+        // Only set a MIME type if all derivative images have the same MIME type.

If it's optional in one case, then why can't we always omit this?
Is it for another front-end performance optimization?

[Emphasis mine]

It is. Best example here is WebP. Browsers supporting it can choose to use those images for better perfomance. Browsers that don't will load the jpeg (or whatever) alternative.

I'll try to address the other points.

BTW: Thanks for the review! ;-)

14.

+++ b/core/modules/responsive_image/responsive_image.module
@@ -302,34 +331,50 @@ function theme_responsive_image_source($variables) {
+ * Determines the MIME type of an image.
...
+  // The mime type guesser needs a full path, not just an extension, but the
...
+  return Drupal::service('file.mime_type.guesser.extension')->guess($fake_path);

"determine" implies certainty, whereas "guesser" does not. So which is it?

If it's not certain, then why is that okay?

Unless somebody does something really exotic, the MIME type "guesser" will always be bang on the money. It is named as a guesser by Symfony, and Drupal just followed their standard, but looking at the code it is pretty straightforward (it just matches extensions to MIME types...). I'm willing to change the comment to "Guesses" (vs "Determines"), but that wouldn't really be correct either I think...

150: Lucky number? =D

Looks like Picture 2.x branch is still beta rather than stable. I'm fine with upgrading to beta code as long as we're confident there'll eventually be a stable release.

If there's a stable release before we hit release candidate, we should go ahead and bump this to critical I think to avoid getting stuck on the 1.x branch. I nearly did that just now before noticing the 2.x branch was still beta.

I was wrong, only 2.2.0-beta is beta, 2.1.0 is stable.

Bumping to critical.

@catch, @attiks am I missing something, Version 1.2.1 is listed as the latest stable version here http://scottjehl.github.io/picturefill/

That makes sense to me. Guess their docs are just a bit behind....

2.2 is stable now. Can we update to that and then maybe we can track down Wim for a review so we can get this in?

"PS: Any reason why this is not a beta blocker?"

Because beta was already released two months ago. :)

Forgot the twig template...

I'm not exactly sure what's causing the exceptions. Are they related to this patch or is testbot acting up?

+++ b/core/modules/responsive_image/config/schema/responsive_image.schema.yml
@@ -29,6 +35,17 @@ responsive_image.mappings.*:
+                - type: string
+                  label: 'Image style'

It says schema related is this not supposed to be hyphened and indented as much?

Guessing by playing the what's different game:)

Well dang, yaml syntax is weird and unpredictable to me;) ... need more practice

Ok, so apparently it was a pretty simple fix (see interdiff). Patch should be green now.

Wow, this has definitely been moving in the right direction! Thank you so much, Jelle, especially for the splitting off of part of the markup-generating logic into a helper function and adding loads of docs to it, that helps tremendously, and I think will make it much easier for others to step up to review this also :)

This is again a pure code review. If you could address that, I think the next step is for me to manually test it and step through it with a debugger to make sure I truly understand the code, rather than just reading the code and thinking "this makes sense".

1. +++ b/core/core.libraries.yml
   @@ -776,12 +776,10 @@ normalize:
   +  version: 2.1.0
   ...
   +    url: https://github.com/scottjehl/picturefill/blob/2.1.0/LICENSE
   

   2.2.0.

2. +++ b/core/modules/responsive_image/config/schema/responsive_image.schema.yml
   @@ -20,6 +20,12 @@ responsive_image.mappings.*:
   +            # Image mapping type. Either 'sizes' (using the 'sizes' attribute)
   +            # or 'image_style' (using a single image styles to map to this
   +            # breakpoint).
   +            image_mapping_type:
   +              type: string
   +              label: 'Responsive image mapping type'
   
   @@ -29,6 +35,17 @@ responsive_image.mappings.*:
   +            # The value for the sizes attribute as described in the spec:
   +            # http://www.w3.org/html/wg/drafts/html/master/embedded-content.html#attr-img-sizes
   +            sizes:
   +              type: string
   +              label: 'Sizes attribute'
   

   Thank you, these docs and more descriptive labels help a lot!

   But in the first doc, it says using a single image styles to map to this breakpoint, which I definitely cannot parse.

3. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -189,114 +190,197 @@ function theme_responsive_image($variables) {
   +    $output[] = '<!--[if IE 9]><video style="display: none;"><![endif]-->';
   +    $output = array_merge($output, array_map('drupal_render', $sources));
   +    $output[] = '<!--[if IE 9]></video><![endif]-->';
   

   I understand what this does, but at first sight it's… unsettling :)

   I wonder if we can simplify this?

   In this case, I'd say it's better to use a foreach-loop instead.

   i.e.

   $output[] = 'crappy IE prefix';
   foreach ($sources as $source) {
     $output[] = drupal_render($source);
   }
   $output[] = 'crappy IE suffix';
   

   It's not slower, and it makes the code easier to follow, because it's much more similar to the surrounding code, hence not taking the reader out of his/her thought flow.

4. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -189,114 +190,197 @@ function theme_responsive_image($variables) {
   +    // Output the fallback image. Use srcset in the fallback image to avoid
   +    // unnecessary preloading of images in older browsers.
   

   Interesting. Won't this be a problem in case you're dealing with a browser that doesn't support srcset?

   And can you link to a reference explaining why this prevents unnecessary preloading?

5. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -189,114 +190,197 @@ function theme_responsive_image($variables) {
   + * Builds an array of render arrays that will output <source> tags to be used in
   + * a <picture> tag.
   

   Great description, but could use one addition: In other words, this function provides the contents for a <picture> tag.

6. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -189,114 +190,197 @@ function theme_responsive_image($variables) {
   + * of its multipliers. A mapping can be either of two types: 'sizes' (meaning
   + * it will output a <source> tag with the 'sizes' attribute' or 'image_style'
   

   Missing closing parenthesis.

7. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -189,114 +190,197 @@ function theme_responsive_image($variables) {
   + * (meaning it will output a source tag based on the selected image style for
   ...
   + * attribute of a source tag have the same MIME type, the source tag will get a
   

   s/source tag/ tag/, for consistency with the above.

8. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -189,114 +190,197 @@ function theme_responsive_image($variables) {
   + * 'mime-type' attribute as well. This way we can gain some front-end
   + * performance because browsers can select which image to load based on the
   + * MIME types they support (which, for instance, can be beneficial for browsers
   + * supporting WebP).
   

   This contradicts what's written above. Above, you say that if and only if all images in the srcset attribute of a <source> have the same MIME type, we set the mime-type attribute. But if they're all of the same MIME type, then how can a browser make a selection?

   If this is not a contradiction, but me misunderstanding, please clarify this aspect.

9. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -189,114 +190,197 @@ function theme_responsive_image($variables) {
   + * Because of this, all image styles for all multipliers of this breakpoint are
   + * merged into one 'srcset' and the 'sizes' attribute is merged as well. For
   

   I don't understand this. Can you give an example of this also?

   Overall, the documentation you added here is of TREMENDOUS value. For something as complex as this, I don't think we almost cannot have too much documentation. This really helps explain design/algorithm decisions, so it's of the utmost importance to explain all these subtleties with clear examples, for those who aren't deeply familiar with the complexity of responsive images.

10. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -189,114 +190,197 @@ function theme_responsive_image($variables) {
    + * @param ImageInterface $image
    ...
    + *     - mapping_id: The ResponsiveImageMapping ID.
    

    Must use fully qualified classes/interfaces in docs.

11. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -304,34 +388,50 @@ function theme_responsive_image_source($variables) {
    +  // The mime type guesser needs a full path, not just an extension, but the
    

    s/mime/MIME/

    In *all* docs we'd want "MIME", not "mime".

12. +++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
    @@ -193,26 +195,42 @@ public function viewElements(FieldItemListInterface $items) {
    +      // Picturefill uses the first matching breakpoint. Meaning the breakpoints
    

    Is this a Picturefill (i.e. polyfill) implementation thing or is it a spec thing?

    We should be conforming to the spec, and if we're not, to accommodate the polyfill, we should stress this EXTREMELY.

13. +++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
    @@ -193,26 +195,42 @@ public function viewElements(FieldItemListInterface $items) {
    +     $image_styles = ImageStyle::loadMultiple($image_styles_to_load);
    

    Small indentation error: one leading space too many.

14. +++ b/core/modules/responsive_image/src/ResponsiveImageMappingInterface.php
    @@ -27,7 +27,15 @@ public function hasMappings();
    +   *     - image_mapping_type: Either 'image_style' or 'sizes'.
    
    @@ -62,17 +73,37 @@ public function setBreakpointGroup($breakpoint_group);
    +   *     - image_mapping_type: Either 'image_style' or 'sizes'.
    

    You now use single quotes here to make it clearer what the values may be.

15. +++ b/core/modules/responsive_image/src/ResponsiveImageMappingInterface.php
    @@ -27,7 +27,15 @@ public function hasMappings();
    +   *     - image_style: If image_mapping_type is image_style, the image style
    ...
    +   *     - sizes: If image_mapping_type is sizes, the value for the sizes
    ...
    +   *     - sizes_image_styles: The image styles to use for the srcset attribute.
    
    @@ -62,17 +73,37 @@ public function setBreakpointGroup($breakpoint_group);
    +   *     - image_style: If image_mapping_type is image_style, the image style
    ...
    +   *     - sizes: If image_mapping_type is sizes, the value for the sizes
    ...
    +   *     - sizes_image_styles: The image styles to use for the srcset attribute.
    

    But please also do so here.

16. +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
    @@ -77,6 +96,64 @@ public function testResponsiveImageFieldFormattersPrivate() {
    +   * Test responsive image formatters on node display linked to the file.
    ...
    +   * Test responsive image formatters on node display linked to the node.
    ...
    +   * Test responsive image formatters linked to the file or node.
    

    Nit: s/Test/Tests/

17. +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
    @@ -77,6 +96,64 @@ public function testResponsiveImageFieldFormattersPrivate() {
    +  public function _testResponsiveImageFieldFormattersLink($link_type) {
    

    First of all this should not be public, since it's a helper function.

    Second, AFAIK we typically name these assertSomethingCustom().

18. +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
    @@ -77,6 +96,64 @@ public function testResponsiveImageFieldFormattersPrivate() {
    +    $node = node_load($nid, TRUE);
    

    Nit: Node::load()

19. +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
    @@ -77,6 +96,64 @@ public function testResponsiveImageFieldFormattersPrivate() {
    +    $image_uri = file_load($node->{$field_name}->target_id)->getFileUri();
    

    Nit: File::load()