Follow-up from #1883526: Decide on the picture polyfill to use.

The <picture>; element is officially part of both Blink (Chrome + Opera). See picture element implementation in blink. <picture> is also being implemented for Firefox. IE is starting to implement scrset and sizes.

To fill the gap in the mean time, Picturefill 2.0 has been released.

Since picture has changed a lot in the mean time, we need to add support for both media queries and sizes, see http://picture.responsiveimages.org/

Adressing

#212why do we have to support "sizes" in order to use picturefill?

Sizes is the preferred way to output responsive images, Drupal will look a bit foolish if we will not support it. See #109 for an explanation, as well as the following links

Steps NOT done:

  1. Switch output to img tag if possible (nice to have)
  2. Add alternative field formatter with support for sizes without needing a mapping (nice to have)
  3. Discuss UI of breakpoint in #1701112: Advanced responsive images UI/UX for breakpoints

Steps done:

  1. Update picturefill library to latest version
  2. Make sure picture and source tags are correct
  3. We need to remove the setting of width and height on source elements
  4. Add type support for source tag (code, not UI), can now use transformMimeType
  5. Allow source tag without a media attribute (code, not UI)
  6. Add support for sizes on source tag
  7. Update UI of responsive images mapping
  8. Add work around for IE9, http://scottjehl.github.io/picturefill/#ie9
Files: 
CommentFileSizeAuthor
#370 Welcome_to_Site-Install___Site-Install.png132.81 KBjoelpittet
#370 Welcome_to_Site-Install___Site-Install.png114.8 KBjoelpittet
#363 interdiff-361-363.txt2.36 KBJelle_S
#363 2260061-363.patch141.68 KBJelle_S
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,687 pass(es). View
#150 2260061-picture-polyfill-2.1-150.patch88.66 KBJelle_S
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2260061-picture-polyfill-2.1-150.patch. Unable to apply patch. See the log in the details link for more information. View
#188 interdiff-177-188.txt17.08 KBJelle_S
#193 2260061-picture-polyfill-2.2-193.patch97.27 KBJelle_S
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2260061-picture-polyfill-2.2-193.patch. Unable to apply patch. See the log in the details link for more information. View
#242 interdiff-241-242.txt670 bytesJelle_S
#242 2260061-picture-polyfill-2.2-242.patch98.24 KBJelle_S
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,996 pass(es). View

Comments

attiks’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
marcvangend’s picture

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/

mdrummond’s picture

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.

attiks’s picture

Issue summary: View changes
attiks’s picture

I left a comment in [##2220865] because I think it has to be postponed since the polyfill is going to change. I think we have to go with the polyfill as is and if we want changes, we have to push them upstream.

I added a small section with needed steps, only problem I see for the moment is the UI part.

marcvangend’s picture

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?

Jelle_S’s picture

Assigned: Unassigned » Jelle_S

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.

Jeff Burnz’s picture

Assigned: Jelle_S » Unassigned

#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.

Jeff Burnz’s picture

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

Jelle_S’s picture

Assigned: Unassigned » Jelle_S

No problem ;-)

Jelle_S’s picture

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

marcvangend’s picture

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.

mdrummond’s picture

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.

Jelle_S’s picture

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.

sun’s picture

Issue tags: -Needs reroll +API change

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?

Jelle_S’s picture

@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.

attiks’s picture

Wim Leers’s picture

FileSize
687 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,566 pass(es). View

#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!

attiks’s picture

Jelle_S and I discussed the possible needed UI changes, and for the moment we can do most of them using the UI of the mapping, we will probably add something so it will be easier for people to test for now, and move the discussion about the UI to the other issue.

attiks’s picture

Issue summary: View changes
Jelle_S’s picture

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

mdrummond’s picture

#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.

Jeff Burnz’s picture

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).

attiks’s picture

I created #2262863: Add srcset to template_preprocess_image so we can use it for the fallback image

mdrummond’s picture

#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.

attiks’s picture

An example hoping to clarify that sorting is not possible:

@media not all and (monochrome)
@media (min-width: 700px) and (orientation: landscape)
@media (min-width: 550px) and (orientation: portrait)
Jelle_S’s picture

FileSize
61.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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)
Jelle_S’s picture

Status: Needs work » Needs review

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

Wim Leers’s picture

Wow, awesome work!

attiks’s picture

Status: Needs work » Needs review
FileSize
0 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,624 pass(es). View

reroll

mdrummond’s picture

0 bytes. Something went wrong with the upload.

attiks’s picture

attiks’s picture

FileSize
66.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch i2260061-picture-polyfill-2-32_0.patch. Unable to apply patch. See the log in the details link for more information. View

Let's try again

attiks’s picture

FileSize
66.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
3.19 KB

Code added to make sure mime type is unique

mdrummond’s picture

Status: Needs work » Needs review

Go testbot go!

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
67.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,679 pass(es). View
1.4 KB

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.

attiks’s picture

Issue summary: View changes
attiks’s picture

Issue summary: View changes
mdrummond’s picture

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!

sun’s picture

  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'.

attiks’s picture

#46-2: The 'none' breakpoint is needed because source does not require a media attribute, so we need a way to specify this in a breakpoint group (and export it to yml). The patch in #37 was creating it in memory, but was causing troubles during unit testing.

The none part is about the media query, it is an empty breakpoint. But you're right that a user should not be able to change the definition.

#46-5: transformMimeType is needed because we need a way to get the mime type of the derivative since an image effect can change the mime type of a file (like converting from png to jpg), we used the same approach as is used for transformDimensions.

#46-7: do we use 'none' or '_none'?

Jelle_S’s picture

FileSize
78.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,281 pass(es). View
18.77 KB

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.

marcvangend’s picture

+++ 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?

Jelle_S’s picture

@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.

attiks’s picture

@marcvangend: see also #27 why sorting is not possible

marcvangend’s picture

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.

attiks’s picture

FileSize
78.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,287 pass(es). View
Jelle_S’s picture

FileSize
82.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,700 pass(es). View
3.54 KB
Jelle_S’s picture

FileSize
79.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,718 pass(es). View
2.31 KB
mdrummond’s picture

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.

yched’s picture

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 :-)

LewisNyman’s picture

Issue tags: +frontend
attiks’s picture

#57: Sizes are not tied to breakpoints, you're right about the syntax.

Sizes can be as simple as sizes="100vw" and a list of image styles, so the browser can choose the most optimal one, taking into account both the viewport width and the dpi.

The sizes can be configured on a picture mapping and don't even need a breakpoint. The user fills in a text field with one or more sizes and selects (checkbox list) some image styles.

yched’s picture

@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’s picture

I guess I was not very clear, when creating a picture mapping you can fill in a textfield for the sizes, this can be as simple as 100vw or 50vw, but you could also enter (max-width: 30em) 100vw, (max-width: 50em) 50vw, calc(33vw - 100px), but the latest example use something like media conditions, but the max-width: 30em, max-width: 50em don't have to be defined inside the theme breakpoints.

A screenshot of the picture mapping UI can be seen at #2271529-10: Move breakpoint settings to theme and module *.breakpoints.yml files at root level, notice also the presence of an empty breakpoint, allowing the user to only use sizes without a breakpoint.

attiks’s picture

Issue summary: View changes

Todo: Add work around for IE9, http://scottjehl.github.io/picturefill/#ie9

yched’s picture

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

mdrummond’s picture

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.

attiks’s picture

Breakpoints group are going away, see #2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level. You can define multiple picture mappings for the same theme.

Since a site will probably have a multitude of sizes, some with/without a breakpoint, I think the best is to keep sizes as part of the picture mapping, so we don't clutter the theme.info file, otherwise a lot will need to be repeated.

Can we move the empty breakpoint discussion to the other issue, although I think defining an empty breakpoint can not be that much of a deal.

"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", this will get complex, since you can combine sources and sizes and this will mean writing a lot of (nested) yaml

PS: This issue is about implementing the new polyfill, I understand your concerns, but if we want to move forward we need this issue or #2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level committed, so at least everybody knows where we're going.

mdrummond’s picture

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.

Jelle_S’s picture

FileSize
78.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,757 pass(es). View

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.

Jelle_S’s picture

Issue summary: View changes
attiks’s picture

mdrummond’s picture

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?

attiks’s picture

We do want 2.1, but there's no rush, we need to get #2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level reviewed/committed first

Wim Leers’s picture

Title: Implement Picture polyfill 2.0 » [PP-1] Implement Picture polyfill 2.0
Status: Needs review » Postponed
Related issues: +#2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level
mdrummond’s picture

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

Jelle_S’s picture

FileSize
77.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2260061-picture-polyfill-2-74.patch. Unable to apply patch. See the log in the details link for more information. View

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.

mdrummond’s picture

Title: [PP-1] Implement Picture polyfill 2.0 » [PP-1] Implement Picture polyfill 2.1

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

attiks’s picture

Title: [PP-1] Implement Picture polyfill 2.1 » Implement Picture polyfill 2.1
Status: Postponed » Needs review
mdrummond’s picture

Issue tags: +Needs reroll
Jelle_S’s picture

Assigned: Jelle_S » Unassigned
Status: Needs work » Needs review
FileSize
78.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
81.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
1.96 KB

Forgot to update a unit test.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
89.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,243 pass(es), 1 fail(s), and 0 exception(s). View
9.76 KB

Ok let's try that again...

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
89.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,245 pass(es). View
749 bytes

This shoud fix the 1 fail from #83

Wim Leers’s picture

Status: Needs review » Needs work

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.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
8.67 KB
90.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,285 pass(es). View

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)?

Jelle_S’s picture

FileSize
2.59 KB
90.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,285 pass(es). View

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

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

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/

attiks’s picture

#89

1/ Picture has support for different mime types, se we need an easy way to determine the mime type after all image styles are run, the is similar to transformDimensions, and uses a byref var as well

6/ This is needed to support IE9

8/ All of these should become ImageStyle::load(…). but this is nowhere else used?

9/ Is this a huge difference?

Wim Leers’s picture

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.

Jelle_S’s picture

@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?

Wim Leers’s picture

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.

Jelle_S’s picture

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.

Jelle_S’s picture

#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?

Wim Leers’s picture

#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.

Jelle_S’s picture

Assigned: Unassigned » Jelle_S

Ok, I'm on it :-)

Jelle_S’s picture

Assigned: Jelle_S » Unassigned
Status: Needs work » Needs review
FileSize
29.13 KB
94.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,287 pass(es). View

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

Wim Leers’s picture

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

Wim Leers’s picture

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.


I began reviewing the code again in dreditor and stepped through theme_responsive_image() and the complexity of responsive image rendering has become worse, not better. It really shouldn't get any worse, which is why I'm proposing the above.
+++ 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.

mdrummond’s picture

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.

Wim Leers’s picture

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.

yched’s picture

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...)

mdrummond’s picture

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.

mdrummond’s picture

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.

attiks’s picture

Defining sizes inside a yml is not the way to go, sizes are not related to breakpoints used inside your theme, you select sizes in function of the min/max size of the displayed image, so if you have an image that can be anything between 150px and 800px wide, you select sizes so the difference in download size is relevant. This depends on a lot of things (original image, image format, ..) all unrelated to the theme.

Granted the UI is complicated, but the whole picture element is complicated.

If it makes you feel better we can start splitting this in smaller issues, but my biggest fear is that all will stop once we start discussing (or bike-shedding) the UI.

If we make a new issue for sizes it means we will need to adapt the image system completely, since sizes can be used without picture, so this means it will take a long time. I rather add it as part of picture and refactor it later so it can be used by image as well.

FYI: #2262863: Add srcset to template_preprocess_image already exists, but needs a reroll, it adds srcset to image and support for MIME type

attiks’s picture

FYI: #2262863-26: Add srcset to template_preprocess_image is rerolled using the relevant parts of this patch

mdrummond’s picture

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.

attiks’s picture

Discussed the approach with @Jelle_S:

1/ Get #2262863: Add srcset to template_preprocess_image committed
2/ Get #2330899: Allow image effects to change the MIME type + extension, add a "convert" image effect committed
3/ Get #2333395: Add sizes to template_preprocess_image committed
4/ Reroll this patch with sizes, but without any UI
5/ Create a new issue to discuss the UI

Jelle_S’s picture

FileSize
94.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,850 pass(es). View
32.28 KB

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.

attiks’s picture

mdrummond’s picture

mdrummond’s picture

Issue tags: +Needs reroll
attiks’s picture

FileSize
94.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,884 pass(es). View

rerolled

mdrummond’s picture

Status: Needs review » Needs work

Needs another reroll.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
94.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,329 pass(es). View

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.

Jelle_S’s picture

Issue tags: -Needs reroll
attiks’s picture

attiks’s picture

Another "problem" found, Drupal does not allow image effects to change the extension, #2341251: Allow image effects to change the extension created since this will be needed to allow people to output both jpg and webp at the same time. Or allow people to output an svg and a jpg fallback.

Jelle_S’s picture

FileSize
82.13 KB

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) .

attiks’s picture

Delete operation added

Jelle_S’s picture

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

Jelle_S’s picture

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.

mgifford’s picture

attiks’s picture

Needs a reroll

mgifford’s picture

FileSize
86.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2260061-picture-polyfill-2.1-127.patch. Unable to apply patch. See the log in the details link for more information. View

Ok, hopefully this helps move this along.

attiks’s picture

#128 Patch fails because it depends on the MIME type patch

PS: Follow up created: #2349461: Move fallback image style into the responsive image style entity

mdrummond’s picture

Issue tags: +Needs reroll

Just retested. Needs reroll.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
84.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,411 pass(es), 35 fail(s), and 0 exception(s). View

Rerolled patch.

Jelle_S’s picture

Issue tags: -Needs reroll
Jelle_S’s picture

Assigned: Unassigned » Jelle_S

Apparently it needs more than just a reroll.

Jelle_S’s picture

Assigned: Jelle_S » Unassigned
Status: Needs work » Needs review
FileSize
84.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,442 pass(es). View
3.78 KB

Let's try this again.

mgifford’s picture

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.

attiks’s picture

Best way to test is to create a new mapping first, bartik will do using the build in image styles, change the display formatter of the image on the article to use the newly defined mapping.

To verify check the html output, preferably using chrome 38 with javascript disabled and using a browser that has no native support.

PS: FF has still one blocker before they can ship with picture support enabled.

attiks’s picture

I pushed my demotheme used for my presentation to github, see https://github.com/attiks/demotheme

Once enabled it will add image styles and mappings so you can test both art case as srcset with sizes.

Keep in mind that the output will always be a picture tag, this is done to avoid double downloads.

mdrummond’s picture

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!

attiks’s picture

FYI development for safari has started https://bugs.webkit.org/show_bug.cgi?id=137496

Wim Leers’s picture

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"

Jelle_S’s picture

Assigned: Unassigned » Jelle_S

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! ;-)

Jelle_S’s picture

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...

Jelle_S’s picture

Assigned: Jelle_S » Unassigned
FileSize
88.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2260061-picture-polyfill-2.1-150.patch. Unable to apply patch. See the log in the details link for more information. View
15.98 KB

150: Lucky number? =D

catch’s picture

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.

catch’s picture

Priority: Major » Critical

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

Bumping to critical.

attiks’s picture

#151 Picturefill Version 2.2.0 is Beta, 2.1.0 is stable

mgifford’s picture

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

attiks’s picture

mgifford’s picture

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

mdrummond’s picture

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?

attiks’s picture

I'll try to reroll today

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

attiks’s picture

Title: Implement Picture polyfill 2.1 » Implement Picture polyfill 2.2
FileSize
91.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,736 pass(es), 33 fail(s), and 0 exception(s). View

Reroll with picturefill 2.2

webchick’s picture

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

Because beta was already released two months ago. :)

attiks’s picture

#161 LOL, not really what I wanted to know, just getting worried that we will keep rerolling this for fun and that we will miss RC as well

attiks’s picture

FileSize
91.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,761 pass(es), 1 fail(s), and 0 exception(s). View
940 bytes
attiks’s picture

Status: Needs work » Needs review
catch’s picture

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
91.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,792 pass(es), 16 fail(s), and 2 exception(s). View
928 bytes
Jelle_S’s picture

FileSize
91.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,816 pass(es), 0 fail(s), and 2 exception(s). View

Forgot the twig template...

Jelle_S’s picture

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

joelpittet’s picture

+++ 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:)

attiks’s picture

#174 I don't think so, the next line after a sequence: is indented and starts with a -, other lines of the sequence are indented again.

joelpittet’s picture

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

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
91.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,821 pass(es). View
746 bytes

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

Wim Leers’s picture

Status: Needs review » Needs work

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()

Jelle_S’s picture

Assigned: Unassigned » Jelle_S

2/
Should be "Using a single image style to map to this breakpoint" (s/styles/style). Meaning that when this breakpoint is active, the selected image style will be used to display the image.

4/
Not really, no, since the polyfill will get triggered for browsers that do not support srcset.
I can't immediately find a link, but when a browser that doesn't support the picture tag parses an image tag with a src attribute (i.e. our fallback image) it will preload that image, only to get replaced once the polyfill kicks in...

8/
The full comment there is:

When all the images in the 'srcset' attribute of a source tag have the same MIME type, the source tag will get a '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).

Maybe this is more clear:

When all the images in the 'srcset' attribute of a source tag have the same MIME type, the source tag will get a 'mime-type' attribute as well. This way we can gain some front-end performance because browsers can select which image (source tag) to load based on the MIME types they support (which, for instance, can be beneficial for browsers supporting WebP).

A source tag can contain multiple images:

<source [...] srcset="image1.jpeg 1x, image2.jpeg 2x, image3.jpeg 3x" />

In the above example we can add the mime-type attribute ('image/jpeg') since all images in the srcset attribute of the source tag have the same MIME type.

If a source tag were to look like this:

<source [...] srcset="image1.jpeg 1x, image2.webp 2x, image3.jpeg 3x" />

We can't add the mime-type attribute ('image/jpeg' vs 'image/webp').

So in order to add the 'mime-type' attribute to the source tag all images in the 'srcset' attribute of the source tag need to be of the same MIME type.

This way, a picture tag could look like this:

<picture>
  <source [...] mime-type="image/webp" srcset="image1.webp 1x, image2.webp 2x, image3.webp 3x"/>
  <source [...] mime-type="image/jpeg" srcset="image1.jpeg 1x, image2.jpeg 2x, image3.jpeg 3x"/>
  <img srcset="fallback.jpeg" />
</picture>

This way a browser can decide which source tag is preferred based on the MIME type.

So the MIME types of all images in one source tag need to be the same in order to set the mime-type attribute but not all MIME types within the picture tag need to be the same.

9/
Mappings are per breakpoint, per multiplier. So 2 breakpoints and 2 multipliers per breakpoint will result in 4 mappings:
eg:


array(
  array(
    'image_mapping_type' => 'sizes',
    'breakpoint_id' => 'bartik.mobile',
    'multiplier' => '1x',
    'sizes' => '(max-width: 30em) 100vw, (max-width: 50em) 50vw, 25vw',
    'sizes_image_styles' => array(
      'thumbnail',
      'medium',
    ),
  ),
  array(
    'image_mapping_type' => 'sizes',
    'breakpoint_id' => 'bartik.mobile',
    'multiplier' => '2x',
    'sizes' => '(max-width: 30em) 100vw, (max-width: 50em) 50vw, 25vw',
    'sizes_image_styles' => array(
      'medium',
      'large',
    ),
  ),
  array(
    'image_mapping_type' => 'image_style',
    'breakpoint_id' => 'bartik.narrow',
    'multiplier' => '1x',
    'sizes' => '(max-width: 30em) 100vw, (max-width: 50em) 50vw, 25vw',
    'sizes_image_styles' => array(
      'thumbnail',
      'medium',
    ),
  ),
  array(
    'image_mapping_type' => 'image_style',
    'breakpoint_id' => 'bartik.narrow',
    'multiplier' => '2x',
    'sizes' => '(max-width: 30em) 100vw, (max-width: 50em) 50vw, 25vw',
    'sizes_image_styles' => array(
      'medium',
      'large',
    ),
  ),
);

But, since we can't use the multiplier when a width descriptor is present, we have to merge the mappings of the 1x and 2x multipliers in to 1 source tag. This means we have to merge the 'sizes' attribute defined in both mappings as well as the 'sizes_image_styles' defined in both mappings.

I'm willing to put all of this in the comment, but I feel like I'm eventually writing an entire book in code :-)

12/
This is a spec thing: see http://www.w3.org/html/wg/drafts/html/master/embedded-content.html#updat... points 2 & 3(.6). I'll add it to the comment.

I won't get this done today, but I wanted to answer here first so we can agree on these points before I spend time on it (so I can avoid doing double work)

PS: Thanks for the review ;-)

yched’s picture

4/
Not really, no, since the polyfill will get triggered for browsers that do not support srcset.

The consequence is : no image is displayed at all for old browsers without srccet support when JS is disabled.
But yes, that is in fact the recommended practice for picturefill.

Wim Leers’s picture

  • 2. Yep, that makes sense :)
  • 4. I'm confused now. I cited code that said Output the fallback image. Use srcset in the fallback image to avoid unnecessary preloading of images in older browsers. (emphasis mine). But now you say an image tag with a src attribute (i.e. our fallback image). So, which is it? src or srcset? Because that's what I was getting at, but didn't explain very well probably: I thought "fallback" meant "zero support for responsive images", hence a plain <img src="" /> image. The code comment conflicts with that, because srcset is not universally supported. But you now also say src… so which is it? Or is it both?
    (EDIT: yched's comment suggests it's only srcset. Your example at the bottom of point 8 also does. But AFAICT this is a broken fallback for browsers without JS and no srcset support, so I'm still not convinced this is the way to go.)
  • 8. Ohhh, I see! That makes sense :) It's because there are so many levels that it becomes hard to understand. Please include the full-length example you wrote here in the docs.
  • 9. I still don't fully understand this. I do understand the need for a mapping per breakpoint/multiplier combo. I don't yet understand how that merging works. It'd help if you could give a concrete example of that as well.
    Don't worry about writing too long comments. In this case, the complexity does not stem from Drupal, but from the spec… and hence we want to make the code understandable to Drupal developers/themers reading this code.

So I'd say: please do continue along the path you're on, you're making me understand the parts I didn't understand yet :)

Thank you so much for working on this! It will be worth it in the end!

yched’s picture

But AFAICT this is a broken fallback for browsers without JS and no srcset support, so I'm still not convinced this is the way to go

That's what Picturefill officially supports. img with only an srcset is the recommended way for fallback.

Snippets copied from https://scottjehl.github.io/picturefill/ :

Version 1.2.1 of Picturefill [the old one] is available if you are interested in the older markup pattern that uses span elements. This version is a lot lighter in weight than version 2 and allows for a fallback image in browsers that have JavaScript disabled (whereas Picturefill version 2 only offers alt text as a fallback for non-JS browsers without picture support).

Your picture element should contain a series of source elements followed by an img element. Each source element must have a srcset attribute specifying one or more image url sources (which can use expanded srcset syntax if desired for resolution switching), and the img element should have a srcset attribute for fallback purposes as well

[follows some sample code where the img tag *only* has an srcet]

Support caveats to keep in mind

Picturefill is tested broadly and works in a large number of browsers. That said, it does have some browser support considerations to keep in mind:

- JS-Disabled Browsers only see alt text: When using the picture element, non-picture supporting browsers will only see alt attribute text as a fallback when JavaScript fails or is disabled. This is because any noscript-based workarounds (such as the one used in Picturefill version 1) will cause future browsers that support the picture element to show two images instead of one when JavaScript is off. Unfortunately, choosing to add a src attribute to the img element in your picture element isn't a good workaround either, as any browser that exists today will fetch that src url even if it is not going to be used (which is wasteful). If you absolutely need an image to appear in non-JS environments, Picturefill 1.2 is a fine choice.

(...)

attiks’s picture

#181 See #180 as well

We have 2 options for the fallback:

1/ output an src attribute, this will work in all browser, even those with js disabled, BUT that src will get prefetched by all browsers not implementing picture (safari, ie, firefox, opera and old chrome browsers)

2/ output srcset, this will work in all browsers as long as they have javascript enabled, so the only time this does not work is if a user has no javascript support.

Ideally we ship without a polyfill, but since this is a design that each site has to make, the best is to use option 2, so most visitors will at least see something, see also #2343351-2: Make picture polyfill optional for more in depth discussion.

attiks’s picture

#181 Point 9 "Mappings are per breakpoint, per multiplier

The data structure we use to store the mapping information is a mutli dimensional area, first level is the breakpoint, the second level is the multiplier.

The problem is that the specs do not allow you to mix width and multipliers inside 1 srcset, you either use something like

<source [...] srcset="image1.jpeg 1x, image2.webp 2x, image3.jpeg 3x" /> to support multipliers
<source [...] srcset="image1.jpeg 300w, image2.webp 600w, image3.jpeg 1200w" /> to support sizes

In practice most people will use the second option, because it gives the browser the most control, but in theory people could add a mapping for the same breakpoint (but different multiplier) so the array contains an entry for breakpointA.1x and breakpointA.2x, if we would output those we will end up with the following <source [...] srcset="image1.jpeg 300w 1x, image2.webp 600w 1x, image3.jpeg 1200w 1x, image1.jpeg 300w 2x, image2.webp 600w 2x, image3.jpeg 1200w 2x" /> which is illegal. SO the solutions is to merge both array into one and disregard the multiplier.

This will mainly be a UI problem, but that has to be handled in a separate issue.

Update: typo fixed, see Wim's remark

Wim Leers’s picture

#182 + #183: Thanks for your patience, for taking the time to explain this! I think this is fairly important downside to Picturefill 2. But I think it's indeed the right trade-off, considering that at least 97% of users have JS enabled.
That being said, I think we should document this very well in the code also.

#184: where you said so the array contains an entry for breakpointA.1x and breakpointB.2x you actually meant so the array contains an entry for breakpointA.1x and breakpointA.2x, I think? If so, then I understand.
I think it'd be valuable to explain this edge case in the code docs as well.

attiks’s picture

##18 Yes, I meant "so the array contains an entry for breakpointA.1x and breakpointA.2x, ", will fix it above as well

Wim Leers’s picture

Ok, great :)

Jelle_S’s picture

Assigned: Jelle_S » Unassigned
Status: Needs work » Needs review
FileSize
17.08 KB
96.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2260061-picture-polyfill-2.2-188.patch. Unable to apply patch. See the log in the details link for more information. View

#181/4 Yeah sorry about that. I should have specified: "If we were to use 'src' in stead of 'srcset' for our fallback image, it will preload that image, only to get replaced once the polyfill kicks in..."

Attached patch addresses all remarks from #178 and should be green :-)

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
94.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,722 pass(es), 1 fail(s), and 1 exception(s). View

Reroll of #188

Jelle_S’s picture

Assigned: Unassigned » Jelle_S
Status: Needs review » Needs work

Some tests were added to the responsive image module. I'll see if they are still relevant after this patch, and if so, make sure they are updated and pass.

Jelle_S’s picture

FileSize
97.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2260061-picture-polyfill-2.2-193.patch. Unable to apply patch. See the log in the details link for more information. View

Ok , the tests were still relevant, so I updated them to work with the new ResponsiveImageMapping structure. Should be green now... (I hope :-) )

Jelle_S’s picture

Assigned: Jelle_S » Unassigned
Status: Needs work » Needs review
Wim Leers’s picture

Issue tags: +Needs manual testing

Interdiff missing for #193 :(

Code review (of interdiff at #188):

  1. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -224,18 +228,46 @@ function theme_responsive_image($variables) {
    + * attribute of a <source> tag have the same MIME type, the source tag will get
    ...
    + * A source tag can contain multiple images:
    ...
    + * If a source tag were to look like this:
    

    s/source/<source>/

  2. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -224,18 +228,46 @@ function theme_responsive_image($variables) {
    + * In the above example we can add the mime-type attribute ('image/jpeg') since
    

    s/mime-type/'mime-type'/

  3. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -254,15 +286,41 @@ function theme_responsive_image($variables) {
    + * <source [...] sizes="(min-width: 40em) 80vw, 100vw" srcset="image1.jpeg 300w 1x, image2.webp 600w 1x, image3.jpeg 1200w 1x, image1.jpeg 300w 2x, image2.webp 600w 2x, image3.jpeg 1200w 2x" />
    

    Is it possible there's a flaw still in this example? How could the same image be used for different multipliers?

    Basically you list images A, B and C for different width descriptors (fine so far), but then you say the exact same images should be used for 2x displays as well. That doesn't make sense?
    Wouldn't it make more sense to list A1, B1, C1 (for 1x) and A2, B2, C2 (for 2x)?

This is now at the point where it needs 1) thorough manual testing, 2) deep code/test review. I'll take care of 2, but I can't do that today. It'd be great if somebody with responsive image knowledge outside of attiks/Jelle_S could do the manual testing.

attiks’s picture

FileSize
97.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,971 pass(es). View
1.9 KB

Addressed comments in #195

mgifford’s picture

1) So you enable the module, then go to configure it:

2) This page here still seems a bit confusing, not sure if it works out of the box:

admin/config/media/responsive-image-mapping/example

1x wide [all and (min-width: 851px)]
1x narrow [all and (min-width: 560px) and (max-width: 850px)]
1x mobile [(min-width: 0px)]

What is the 1x for? Why is there a 0px?

The description "Select an image style for this breakpoint." seems repetitive and not very helpful.

3) Then you go to say the article page and change the Image to a Responsive image /admin/structure/types/manage/article/display

4) Finally, after creating a content page, what should I see? I was looking for <picture> but didn't see it.

Looking to how to test this functionality as I think it's important to try to get in.

attiks’s picture

#197

2/
- 1x is the multiplier, everything between [] is the media query
- the UI is not updated, is a separate issue

3/
- yes
- but don't forget to select the defined mapping

kattekrab’s picture

I'd love to do some manual testing of this, but I'm honestly not sure where to start. I get a vague idea of what to do from #197 but some steps to test would be really helpful here.

In the meantime, I'll see if I can direct the eyeballs of people more knowledgeable than me to have a look!

Thanks for the great work on this, I agree with mgifford this would be great to get in.

kattekrab’s picture

update:
Some steps to test here #143

But unfortunately I still can't quite figure out what I need to do to test it! Sorry :/

xjm’s picture

Per discussion with @catch, @effulgentsia, @webchick, and @alexpott, it's release-blocking to upgrade the library if we keep it in core. However, there are also reasons to consider #2343351: Make picture polyfill optional instead. In either case, there will need to be some work in core as well. So we're tagging both issues for later triage and discussion.

Unrelated: When rolling patches that add asset libraries like this, also add a -do-not-test.patch that contains only the changes to Drupal (i.e. excluding the vendor changes) so that those can be reviewed. There's definitely a lot more going on in that patch than a library upgrade. Thanks!

catch’s picture

Title: Implement Picture polyfill 2.2 » Responsive image module does not support sizes/picture polyfill 2.2

Trying a re-title to make it clearer what the scope is. Please feel free to improve further.

From what I understand most of the changes here are still critical if we do #2343351: Make picture polyfill optional - since the markup has to change anyway.

attiks’s picture

#201 Regardless of we ship a polyfill or not, this issue solves a lot more problems.

#202 Not sure about the title, but yes, this is critical for all other responsive image related issues and is blocking progress on them.

mdrummond’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

I can do manual testing of this patch, but we need a reroll first.

In #195, Wim mentioned there was additional deep code/testing review needed as well that he might be able to do.

catch’s picture

This changes the configuration schema, so tagging with D8 upgrade path.

Slightly cutting down the tags list.

holist’s picture

FileSize
98.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,207 pass(es), 1 fail(s), and 0 exception(s). View

Rerolled patch. Hit some minor conflicts, here's a summary for the record:

In core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php changed image_style:medium to config:image.style.medium.

In core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php removed lines that were removed in 8.0.x and not touched upon in #196.

In core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php changed delete-form link definition in docblock to current format.

holist’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
99.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,575 pass(es). View
3.44 KB

I diffed #196 with #206. The changes *mostly* look good. But there are also changes in the polyfill, and it'd be better if attiks or Jelle verified that. The only bug I found that #206 introduced was a $uri that should have been $url.
But, more interestingly, as of #206, there is now one test failure. That led me to discover a pre-existing bug in the test. Which led to another, analogous test failure, that has been masked for a long, long time due to the switch statement in assertResponsiveImageFieldFormattersLink().
Both of these test failures are legitimate, and they point to a bug in the way the image_link setting on the ResponsiveImageFormatter is handled when rendering responsive images.
Fixed that bug.

Next: an actual round of review.

Wim Leers’s picture

Issue tags: -Needs manual testing

I verified in both Chrome (which already supports <picture> natively) and Firefox (which needs the polyfill to work) that the perceived behavior is correct: the image is shown in the expected image style at each breakpoint. I also verified that *only* the necessary image is loaded (both at narrow and wide width), that upon resizing the additional necessary images are loaded, and that in no case an image is fetched twice.

Any configuration I created for the responsive image mapping (i.e. assign image styles to breakpoints), as well as for the responsive image formatter (i.e. select an image mapping, disable/enable linking to content/file, fallback image style).

EDIT: oh, and just like #197, I found the UI rather confusing, but as per #198: improving that is out of scope for this issue.

Next: that deep code review. That will probably take a while.

attiks’s picture

#210 Thanks for the reroll, changes to the polyfill should not happen.

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ 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 style to map to this
    +            # breakpoint).
    +            image_mapping_type:
    

    AFAICT this is a change
    1) to enable a follow-up issue to use sizes instead of <picture> with image styles, i.e. to not need any further upgrade path-breaking changes
    2) that is therefore only a config entity-level change, zero UI or test changes were made to respectively use this or test this explicitly.

    I think that's acceptable.

  2. +++ b/core/modules/responsive_image/config/schema/responsive_image.schema.yml
    @@ -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'
    +            sizes_image_styles:
    +              type: sequence
    +              label: 'Image styles to be used when using the ''sizes'' attribute'
    +              sequence:
    +                - type: string
    +                  label: 'Image style'
    

    All of this is only necessary if image_mapping_type === 'sizes'.

    Which makes this feel dirty. I think this should actually use dynamic typing using [%parent]. See the cheat sheet at https://www.drupal.org/node/1905070, that makes it much easier to get started with that.

    Apologies for not remarking that sooner, but I didn't know config schemas as well before.

    I would say that it's okay for this to happen in a follow-up, but since this is an upgrade path critical now, it should happen here.

  3. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -189,114 +186,255 @@ function theme_responsive_image($variables) {
    +  $image = \Drupal::service('image.factory')->get($variables['uri']);
    +  $sources = responsive_image_build_source_tags($image, $variables);
    +  $output = array();
    +  $output[] = '<picture>';
    +  if (!empty($sources)) {
    +    // Internet Explorer 9 doesn't recognise source elements that are wrapped in
    +    // picture tags. See http://scottjehl.github.io/picturefill/#ie9
    +    $output[] = '<!--[if IE 9]><video style="display: none;"><![endif]-->';
    +    foreach ($sources as $source) {
    +      $output[] = drupal_render($source);
    +    }
    +    $output[] = '<!--[if IE 9]></video><![endif]-->';
    +  }
    +  // Output the fallback image. Use srcset in the fallback image to avoid
    +  // unnecessary preloading of images in older browsers. See
    +  // http://scottjehl.github.io/picturefill/#using-picture and
    +  // http://scottjehl.github.io/picturefill/#gotchas for more information.
    +  $fallback_image = array(
    +    '#theme' => 'image',
    +    '#srcset' => array(
    +      array(
    +        'uri' => _responsive_image_image_style_url($variables['style_name'], $image->getSource()),
    +      ),
    +    ),
       );
    +  foreach (array('alt', 'title', 'attributes') as $key) {
    +    if (isset($variables[$key])) {
    +      $fallback_image["#$key"] = $variables[$key];
    +    }
    +  }
    +  $output[] = drupal_render($fallback_image);
    +  $output[] = '</picture>';
    

    What would make this much, much easier to understand, is if this would live in a twig template: picture.html.twig.

    Also, playing the devil's advocate; the function is called theme_responsive_image(), which implies it is the standard way of rendering responsive images. But this cannot render <img sizes>… so is that actually the appropriate name? Wouldn't theme_responsive_image_picture() or theme_picture() be more appropriate?

I think the fundamental question is: why do we have to support "sizes" in order to use picturefill? Isn't that itself merely a nice-to-have? Something that we could add in Drupal 8.1 or 8.2? Because if we could remove the introduction of "sizes" from this patch, this patch would become significantly easier, I think. AFAICT all of the complexity and confusion comes from the apparent need to support both simultaneously. This makes the config entity (+ schema) and the code even more abstracted and with even more branching than is already the case.
Assuming that's correct; it'd still allow us to upgrade to the latest version of Picturefill. It'd still allow us to ship Drupal 8.0, and then add support for 'sizes' in a later version, when we'll have had more hands-on experience with both <picture> and 'sizes'.

Finally, what would be most helpful, is if the IS is updated to contain a summary of the discussion about <picture> versus <img sizes> earlier in this issue. I think that might be *required* for a core committer to be able to commit this, since (s)he will have to understand this issue first, and reading throug all >200 comments is not going to be possible.

attiks’s picture

Issue summary: View changes
attiks’s picture

#212 <img sizes > is still on our todo list as mentioned in the IS

mdrummond’s picture

#212: I'd be very reluctant to pull whatever support we have for sizes at this point. The sizes (and srcset attribute that needs to be used with sizes) can be used either on source elements within a picture element or on an img element. The combination of sizes and srcset is by far the preferred method for the most common use case, which is providing different size image files at various breakpoints. 8.1 won't likely be out until 2016, my guess is very late 2016. So dropping sizes is the difference between saying "Drupal 8 has great support for responsive images!" and "Well, Drupal 8 has support for responsive images, yes, but not the recommended way that most people will want to use."

Supporting sizes may well be more complicated, but it's very important we do so. I'd be glad to write something in the issue summary about this.

As for a picture.html.twig template, there is a separate issue to do that #1898442: responsive_image.module - Convert theme_ functions to Twig that has been postponed based on this issue (amongst others) for ages. This issue is holding up the entire issue queue for the responsive images module, so I'm not thrilled about major changes to this issue like pulling sizes support or switching to a twig template (particularly when an issue already exists to do that). Those sorts of changes would likely set back this issue by months, quite frankly. I'd much rather us see fixing any minor remaining bugs in this patch and get it in.

Wim Leers’s picture

I'm not thrilled about major changes to this issue like pulling sizes support or switching to a twig template (particularly when an issue already exists to do that). Those sorts of changes would likely set back this issue by months, quite frankly. I'd much rather us see fixing any minor remaining bugs in this patch and get it in.

I completely share your sentiment. But please know that the only reason I said what I said is out of concern to get this issue in. The whole reason this hasn't gone anywhere for months is because it's so very difficult to review. My suggestions were to simplify the review, by simplifying the patch, and hence getting this patch in ASAP, so that all the other work is unblocked.

An improved IS should help with getting this in.

But at this point it's feeling more and more like "just commit this even if it's imperfect, so that we can make things more understandable in the follow-up issues that this blocks". Is that a fair assessment? I wonder if core committers would be willing to do that?

mdrummond’s picture

I'm not saying we should commit something imperfect or broken. Let's get this code to where it needs to be. However the point of the issue is to support the sizes attribute as part of the updated responsive images spec. So if we pull out support for sizes, then we're not actually solving the goals of this issue.

The other issues that this is blocking are based on old code that this issue cleans up, so the vast majority of other responsive images issues will simply go away.

We already do have a twig file for the source element in this patch, so if we need to convert the picture markup to twig in this issue to get this in, it's probably not the end of the world. The other twig conversion issue is based on the old responsive images code, so maybe it's cleaner to take care of the twig conversion here.

I want to make sure we're committing good, clean, understandable code here. Wim, I'd really appreciate your help to get us there. I know that sizes may add more complexity, but it's the heart of the goal of this issue.

Wim Leers’s picture

I see that we apparently already had this discussion in August last year :(

I think it'd be very helpful for all of us if we had an issue summary that very clearly explained *why* Drupal 8.0 must support sizes. I'd be very grateful if you could add that. I'm pretty certain that's going to be necessary for a core committer to be able to review/approve/commit this anyway, and it would greatly help getting this issue on track again :)

catch’s picture

For me I think we should either support the latest spec, or not ship with responsive image support at all, and bring it back in a minor release once the spec has settled down.

Shipping with support for an outdated spec (which will require data-model changes to upgrade to the new one) seems like the worst of all possible options.

attiks’s picture

#219 You're right, but shipping without picture support is not an option, it will drive people away from Drupal.

#218 What is missing from the IS, as said http://ericportis.com/posts/2014/srcset-sizes/ explains the importance of sizes, but if you need more information I'm happy to write more.

Let us know how we can make reviewing easier, if needed we pull in the issue about the twig template in this issue, but it means it will grow.

webchick’s picture

FWIW I'm a core maintainer and I understand why we need to support sizes to say we support responsive images. https://developers.google.com/web/fundamentals/media/images/images-in-ma... is a nice link with a TL;DR.

Basically, we can't reasonably say we support responsive images if we only support the "art direction" case, and not the "different images based on device capabilities/screen size" case, which is far more common.

So while yes, an updated issue summary would be welcome, I am a lot more interested in details about how we're planning to resolve this in Drupal and what's left to do before we can close this critical than I am documenting stuff I can look up on other sites. Agreed though that if this is one of the last criticals upgrade path blockers standing, we're probably going to have to end up ripping out Responsive Images module and hoping to restore it in a later point release. :\

attiks’s picture

So according to #212 the biggest thing left is changing the yaml config and using twig for all output, if I'm missing something, now is a good time to add it so we can rewrite it tomorrow.

#221Are you really considering shipping Drupal 8 without a solution for responsive images?

Update: typo

webchick’s picture

I really, really would not want to since that's one of the core staples of the mobile initiative. However, if it's between no responsive images or another N months in the way of supporting a D8 upgrade path, then yes, that's something we'd need to consider. A working D8 upgrade path is key to early adoption of Drupal 8 and getting a hand resolving the remaining ~70 criticals.

attiks’s picture

#223 Thinking out loud: Is it an option to rip everything out so the upgrade path isn't block and work can continue on the other 70 criticals and add it back before the official release?

webchick’s picture

Well, before we even go down that road, how close/far away is this from being RTBC? Because obviously if it lands in the next ~month or so we don't even need to be having this discussion.

attiks’s picture

#225 Next month should be doable unless i'm missing a lot, most work for the moment is the changes to the yaml file, once that is fixed this should be good to go and we can fix the follow up issues, which are minor ones.

I'll sit down with Jelle_S tomorrow to rewrite it.

mdrummond’s picture

attiks, thanks for working on this. I'll stay on top of this issue to provide reviews as necessary. Would really like to see us be able to ship with responsive images support, and I agree we need to support the latest spec.

One question, attiks, is it possible with the patch as is for somebody to output just an img element with sizes and srcset attributes? For the use case of switching image file sizes at various viewports, that's typically ideal. If there are multiple image formats, or for more complicated use cases like art direction, picture is definitely handy to have. Just wanted to check if will be possible to output an img element without a picture wrapper. If that's something we can take care of in a future follow-up issue (that would be acceptable to commit at this point), that's fine.

For your question on the spec settling down, catch, the spec is stable at this point with browsers getting their support inline. At this point the syntax will be unlikely to change. The only thing that will be likely to change is which browsers are supporting the syntax, and what percentage of those browser versions show up in typical site's user stats.

Thanks webchick for the feedback, for sharing that link and for working with us to provide guidelines on how we can get this work in.

Thanks for everybody's hard work on this!

attiks’s picture

#227 Outputting <img src="" scrset"" sizes=""> is possible, but we need to decide if we want to take the risk of double downloads, we're now always outputting picture with the fallback defined as srcset to avoid this.

Adding this is a small change: we just need to implement hook_theme_suggestions to make the switch and decide when to do it.

@Jelle_S is starting on the yml part

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
98.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,022 pass(es). View
28.73 KB

This patch only addresses the YAML part. I'll convert the theme function to a twig template in the next patch, but this might be easier to review the interdiff. The responsive image tests were green on my local machine and I didn't encounter any problems during manual testing.

Gábor Hojtsy’s picture

The config schema updates make sense so long as there is only ever one variant used, either image_style or sizes.

attiks’s picture

Jelle_S’s picture

FileSize
100.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,028 pass(es). View
4.08 KB

New patch with twig template replacing theme_responsive_image().

yched’s picture

Wouldn't it be simpler to leave generation of the html for each of the sources directly in responsive-image.html.twig, rather than going through a separate '#theme' => 'responsive_image_source' theme hook ?

Seeing the full

<picture>
  <source/>
  <source/>
</picture>

tree inside a single template might be a bit clearer.
We don't really have a need to generate <source> output outside of a <picture> tag, do we ?

Jelle_S’s picture

@yched: The source template was already in the patch. I think we decided on that earlier. I'm not sure what the reasoning behind it was. I'll try to find the comment/patch where it was added. It should be somewhere in this issue though.

Jelle_S’s picture

Ok so apparently it was in comments #147-#168

attiks’s picture

#232 I think it might make sense to combine them inside 1 twig file, I think we should add the img tag as well (if possible) to make it clear it is there, so something like if it makes sense?

<picture {{ attributes }} >
  {% 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]-->
    {% loop sources %}
      <source {{ source_attributes }} >
    {% end loop %}
    <!--[if IE 9]></video><![endif]-->
  {% endif %}
  <img {{ fallback_image_atributes }} >
</picture>
yched’s picture

Yep, #236 looks nice :-)

Jelle_S’s picture

Assigned: Unassigned » Jelle_S

on it.

yched’s picture

[edit: crosspost with #238]

re @Jelle_S: If I read it correctly, @Wim's remark in #147 was mostly about moving the logic for computing the source's attributes to a helper function.
That's still a valid organization IMO. Just, instead of a full blown theme hook, it simply would be a helper function called by template_preprocess_responsive_image() to generate the attributes array for a source ?

Seeing the full markup within a single template, as in @attiks' sample in #236, IMO really helps getting the big picture (HAH! Saw what I did ?)

mdrummond’s picture

#228: If we did output img with sizes and srcset, I would imagine we would drop the src element, just as we do for the controlling img in the picture element. It's the same tradeoffs: Picturefill will make sure the image displays if JS is working and there isn't native browser support for srcset/sizes. If there's no JS, an alt would display.

If we did provide that option, is that something that could be handled with a config entity? Would we require any schema changes to make that possible?

The twig work is looking good. I agree that having one template for picture/sources would help make things more clear.

Jelle_S’s picture

FileSize
98.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,032 pass(es). View
7.92 KB

#239: Hah! I see what you did there xD

I didn't completely follow the template in #236. The fallback image still goes through the separate image.html.twig template. Following the logic in #233: We use the image tag outside of the picture tag as well (!= source tag), so use that template.

Tests on my machine were green, no problems during manual tests.

Jelle_S’s picture

FileSize
98.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,996 pass(es). View
670 bytes

Forgot to update a comment in the twig template.

attiks’s picture

#240 The idea is to output a plain img with sizes and srcset if we can, dropping src should avoid the double download, but it might be a good idea to make it configurable (site wide). Do we add this in this issue or a separate one?

mdrummond’s picture

#243: I'm fine if we handle outputting an img with no picture/source elements in a follow-up issue if it speeds up getting this in. However I do think it's important that gets in, so if we can handle it here without too much trouble, maybe we should while we have some momentum.

In the responsive images UI issue, I had a suggestion for how to toggle this for an individual responsive image mapping, which I think would be more useful than a global configuration. However, that UI issue is definitely out of scope for this.

If we did the switch between img and picture output as a business rule, I would imagine it would be something like:

1) If there would be only one source element AND

2) There is no need for an image type attribute AND

3) The mapping type is sizes, THEN

4) Use an img element with no picture or source elements.

Does that sound about right?

attiks’s picture

#244 Approach is what we had in mind as well.

davidhernandez’s picture

+++ b/core/modules/responsive_image/templates/responsive-image.html.twig
@@ -0,0 +1,30 @@
+<picture {{ attributes }} >

Minor comment. You don't need the spaces on either side of the braces.

attiks’s picture

#246 So just like <picture{{attributes}}>?

Is there a 'coding style' for twig?

Jelle_S’s picture

Assigned: Jelle_S » Unassigned
FileSize
825 bytes
98.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,056 pass(es). View

Looking at https://www.drupal.org/node/1823416#attributes it should be

<picture{{ attributes }}>

Minor change, tests should still be green

davidhernandez’s picture

It looks like our doc page doesn't specifically describe it, but basically when attributes is printed it will provide the space it needs.

killerpoke’s picture

I just tested patch #248 and it works for me.

Testing process:

  1. install fresh drupal from 8.0.x
  2. set responsive image mapping for bartik theme and change news article display image to responsive image
  3. Responsive image works in Safari, but not latest Chrome
  4. Apply patch #248 and set the responsive image mapping again
  5. Responsive image now works in Safari and latest Chrome
attiks’s picture

#250 @killerpoke thanks for testing, good to know that updating will work as well. Any chance you can review the code as well?

killerpoke’s picture

At a first glance the code looks good to me. I have found a super small maybe-issue.

+++ b/core/modules/breakpoint/src/Tests/BreakpointDiscoveryTest.php
@@ -33,54 +33,54 @@ protected function setUp() {
-        'breakpoint_theme_test.mobile' => array(
-          'label' => 'mobile',
-          'mediaQuery' => '(min-width: 0px)',
-          'weight' => 0,
-          'multipliers' => array(
-            '1x',
-          ),
-          'provider' => 'breakpoint_theme_test',
-          'id' => 'breakpoint_theme_test.mobile',
-          'group' => 'breakpoint_theme_test',
-          'class' => 'Drupal\\breakpoint\\Breakpoint',
+      'breakpoint_theme_test.tv' => array(

+++ b/core/modules/responsive_image/responsive_image.module
@@ -113,8 +105,8 @@ function responsive_image_theme() {
+ *   - url: An optional \Drupal\Core\Url object.

add description for the url parameter

What do you think about this template? Is there an alternative way to do this by using feature detection and not browser detection?

+++ b/core/modules/responsive_image/templates/responsive-image.html.twig
@@ -0,0 +1,30 @@
+    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]-->

is there a better way to decide if video tag is used? It doesn't feel right to add specific IE9 detection to a core template.

attiks’s picture

#252 There's no other work around for IE9, sad but true.

yched’s picture

Yes, the IE9 thing is clearly indicated as the officially recommended way to use the picturefill lib, lots of smart people have spent time getting to that conclusion, IMO we should just bite the bullet and not spend too much time trying to be smarter.

(same for the "no fallback without JS", for that matter - that's just the way the picturefill works, we can decide it's not accpetable for us, but that would leave us without much options...)

Wim Leers’s picture

Status: Needs review » Needs work

Reviewing all interdiffs since my last comment… and the actual patch. Grab a drink, this is going to be long.

But, the good news: I no longer feel obligated to push back on this patch. I think it's now significantly easier to understand what's happening — the config schema improvements and the move towards Twig templates have done wonders IMHO.

In fact, I think this patch is now almost RTBC! I feel the code is now sufficiently understandable to be committed. Below are bunch of nitpicks, minor problems and one biggish problem. But they're all easily surmountable.

That only leaves #244 + #245 left to be done. mdrummond said he'd prefer if it happened in this issue. I'll defer to others to decide on that.


#229:

  1. +++ b/core/modules/responsive_image/config/schema/responsive_image.schema.yml
    @@ -26,26 +26,14 @@ responsive_image.mappings.*:
    +            image_mapping:
    +              type: responsive_image.image_mapping_type.[%parent.image_mapping_type]
    

    This change has made the config schema infinitely more understandable!

    It makes it clear that you cannot mix the two within one responsive image mapping.

    This also means we'll be able to simplify the code: the big switch statement should be simplifiable, it should be possible to move it out of the loop, which then mirrors the "you have to choose one of the two" strictness now enforced by the config schema.
    That should be within one responsive image mapping's mapping definition — see the bottom for why.

  2. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
    @@ -93,9 +93,7 @@ public function __construct(array $values, $entity_type_id = 'responsive_image_m
    +      'image_mapping' => '',
    

    AFAICT this should be array(), not ''.


#232:

  1. +++ b/core/modules/responsive_image/responsive_image.module
    --- /dev/null
    +++ b/core/modules/responsive_image/templates/responsive-image.html.twig
    
    +++ b/core/modules/responsive_image/templates/responsive-image.html.twig
    @@ -0,0 +1,28 @@
    + * Default theme implementation of a <picture> tag.
    

    Mismatch? Should IMO read:
    Default theme implementation of a responsive image.

  2. +++ b/core/modules/responsive_image/templates/responsive-image.html.twig
    @@ -0,0 +1,28 @@
    +<picture {{ attributes }} >
    +  {% 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]-->
    +    {{ sources }}
    +    <!--[if IE 9]></video><![endif]-->
    +  {% endif %}
    +  {{ fallback_image }}
    +</picture>
    

    BEAUTIFUL! This is SO much more legible and understandable!


#233#239: agreed on all counts, and yched++ for that wonderful pun :P


#241:

+++ b/core/modules/responsive_image/responsive_image.module
@@ -374,48 +359,6 @@ function responsive_image_build_source_tags(ImageInterface $image, $variables) {
-function template_preprocess_responsive_image_source(&$variables) {
-  $srcset = array();
-  foreach ($variables['srcset'] as $src) {
-    // URI is mandatory.
-    if (!isset($src['uri'])) {
-      throw new \LogicException('Key \'uri\' is required for each source in a srcset.');
-    }
-    $source = file_create_url($src['uri']);
-    if (isset($src['width']) && !empty($src['width'])) {
-      $source .= ' ' . $src['width'];
-    }
-    elseif (isset($src['multiplier']) && !empty($src['multiplier'])) {
-      $source .= ' ' . $src['multiplier'];
-    }
-    $srcset[] = $source;
-  }
-  $variables['attributes']['srcset'] = implode(', ', $srcset);
-  if (isset($variables['media']) && !empty($variables['media'])) {
-    $variables['attributes']['media'] = $variables['media'];
-  }
-  if (isset($variables['mime_type']) && !empty($variables['mime_type'])) {
-    $variables['attributes']['type'] = $variables['mime_type'];
-  }
-  if (isset($variables['sizes']) && !empty($variables['sizes'])) {
-    $variables['attributes']['sizes'] = $variables['sizes'];
-  }
-}

+++ /dev/null
@@ -1,15 +0,0 @@
-{#
-/**
- * @file
- * Default theme implementation of a <source> tag.
- *
- * Available variables:
- * - attributes: HTML attributes for the <source> tag.
- *
- * @see template_preprocess()
- * @see template_preprocess_responsive_image_source()
- *
- * @ingroup themeable
- */
-#}
-<source {{ attributes }} />

+++ b/core/modules/responsive_image/templates/responsive-image.html.twig
@@ -21,7 +21,9 @@
-    {{ sources }}
+    {% for source_attributes in sources %}
+      <source {{ source_attributes }} />
+    {% endfor %}

So thanks to a minor reorganization in responsive_image_build_source_tags(), now renamed to responsive_image_build_source_attributes(), we can delete all this code… WONDERFUL!

responsive-image.html.twig keeps getting better! :)


#244 + #245: sounds great!


Reviewing the actual patch:

  1. The config schema still looks a bit weird, but that's out of scope to fix here. I filed #2419857: Responsive image and View mode schemas should use the config_entity type, can then be simpler (with patch). I also noticed it doesn't declare dependencies on the image styles it uses yet, but that's also out of scope, and being fixed in #2383165: ResponsiveImageStyle config entities should depend on the image styles they use.
  2. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -189,114 +179,192 @@ function theme_responsive_image($variables) {
    +  // Output the fallback image. Use srcset in the fallback image to avoid
    

    Since this is now just a preprocess function, this is no longer outputting anything.

    So: s/Output/Prepare/

  3. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -189,114 +179,192 @@ function theme_responsive_image($variables) {
    +}
    +/**
    + * Helper function for template_preprocess_responsive_image().
    

    Missing blank line.

  4. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -189,114 +179,192 @@ function theme_responsive_image($variables) {
    + * In a responsive image mapping, each breakpoint has a mapping defined for each
    + * of its multipliers. A mapping can be either of two types: 'sizes' (meaning
    
    1. Hrm… so it's still true that each individual mapping (i.e. for a given breakpoint) can be of either type…? Is that a true statement? The schema at least still supports this (i.e. what I initially wrote above, in my interdiff reviews, was wrong — now correcte).

      It'd be great if there were a practical example of that mixing of both 'sizes' and 'image_style' within a single <picture>, because that'd remove any confusion.

    2. Looking at http://scottjehl.github.io/picturefill/, I also see that it's possible for the sizes attribute to specify on a <picture>'s <source> tag:

      Your picture element should contain a series of source elements followed by an img element. Each source element must have a srcset attribute specifying one or more image url sources (which can use expanded srcset syntax if desired for resolution switching), and the img element should have a srcset attribute for fallback purposes as well (some browsers like Android 2.3's won't see the source elements). Additionally, you may add a media attribute containing CSS3 Media Queries, and/or a sizes attribute to pair with srcset.

      (Emphasis mine.)
      It'd be great if there were an example of there too.

  5. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -189,114 +179,192 @@ function theme_responsive_image($variables) {
    +              // Add the image source with its width descriptor. When a width
    ...
    +              $srcset[floatval($dimensions['width'])] = file_create_url(_responsive_image_image_style_url($image_style_name, $image->getSource())) . ' ' . $dimensions['width'] . 'w';
    +              $sizes = array_merge(explode(',', $mapping_definition['image_mapping']['sizes']), $sizes);
    

    The "when" here indicates conditionality, but this is not happening conditionally.

  6. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
    @@ -89,18 +90,25 @@ public function __construct(array $values, $entity_type_id = 'responsive_image_m
           if ($mapping['breakpoint_id'] === $breakpoint_id && $mapping['multiplier'] === $multiplier) {
    -        $mapping['image_style'] = $image_style;
    +        $mapping = array(
    +          'breakpoint_id' => $breakpoint_id,
    +          'multiplier' => $multiplier,
    +        ) + $mapping_definition + $defaults;
    

    This is a bit strange. First, we check if $mapping's 'breakpoint_id' and 'multiplier' keys have a certain value, then we set them again. This didn't happen before. Why?

  7. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
    @@ -109,7 +117,8 @@ public function addMapping($breakpoint_id, $multiplier, $image_style) {
    -    return !empty($this->mappings);
    +    $mappings = $this->getKeyedMappings();
    +    return !empty($mappings);
    

    Hehe. Stupid PHP :P

  8. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
    @@ -119,7 +128,13 @@ public function getKeyedMappings() {
    +          // Only return the selected image styles.
    +          if ($mapping['image_mapping_type'] == 'sizes') {
    +            $mapping['image_mapping']['sizes_image_styles'] = array_filter($mapping['image_mapping']['sizes_image_styles']);
    +          }
    

    I don't understand why the if-test is necessary.

  9. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
    @@ -128,16 +143,6 @@ public function getKeyedMappings() {
         return $this->get('mappings');
    
    @@ -182,4 +187,38 @@ public function calculateDependencies() {
    +  public function getMappingDefinition($breakpoint_id, $multiplier) {
    +    $map = $this->getKeyedMappings();
    +    if (isset($map[$breakpoint_id][$multiplier])) {
    +      return $map[$breakpoint_id][$multiplier];
    +    }
    +  }
    

    I'd question the value of this helper method, because it introduces more ambiguity than it helps writing code.

    There is only one call to this outside of tests. Why not just use getKeyedMappings() directly?

    The reason I think this introduces too much ambiguity: we already have the following mapping-getters:

    1. getMappings()
    2. getKeyedMappings()

    The first returns the mappings as stored in the config entity. This makes sense.

    The second returns the mappings keyed by breakpoint first, multiplier second. This makes sense when rendering.

    This new one, getMappingDefinition() says "mapping definition". So is that like getMappings() or like getKeyedMappings()? That's the first ambiguity/confusion. Second, what does that even mean? It's not used elsewhere. After having written the above, I now see that this is being used elsewhere in the code too, and it's used with the same meaning.

    So… I think what is missing then is a mismatch between the config schema and the code/interface. The notion of a "mapping definition" should be embodied/captured in the config schema too.

    I think we want to change the mappings key to mapping_definitions, the 'Mapping(s)' labels to 'Mapping definition(s)', and the getMappings() and getKeyedMappings() to getMappingDefinitions() and getKeyedMappingDefinnitions() respectively.

    Alternatively… rename this method to just getMapping() and replace all variable names in the module. But I think "mapping definition" is a better name overall, because the config entity already is a "responsive image mapping", if that itself contains "mappings", then that's more confusing than if it contained "mapping definitions".

    Anyway, I'm fine with either solution. As long as everything is internally consistent. It's these sort of seemingly small inconsistencies that make code hard to follow, because they are unconsciously ignored by those familiar with the code, but thoroughly confuse others reading the code.

  10. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
    @@ -182,4 +187,38 @@ public function calculateDependencies() {
    +          if ($mapping_definition['image_mapping']['sizes'] && array_filter($mapping_definition['image_mapping']['sizes_image_styles'])) {
    

    Why array_filter() and not count()?

    (Also elsewhere in the patch.)

attiks’s picture

@Wim Leers Thanks for reviewinf

#255

Hrm… so it's still true that each individual mapping (i.e. for a given breakpoint) can be of either type…? Is that a true statement?

Yes it is true, it is an edge case but it should be allowed.

5. // Add the image source with its width descriptor. When a width

This is a tricky one, normally we always get the dimensions of the derivative, but since core does not enforce it (base class returns NULL for width and height) we have to check it. The tricky part is in the spec of srcset, I quoted the important parts:

If a source element has a sizes attribute present or an img element has a sizes attribute present, all image candidate strings for that element must have the width descriptor specified.
If an image candidate string for an source or img element has the width descriptor specified, all other image candidate strings for that element must also have the width descriptor specified.

The question remains what our code has to do:

  • Log a warning
  • Throw an exception
  • Ignore the error and not output the image
Jelle_S’s picture

2. No. Since the schema changed a bit, the structure changed a bit as well.

The value for the 'image_mapping' key is now either an array or a string, which is documented in ResponsiveImageMappingInterface:

   *   The mapping definition has following keys:
   *     - image_mapping_type: Either 'image_style' or 'sizes'.
   *     - image_mapping:
   *       - If image_mapping_type is 'image_style', the image style ID.
   *       - If image_mapping_type is 'sizes', an array with following keys:
   *         - sizes: The value for the 'sizes' attribute.
   *         - sizes_image_styles: The image styles to use for the 'srcset'
   *           attribute.
   *     - breakpoint_id: The breakpoint ID for this mapping definition.
   *     - multiplier: The multiplier for this mapping definition.
Jelle_S’s picture

4.1: I think you might be a bit confused:
A ResponsiveImageMapping is a group of mappings: It contains several breakpoints with a mapping for each breakpoint (== "mapping definition").
A mapping definition is for one single breakpoint, and thus of one type ('sizes' or 'image_style').
The various mapping definitions within a ResponsiveImageMapping are not necessarily of the same type.
Example from the tests:

$this->responsiveImgMapping
        ->addMapping('responsive_image_test_module.mobile', '1x', array(
          'image_mapping_type' => 'image_style',
          'image_mapping' => RESPONSIVE_IMAGE_EMPTY_IMAGE,
        ))
        ->addMapping('responsive_image_test_module.narrow', '1x', array(
          'image_mapping_type' => 'sizes',
          'image_mapping' => array(
            'sizes' => '(min-width: 700px) 700px, 100vw',
            'sizes_image_styles' => array(
              'large' => 'large',
              'medium' => 'medium',
            ),
          ),
        ))
        ->addMapping('responsive_image_test_module.wide', '1x', array(
          'image_mapping_type' => 'image_style',
          'image_mapping' => 'large',
        ))
        ->save();

So $this->responsiveImgMapping contains 3 mapping definitions:

  1. 'responsive_image_test_module.narrow', '1x': mapping of type 'image_style' (also notice: the 'image_mapping' key is a string here)
  2. 'responsive_image_test_module.narrow', '1x': mapping of type 'sizes' (also notice: the 'image_mapping' key is an array here)
  3. 'responsive_image_test_module.wide', '1x': mapping of type 'image_style' (again, the 'image_mapping' key is a string here)

==> 3 mapping defintions of 2 different types withing one ResponsiveImageMapping.

4.1 & 4.2: There are examples in the tests. Mapping definitions provided in the tests (like the one above) will result in source tags with the sizes attribute (for the mapping definitions of image_mapping_type sizes) the other mapping definitions (of type image_style) will result in source tags without a sizes attribute.
The sizes attribute can only be present on <img> tags or <source> tags within <picture> tags

6. Because before, we didn't support the sizes attribute, so all we had to do was overwrite the 'image_style' value. Now we have to overwrite the whole mapping definition.

8. Because if 'mapping_type' is not 'sizes' (and thus 'image_style'), $mapping['image_mapping'] will be a string (the image style id to map to) and $mapping['image_mapping']['sizes_image_styles'] will not be set.

10. That's a leftover from when there was a UI (maybe it should be solved before saving?):
Because of the way Drupal works with checkboxes this array might look like:

array(
  'large' => 'large',
  'medium' => 0,
  'small' => 'small',
);

count() will return 3 here and array_filter() returns an array with 2 elements, which would be no big deal, because both evaluate to TRUE. But if the array were to look like:

array(
  'large' => 0,
  'medium' => 0,
  'small' => 0,
);

count() would result in 3, but array_filter() in an empty array.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
102.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,862 pass(es), 32 fail(s), and 5 exception(s). View
20.8 KB

I think this patch covers #255.

Jelle_S’s picture

FileSize
104.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,858 pass(es), 32 fail(s), and 5 exception(s). View
5.04 KB

Forgot a few renames.

Jelle_S’s picture

FileSize
3.33 KB
26.09 KB
105.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,994 pass(es). View

Ugh, config schema's confuse me... + Some more renames

Wim Leers’s picture

Status: Needs review » Needs work

#256: I'd say it must throw an exception, so that the user/developer is informed that a particular image style they're using does not return a width as it should, and hence cannot be used for generating responsive images.


#257: ahhh! Right! That makes sense :) But to avoid confusion for future readers, could you perhaps add an inline comment to the code I cited to explain that that is why you're assigning the empty string?


#258:

  • 4.1 A: That actually is how I understood it :) Probably I didn't word it very clearly.
  • 4.1 B: It'd be great if that sample test you cited actually documented that the combining of different image mapping types was what it was testing! Also: thanks for pointing that out, that makes things even clearer.
  • 4.2: I understand now that there already is test coverage demonstrating that my point 4.2 is supported, but it'd be great if the docs for responsive_image_build_source_attributes() were updated to also include a concrete example of this — that is what I was asking. Sorry for the confusion.
    Because right now, the fact that this is supported is buried in the following two assertions:
          $this->assertRaw('sizes="(min-width: 700px) 700px, 100vw"');
          $this->assertPattern('/media="\(min-width: 560px\)".+?sizes="\(min-width: 700px\) 700px, 100vw"/');
    

    Having it in the docs of responsive_image_build_source_attributes() would be much more discoverable, because people wanting to learn how the Responsive Image module works would surely end up reading the docs of that function, since it explains so much, since generating the right attributes is the bulk of the complexity of the module :)

  • 6: Ok.
  • 8: But this is getKeyedMappings(), i.e. "return the mappings, keyed by breakpoint ID and multiplier". Then why is any processing necessary at all? It should be up to the caller of getKeyedMappings(), the one doing something with its return value to do any sort of processing/filtering/whatever.
  • 10: Eh… wow, yes, we definitely want this filtering to happen before saving. Otherwise, the config schema does not match the data being stored, for it'd be a mapping, not a sequence. And yes, '#type' => 'checkboxes' is annoying like that. You'll find this pattern in several places in core, and it's what you'll want to use too:
        // Transform the #type = checkboxes value to a numerically indexed array.
        $contexts = $form_state->getValue(array('cache', 'contexts'));
        $form_state->setValue(array('cache', 'contexts'), array_values(array_filter($contexts)));
    

    We'll also want test coverage to ensure that verifies an actual sequence gets stored, and not the data in the form you cited.


#259:

+++ b/core/modules/responsive_image/responsive_image.module
@@ -330,7 +331,12 @@ function responsive_image_build_source_attributes(ImageInterface $image, $variab
+              else {
+                \Drupal::logger('responsive_image')->warning('Could not determine image width for @file using image style with ID: @image_style_name', array('@file' => $image->getSource(), '@image_style_name' => $image_style_name));
+              }

So you went for a warning, as it was one of the three things attiks proposed in #256.

It looks like you chose to omit those images? The warning should say that too then.

I prefer exceptions, because this will likely result in an absolute avalanche of log messages, site admins will be completely flooded. And it'll appear broken. I don't think they will think of checking their logs.

If I'm wrong in this appearing broken, then it's probably acceptable to with logged warnings.

What do others think?


#260:

+++ b/core/modules/responsive_image/config/schema/responsive_image.schema.yml
@@ -10,11 +10,11 @@ responsive_image.mappings.*:
-    mapping_definitions:
+    mappingDefinitions:

We don't use camelCase in config schemas.

… which also means that we should rename breakpointGroup to breakpoint_group in the config schema. Doing that breaks the upgrade path, so should probably happen here. Sorry :(


#262: no remarks.

attiks’s picture

#264

Regarding the exception versus logger: the problem with an exception is that it will break the site for anonymous users. Let's say I have a contrib module with image effects, and for some reason that module no longer returns dimensions, if the site is updated by a site administrator everything will be fine, the error will only show the moment a page using responsive images is rendered. But it can be that this is done by a visitor, not the site admin.

Proposal: change warning to error?

Jelle_S’s picture

#264

  • #257: Will do.
  • #258:
  • 4.1 B: I'll add some documentation to the test.
    4.2: I'll add it to the docs.
    8. This will actually be solved by 10, but since there is no UI for sizes, there's no need for it. Or should this happen explicitly before saving (which means we won't assume the UI will solve this as you described in 10 before saving)?
    10. Thanks for the tip. I'll keep that in mind!

  • #260: Will fix. I wasn't sure which had the highest priority in the coding standards: no camelCase in config schemas, or camelCase for properties.
Wim Leers’s picture

#265: I would say that >90% of the time (hopefully more often, but…), people test changes to their site before putting it in production (or keeping it in production). In that case, an exception is significantly better, because the problem is immediately obvious, whereas a log entry is far less clear.

All that being said… the entire reason this is a problem, is because \Drupal\image\ImageEffectBase does this:

  /**
   * {@inheritdoc}
   */
  public function transformDimensions(array &$dimensions) {
    $dimensions['width'] = $dimensions['height'] = NULL;
  }

But this is an abstract base class. Why can't it just choose to not implement this method, hence forcing every single image effect to implement this, or at least consciously choose to set the dimensions to NULL? Right now, it's very easy to create an image effect that subclasses that abstract base class, but to forget to override the default transformDimensions() implementation from the abstract base class.

Is this even necessary? It's undocumented in Drupal 8 why you would ever need/want to return NULL.

So I did a bit of archeology. This was introduced in #1821854: Convert image effects into plugins (commit a48f0d88) and it seems zero consideration was given to this… because it was actually just moving code around. This funny $dimensions['width'] = $dimensions['height'] = NULL; business originates from the ImageStyle class (see it in its full glory using git show a48f0d88^:core/modules/image/lib/Drupal/image/Plugin/Core/Entity/ImageStyle.php). That in turn points to commit 27f8cd4c/#2027423: Make image style system flexible:

$ git blame -L292,+18 a48f0d88^ -- core/modules/image/lib/Drupal/image/Plugin/Core/Entity/ImageStyle.php
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 292)   public function transformDimensions(array &$dimensions) {
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 293)     module_load_include('inc', 'image', 'image.effects');
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 294) 
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 295)     if (!empty($this->effects)) {
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 296)       foreach ($this->effects as $effect) {
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 297)         if (isset($effect['dimensions passthrough'])) {
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 298)           continue;
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 299)         }
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 300) 
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 301)         if (isset($effect['dimensions callback'])) {
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 302)           $effect['dimensions callback']($dimensions, $effect['data']);
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 303)         }
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 304)         else {
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 305)           $dimensions['width'] = $dimensions['height'] = NULL;
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 306)         }
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 307)       }
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 308)     }
27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 309)   }

And that then came from image.module/image_style_transform_dimensions(), unchanged, so we need to dig further:

$ git blame -L568,+1 27f8cd4c^ -- core/modules/image/image.module
c16a978c (Dries 2012-03-23 16:21:49 -0600 568)         $dimensions['width'] = $dimensions['height'] = NULL;

… before I went further, I figured I'd look at the docs for D7's image_style_transform_dimensions(), which also doesn't explain it. But the first comment there confirms what I thought: some image effects make it impossible to transform dimensions. Though it's not fully clear to me why. I'm guessing it's because we're transforming dimensions without actually applying the image effects, i.e. it's more about calculating the expected dimensions — actually generating the image style to just get the dimensions is considered too expensive?

Conclusion: I don't see why this needs to be part of the abstract base class. Removing that implementation will minimize the problem. And so far, I still think it'd be better to throw an exception. Unless other people agree with attiks :)


  • #266.#258.8: (hahaha, this nesting is getting ridiculous, but it does help! :P) Right, good point! Even though the UI doesn't support it yet, I think we at least would want to add a @todo to \Drupal\responsive_image\ResponsiveImageMappingForm::validate(), referring to the example in \Drupal\Core\Block\BlockBase::validateConfigurationForm(), to ensure we don't forget to do it. That will allow us to remove those array_filter() calls.
  • #266.#260: Yes, that is very confusing. I asked @Gábor Hojtsy and he pointed me to #2345097: Fix ThirdPartySettingsTrait property capitalization + #2335287: [policy, no patch] Class properties don't always follow coding standards, where you could say that precedents were ruled in favor of consistency with config schemas over consistency with coding standards, hence approving code style violations for config entity properties to not be camelCase.
Jelle_S’s picture

attiks’s picture

Wim Leers’s picture

#268: let me file an issue + patch for it, then you can review it.

EDIT: attiks beat me to it :P

attiks’s picture

Since @Wim Leers is going to fix #2420751: ImageEffectBase::transformDimensions() should have a sane default implementation we'll throw an exception, because AFAIK all dimension are calculatable.

Wim Leers’s picture

#271: patch is up there. All dimensions can be calculated, with a sole exception: when you use the rotation image effect with the "random rotation" setting enabled. But I can't imagine that's a very frequently used one. And even then… we could make it "deterministically random" if we'd want to: we could take the hash of the file name and map that to a float/integer between 0 and 360, and voila: deterministically random.

mdrummond’s picture

I just wanted to follow-up that on further review, we do not need to create business logic in this issue to fall back from a one-source picture element with sizes/srcset but not media query to an img with sizes/srcset. I checked with the responsive images community group that developed and is implementing the spec, and those two are functionally the same. An img involves less markup, but it works the same. So it's not critical to fallback to an img, so let's not do that here. It's a nice-to-have that could go into a follow-up issue.

Thanks for the deep code review Wim!

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
110.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,113 pass(es). View
18.28 KB

I think I covered everything in the comments since #262. Patch was green on my local machine.

Wim Leers’s picture

Status: Needs review » Needs work

We're really getting there! :)


Reviewing the #274 interdiff:

  1. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -207,16 +208,50 @@ function template_preprocess_responsive_image(&$variables) {
    + * $responsiveImgMapping = entity_create('responsive_image_mapping', array(
    

    Nit: ResponsiveImageMapping::create(array(

  2. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -207,16 +208,50 @@ function template_preprocess_responsive_image(&$variables) {
    + * The above responsive image mapping will result in a <picture> tag similar to
    + * this:
    

    Nit: s/similar to this/like this/, also allows it to fit on a single line.

  3. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -207,16 +208,50 @@ function template_preprocess_responsive_image(&$variables) {
    + *   <source media="(min-width: 560px)" sizes="(min-width: 700px) 700px, 100vw" srcset="sites/default/files/styles/large/image.jpeg 480w, sites/default/files/styles/medium/image.jpeg 220w"/>
    

    Nit: Missing space before the trailing slash.

  4. +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
    @@ -119,10 +119,12 @@ protected function addTestMappings($empty_styles = FALSE) {
    +        // Test the output of an empty image.
    ...
    +        // Test the output of the 'sizes' attribute.
    
    @@ -133,6 +135,7 @@ protected function addTestMappings($empty_styles = FALSE) {
    +        // Test the normal output of mapping to an image style.
    
    @@ -232,10 +235,13 @@ protected function doTestResponsiveImageFieldFormatters($scheme, $empty_styles =
    +      // Assert the empty image is present.
    ...
    +      // Assert the output of the breakpoints.
    ...
    +      // Assert the output of the 'sizes' attribute.
    

    Thanks for these, they're super helpful!


And another review of the entire patch:

  1. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -189,114 +180,229 @@ function theme_responsive_image($variables) {
    + * The data structure we use to store the mapping information is a
    + * multidimensional array. The first level is the breakpoint, the second level
    + * is the multiplier. Since the specs do not allow width descriptors
    

    This data structure description is not actually accurate; it describes the structure of the return value of ::getKeyedMappingDefinitions().
    I don't think we need to describe the data structure in here; the config schema describes the data structure already.

    Plus, what follows (Since the specs […]) seems completely unrelated to the data structure? It feels like it should become something like: Note: Since the specs […], i.e. a description of a tricky aspect in the implementation, a caveat that justifies some of the complexity in the code.

  2. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -189,114 +180,229 @@ function theme_responsive_image($variables) {
    + * breakpointA.2x. If we would output those we will end up with something like
    + * @code
    + * <source [...] sizes="(min-width: 40em) 80vw, 100vw" srcset="a1.jpeg 300w 1x, a2.jpeg 600w 1x, a3.jpeg 1200w 1x, b1.jpeg 300w 2x, b2.jpeg 600w 2x, b3.jpeg 1200w 2x" />
    + * @endcode
    + * which is illegal. So the solution is to merge both arrays into one and
    + * disregard the multiplier. Which, in this case, would output
    + * @code
    + * <source [...] sizes="(min-width: 40em) 80vw, 100vw" srcset="image1.jpeg 300w, image2.webp 600w, image3.jpeg 1200w" />
    + * @endcode
    

    Why are it a(1,2,3).jpg and b(1,2,3).jpg in the first example and image(1,2,3).jpg in the second example? shouldn't that be either of the two sets of images in the earlier example? Because of this change of images, the merge both arrays into one and disregard the multiplier part is actually not clear.

  3. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -189,114 +180,229 @@ function theme_responsive_image($variables) {
    + * @param ImageInterface $image
    

    Needs to be fully qualified.

    (i.e. \Drupal\Core\Image\ImageInterface)

  4. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -328,17 +429,38 @@ function responsive_image_get_image_dimensions($variables) {
    -    return 'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7';
    +    // http://probablyprogramming.com/2009/03/15/the-tiniest-gif-ever
    +    return 'data:image/gif;base64,R0lGODlhAQABAIABAP///wAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==';
    

    Hrm; this new one is actually bigger than the previous one? :P That's odd.

    Not sure how I missed that all this time.

  5. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
    @@ -56,28 +57,35 @@ class ResponsiveImageMapping extends ConfigEntityBase implements ResponsiveImage
    +   *     - If image_mapping_type is 'image_style', the image style ID.
    

    Nit: s/the image style ID/the image style ID (a string)./, just like the patch does elsewhere.

  6. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
    @@ -56,28 +57,35 @@ class ResponsiveImageMapping extends ConfigEntityBase implements ResponsiveImage
    +   *       - sizes: If image_mapping_type is 'sizes', the value for the
    +   *         'sizes' attribute.
    
    +++ b/core/modules/responsive_image/src/ResponsiveImageMappingInterface.php
    @@ -20,16 +20,26 @@
    +   *         - sizes: If image_mapping_type is 'sizes', the value for the
    +   *           'sizes' attribute.
    
    @@ -62,17 +73,39 @@ public function setBreakpointGroup($breakpoint_group);
    +   *         - sizes: If image_mapping_type is 'sizes', the value for the
    +   *           'sizes' attribute.
    

    Nit: 'sizes' fits on the preceding line.

  7. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
    @@ -89,68 +97,73 @@ public function __construct(array $values, $entity_type_id = 'responsive_image_m
    +    $defaults = array(
    +      'image_mapping_type' => 'image_style',
    +      'image_mapping' => '',
    +    );
    ...
    +        ) + $mapping_definition + $defaults;
    ...
    +    ) + $mapping_definition + $defaults;
    

    Actually… are these defaults even necessary? Wouldn't config schema validation catch these problems right away?

    I tried, and removing this still allows all responsive image tests to pass.

  8. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
    @@ -89,68 +97,73 @@ public function __construct(array $values, $entity_type_id = 'responsive_image_m
    -  public function hasMappings() {
    -    return !empty($this->mappings);
    +  public function hasMappingDefinitions() {
    +    $mappings = $this->getKeyedMappingDefinitions();
    +    return !empty($mappings);
       }
    

    Why is the old logic now insufficient?

    I think because a mapping definition itself could be empty?

    If so: fine, this change is ok I think.

  9. +++ b/core/modules/responsive_image/src/ResponsiveImageMappingForm.php
    @@ -105,10 +105,18 @@ public function form(array $form, FormStateInterface $form_state) {
    +        $form['keyed_mappings'][$breakpoint_id][$multiplier]['image_mapping_type'] = array(
    +          '#type' => 'value',
    +          '#value' => 'image_style',
    +        );
    

    Perhaps add a @todo that this is only temporarily hardcoded, until support for the other iamge mapping type is added in #2334387: UI changes to support current responsive image standards?

  10. +++ b/core/modules/responsive_image/src/ResponsiveImageMappingInterface.php
    @@ -62,17 +73,39 @@ public function setBreakpointGroup($breakpoint_group);
    +   *   The mapping definition array.
    
    @@ -81,18 +114,18 @@ public function getImageStyle($breakpoint_id, $multiplier);
    +   *   The mapping definition array.
    

    Nit: I think "array" can be omitted here; it's implied by the type hint, and you're talking about the concept.

  11. +++ b/core/modules/responsive_image/src/ResponsiveImageMappingInterface.php
    @@ -81,18 +114,18 @@ public function getImageStyle($breakpoint_id, $multiplier);
    +   * @param string $mapping_definition
    

    s/string/array/

  12. +++ b/core/modules/responsive_image/templates/responsive-image.html.twig
    @@ -0,0 +1,30 @@
    +<picture{{ attributes }}>
    +  {% 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 %}
    +  {{ fallback_image }}
    +</picture>
    

    I want to stress again how beautiful this is! <3

cosmicdreams’s picture

Is the fallback_image being set? it doesn't seem so.

We are still not getting an <img src="fallback_image_url" /> at the end. And it critical that we get one. The spec requires it and the picture tag doesn't work without it.
see: http://www.w3.org/html/wg/drafts/html/master/embedded-content.html#the-p...

The tests might not be breaking but this feature is broken. I can't get any images to appear when using responsive images. Manually test it, maybe it's just me.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
110.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
7.83 KB

#275.4: See #94 Chrome claimed it was a bigger size... That's why I opted for the other one. Not sure why Chrome seems to think it was bigger?

#275.6: In ResponsiveImageMappingInterface it doesn't (by 1 character) because there is more indentation.

#275.7: Whoops, yep you're right.

#275.8: Indeed.

Comments from #275 should all be addressed.

Jelle_S’s picture

FileSize
110.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,130 pass(es). View
925 bytes

Focus Jelle!

Jelle_S’s picture

FileSize
111.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,143 pass(es). View
1006 bytes

Came up with manual testing. Forgot a small rename.

RE #276: Output looks fine here:

<picture>
          <!--[if IE 9]><video style="display: none;"><![endif]-->
          <source srcset="http://picture.dev/sites/default/files/styles/large/public/field/image/aDwdN5B_460s.jpg?itok=ZIpIvkBE 1x" media="all and (min-width: 851px)" type="image/jpeg" sizes="">
          <source srcset="http://picture.dev/sites/default/files/styles/medium/public/field/image/aDwdN5B_460s.jpg?itok=tbqMYzqS 1x" media="all and (min-width: 560px) and (max-width: 850px)" type="image/jpeg" sizes="">
          <source srcset="http://picture.dev/sites/default/files/styles/thumbnail/public/field/image/aDwdN5B_460s.jpg?itok=kG-z7zqx 1x" media="(min-width: 0px)" type="image/jpeg" sizes="">
        <!--[if IE 9]></video><![endif]-->
    <img srcset="http://picture.dev/sites/default/files/styles/thumbnail/public/field/image/aDwdN5B_460s.jpg?itok=kG-z7zqx" alt="" typeof="foaf:Image">

</picture>
Wim Leers’s picture

#276: Woah, good catch! (Haven't tested manually in the last iteration; that's for when I'm about to RTBC.)

Looking at the patch and the existing code, there are tests asserting some aspects of the rendered output, but none of them are comparing to a (mostly) hardcoded string, which is what we usually want in these cases I think.

This comment suggests there is at least some gap in the test coverage (unless @cosmisdreams forgot to clear caches or something like that).


#277:

  1. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -298,11 +297,8 @@ function template_preprocess_responsive_image(&$variables) {
    + * Note: Since The specs do not allow width descriptors and multipliers combined
    

    s/Since The/Since the/

  2. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -316,17 +312,17 @@ function template_preprocess_responsive_image(&$variables) {
    - * <source [...] sizes="(min-width: 40em) 80vw, 100vw" srcset="image1.jpeg 300w, image2.webp 600w, image3.jpeg 1200w" />
    + * <source [...] sizes="(min-width: 40em) 80vw, 100vw" srcset="b1.jpeg 250w, a1.jpeg 300w, a2.jpeg 600w, b2.jpeg 680w, a3.jpeg 1200w,  b3.jpeg 1240w" />
    

    That's very different from what was there before, but it's also significantly easier to understand/more logical.

    Can you point to where in the code it is calculated that b1.jpeg would get 250w instead of 300w?


#280: that points to another gap in test coverage AFAICT?


Leaving at NR because this could use still more feedback, can't find many problems anymore, except for the apparent lacks in test coverage of the generated markup.

Jelle_S’s picture

#291.#276: Nope, the fallback image is explicitly tested (ResponsiveImageFieldDisplayTest.php line 258)
#291.#277.2: That depends on which image style it is mapped to. The image style determines the width of the image/source.
#291.#280: I' writing the test as we speak.

Testing the picture tag against a hardcoded string is pretty hard because it depends on many variables. I think the current tests pretty much cover everything

cosmicdreams’s picture

#291 I did clear cache through admin/development/performance but not through drush (I have the wrong version of drush) Let me see if I can clear cache a different way so I can do a proper test.

cosmicdreams’s picture

Tested on simplytest.me, works great!

<div property="schema:image" class="field-item"><picture>
          <!--[if IE 9]><video style="display: none;"><![endif]-->
          <source srcset="http://s8b2251e636fa0a2.s3.simplytest.me/sites/default/files/styles/large/public/field/image/bg.jpg?itok=BoLMoSRp 1x" media="all and (min-width: 851px)" type="image/jpeg" sizes="">
          <source srcset="http://s8b2251e636fa0a2.s3.simplytest.me/sites/default/files/styles/medium/public/field/image/bg.jpg?itok=Q5-eEDDS 1x" media="all and (min-width: 560px) and (max-width: 850px)" type="image/jpeg" sizes="">
          <source srcset="http://s8b2251e636fa0a2.s3.simplytest.me/sites/default/files/styles/thumbnail/public/field/image/bg.jpg?itok=EleYxQkn 1x" media="(min-width: 0px)" type="image/jpeg" sizes="">
        <!--[if IE 9]></video><![endif]-->
    <img srcset="http://s8b2251e636fa0a2.s3.simplytest.me/sites/default/files/styles/thumbnail/public/field/image/bg.jpg?itok=EleYxQkn" alt="mirror kitty" typeof="foaf:Image">

</picture>
</div>

Must have been something wrong with my setup.... OH DANG, yep codebase and site are in different folders. sry my bad.

On a positive note, this patch does an excellent job of handling all the problems that led me to track this issue down.

Jelle_S’s picture

FileSize
116.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,209 pass(es). View
6.4 KB

Added tests for the formatter UI.

yched’s picture

Yay, awesome to see such fast progress here ! Really sorry I couldn't help more with the review, @wim++++++

Just a nitpick :

+++ b/core/modules/responsive_image/templates/responsive-image.html.twig
@@ -0,0 +1,30 @@
+  {{ fallback_image }}

This var name is misleading, the "fallback" part is the srcset in the img tag, but the img tag itself is an integral, required part of the <picture> group, without which nothing would ever get displayed at all - at least that's how I understood picture & source work so far ?

(Just like I always heard "do not apply CSS on the <picture> tag, always style the <img> tag, it's the only one that has an actual display" - which makes me wonder if we pass attributes to the right tag currently - img rather that the picture tag ?)

davidhernandez’s picture

I agree that name is bit misleading. Going to yched's point, the alt text, I believe, only goes on <img> not on <picture>, so it's not optional. I only scanned the patch briefly, but it does look like the alt is going to the <img>. Its attributes are processed separately.

As far as I know <picture> element can use global attributes, though probably not used very often. Is anything being put there by default? It is certainly conceivable to add a class to
even if you aren't styling <picture> itself. (passing no judgement as to whether that is recommended)

attiks’s picture

#296 & #297

  1. Any alternative for fallback_image? You're right that it is mandatory, but not really an idea on how to call it.
  2. Styling picture is indeed not done, the picture tag acts like a container.
  3. All attributes go to the img tag, adding anything to picture has no real value since most browsers will simply ignore it. I think class and id are exceptions, but maybe we should allow it, the big question is then: how to differentiate between attributes for the picture tag and those for the img tag? And is this something that can be done in a follow up issue?
cosmicdreams’s picture

Here's a thought:

When looking at the specifications:
( http://www.w3.org/html/wg/drafts/html/master/embedded-content.html#the-p... )
( https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-ele... )

This final child of the picture element is simply called the "img element". So why don't we call it img_element?

attiks’s picture

#299 Works for me, we'll change it unless anybody else has a problem with it

yched’s picture

+1 on img_element

Regarding attributes:
- ok, the patch correctly places 'alt', 'title', 'text', and 'attributes' on the img tag, but:
- 'attributes' isn't documented in the phpodoc for template_preprocess_responsive_image()
- 'attributes' stays in the $variables array that gets passed to responsive-image.html.twig, and is thus *also* printed as {{attributes}} on the picture tag ? I'd tend to agree with @attiks in #298.2 that we don't necessarily need to support attributes on the picture tag, but even if we do, it can't just be the same ones the than those of the img ;-)

yched’s picture

Status: Needs review » Needs work

Seems like a NW then

davidhernandez’s picture

Yeah, if they are both using the same attributes that's not good. They have to be different or conflicts will occur.

Wim Leers’s picture

#294:

type="image/jpeg" sizes="">
An empty sizes attribute. Is that problematic?


#295: WOW, you write tests at never-seen-before speeds! :O

+++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldUITest.php
@@ -0,0 +1,136 @@
+class ResponsiveImageFieldUITest extends WebTestBase {

Super nitpicky nitpick: The class name should probably contain Ui instead of UI… that's our rule (which I personally disagree with, I think this is much nicer). That'd be consistent with FieldUiTestTrait.


#296#300 (RE: fallback image): great points! But it feels to me we could make this even clearer by replacing

{{ fallback_image }}

with

<img{{fallback_img_attributes}}>

That make it abundantly clear that <img> is an integral part of <picture>, and it's analogous to how we deal with <source>.


#298#303 (RE: attributes):

All attributes go to the img tag […]

Indeed:

+++ b/core/modules/responsive_image/responsive_image.module
@@ -189,114 +180,225 @@ function theme_responsive_image($variables) {
+  foreach (array('alt', 'title', 'attributes') as $key) {
+    if (isset($variables[$key])) {
+      $variables['fallback_image']["#$key"] = $variables[$key];

I think that @yched's proposal in #301 makes sense, and together with my proposal for {{fallback_img_attributes}}, the code could become very clear. There would be no need to use '#theme' => 'image' anymore, so template_preprocess_responsive_image() could then become simpler: it just needs to build the final list of attributes, and pull out the alt and title attributes, everything else can go to <picture>'s attributes.

The only downside: no reuse of the logic in template_preprocess_image(), but 95% of that is for dealing with srcset containing multiple images, which by definition is not the case here anyway.

But perhaps I'm missing something and implementing what I propose either causes problems or complicates things too much?

attiks’s picture

#304

-An empty sizes attribute. Is that problematic? Nope
- <img{{fallback_img_attributes}}> looks nice, but this means that theme image is circumvented, making overwriting the image output (for whatever reason) impossible, hence why we selected this approach. We'll rename it to img_element (#299)
- Attributes are now outputted on picture, but we'll remove the attributes in twig, adding attributes on picture will be handled in a new issue

attiks’s picture

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
116.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,193 pass(es). View
4.32 KB

It seems the attributes on the <picture> tag were introduced by me (by accident) when converting the theme function to the twig template.

Code before patch:

$output = array();
$output[] = '';

// Add source tags to the output.
foreach ($sources as $source) {
  $responsive_image_source = array(
    '#theme' => 'responsive_image_source',
    '#src' => $source['src'],
    '#dimensions' => $source['dimensions'],
  );
  if (isset($source['media'])) {
    $responsive_image_source['#media'] = $source['media'];
  }
  if (isset($source['srcset'])) {
    $responsive_image_source['#srcset'] = $source['srcset'];
  }    
  $output[] = drupal_render($responsive_image_source);
}
$output[] = '';
return SafeMarkup::set(implode("\n", $output));

=> No attributes on the <picture> tag.

#304.#294: I cleaned up responsive_image_build_source_attributes a bit so the type attribute and the sizes attribute are not outputted if they don't have a value (to avoid confusion).
#304.#295: Haha! We should start a Drupal Book Of Records then! File & class renamed.
#304.#296#300: Renamed {{ fallback_image }} to {{ img_element }}.
#304.#298#303: All attributes go on the {{ img_element }}. Attributes for the <picture> tag will be solved in #2421317: Do #attributes/$variables['attributes'] to go the <picture> or the fallback <img>?.

Wim Leers’s picture

After that intensive review/reroll process, I'd now like to ask mdrummond, yched and any others willing to take another good look at this patch. IMHO we're quickly approaching RTBC.


+++ b/core/modules/responsive_image/templates/responsive-image.html.twig
@@ -26,5 +25,5 @@
-  {{ fallback_image }}
+  {{ img_element }}

Perhaps a comment like:
{# The fallback image. #}
? The previous variable name left no doubt what it was, readers less familiar with responsive images will be confused to see just "img_element", without qualification.

yched’s picture

Or rather {# The img element, with the fallback image in srcset. #} ? ;-)

I also wanted to suggest something like <img srcset="{{fallback_image}}"{{attributes}}/> earlier, would definitely make the picture tpl crystal clear, but figured bypassing #theme 'image' might be controversial.
Not sure I see the real interest of allowing overrides of the img markup within a picture, but I can't say I feel too strongly either way :-)

yched’s picture

Actually, is it really something that we want, that an override of how simple images are displayed also affects the img of a picture group ?

attiks’s picture

#310 I think so, people would expect that if they change the twig for img that it will change everywhere, but maybe best to discuss in a separate issue so we don't stall this one.

Wim Leers’s picture

#309: your comment suggestion is even better :) And +1 to Not sure I see the real interest of allowing overrides of the img markup within a picture.

Agreed with #310 too.

But yes, this can be a follow-up. No API changes.

mdrummond’s picture

For what it's worth, the technical term for the img within a picture element is "controlling image" as it's the img that controls what displays. The source elements are just to determine what files show up on that controlling image.

So fallback isn't necessarily the appropriate term, because it's not a fallback, it's the img that actually matters. I think using fallback actually somewhat confuses things. So I'd just changing this to controlling_image or something like that. Default image might be another option, but I'm just pulling that name from a hat. If we stay with fallback_image it's not the end of the world though.

I also don't really see the value of using the img template within picture. That seems like it could introduce problems if somebody wants to make changes to non-picture img elements without those changes showing up within picture too.

Having img with {{controlling_attributes}} within the picture template would be nicer because it would make it much easier for front-end developers to add classes to the img element. That's the general direction we've been going with Twig, adding in classes within the template rather than in preprocessor functions. Adding a class to a picture img but not a non-picture img would be significantly easier with img in the picture template.

I suppose the reasoning for why not to do that would be to make it easier for people to switch between picture and a standard img.

Bottom line is since there might be arguments on both sides, let's not try to solve that in this issue.

yched’s picture

@mdrummond #313

"controlling image" : agreed, but I think we're fine with calling it {{img_element}} in the template ?

Other than that, yep, +1 on the arguments about skipping theme_image, but discussing that in a followup is fine by me :-)

Jelle_S’s picture

Ok, so to recap #307 -> #314:

  1. Add a comment to responsive-image.html.twig:
    {# The controlling image, with the fallback image in srcset. #}
  2. Separate issue for '#theme' => 'image' vs <img srcset="{{fallback_image}}"{{attributes}}/> (or similar) discussion.

Anything else?

mdrummond’s picture

{{img_element}} is good for the template right now.

1. That sounds good.
2. Yes.

Jelle_S’s picture

FileSize
116.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,336 pass(es). View
516 bytes

Here you go :-). Feels like we're getting close :-D

mdrummond’s picture

Status: Needs review » Reviewed & tested by the community

I have just completed a thorough review of this patch as well as the entire responsive image module. That includes testing the use of the responsive image module. From my point of view as a front-end developer who wants to make sure I can use the current responsive images spec, this looks good. Wim and yched seem satisfied with the current state of the patch, so I'm going to trust their judgment as to the quality of the code and implementation.

Our goals here are to be able to update our asset library to use a current version of Picturefill, a polyfill which supports the current responsive images spec. That primarily includes the picture and source elements, along with the usage of the type, sizes and srcset attributes. This issue achieves that goal.

This is a critical goal, as prior to this patch, Drupal 8 was supporting an out of date version of the picture element. Launching Drupal 8 with incorrect support of the picture element would be a severe problem. This issue also clears up a whole slew of other issues and bugs within the responsive images queue, not the least of which is that this converts the theming of responsive images to use Twig. Because this involves schema changes, it's important we get this in now rather than later.

attiks and Jelle_S have done a huge amount of work to get to this point, and the reviews of Wim, yched and others are all deeply appreciated.

I think there may be more improvements we can make in follow-up issues. In particular I think #2334387: UI changes to support current responsive image standards is important to make it easier to use the new sizes attribute. For now, though, this allows us to actually use the correct picture spec and really clean up the responsive image module.

I'm going to mark this as RTBC. We've had thorough review of this issue and cleaned up the code with issues both large and small. If somebody else spots something after we're at RTBC, we'll clean that up as fast as possible, but hopefully we're now good to go.

Thanks so much everybody for your hard work.

Wim Leers’s picture

Hurray! :)

Awesome work, attiks & Jelle!

yched’s picture

Sorry for being late in taking a deep look at the code :-/
Got a couple meaty (but not necessarily time-consuming) remarks below, will post a couple nitpicks in a later comment.

  1. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
    @@ -56,28 +57,36 @@ class ResponsiveImageMapping extends ConfigEntityBase implements ResponsiveImage
    +   * The responsive image mapping definitions.
    

    Sorry for spotting this so late, but it feels lika a smoke sign (and IMO doesn't help getting a clear view of the conceptual model) that a Mapping is a collection of mappings...

    The current code sweeps that under the carpet a bit, by (almost) consistently calling the outer one "mapping" and the inner ones "mapping definition" - so ResponsiveImageMapping::addMappingDefinition($mapping_definition) adds a mapping definition to the mapping. That's still a bit "yo dawg" ;-)

    At the end of the day, we have a 1-1 correspondance between "ResponsiveImageMapping and its contents" and "picture and its source tags in the HTML". But our current names totally obfuscate that, and call both levels "mapping" :-). That's probably because they come from a time when there was no official HTML spec for responsive images yet. There is now, so it's a bit sad that we're stuck with the meta names we had to invent in the meantime - especially if those names are confused :-). We'll look at them in two years and go "man, why so obscure ?".

    - ResponsiveImageMapping is in fact a named preset for a responsive image, right ? --> rename to ResponsiveImagePreset or PicturePreset ? Thus, we have responsive image presets for generating picture tags, just like we have image presets for generating flat imgs ?
    - The various "mapping definitions" it holds are simply the description of the source tags in the picture. It could clarify the code and help getting "who controls what" a lot if the names of the properties and methods reflected that.

    I'd love not to block the patch on this, but cleaning it later would likely mean API and/or config schema changes :-/.
    Renaming the inner "mappings" to "something about sources" might be a lot of changes at this point. Maybe just renaming ResponsiveImageMappingSet to ResponsiveImagePreset would be enough ?

  2. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -189,114 +181,230 @@ function theme_responsive_image($variables) {
    +function responsive_image_build_source_attributes(ImageInterface $image, $variables) {
    

    Lacks a @return phpdoc. Also, this points that the function is slightly misnamed, it doesn't deal with the attributes of one source, it returns an array describing all sources of the Mapping / picture tag.

    Related : matter of taste, but it might slightly help understanding the code flow if the function was really about *one* single source, and the "foreach(Mapping->mappings()) {build the source}" was moved up into template_preprocess_responsive_image().
    It would also match what we ended up doing in the twig template :-)
    As a side effect, it might also help clarifying the non-obvious dance with $variables['width' / 'height'] currently between the caller and the callee, by moving it entirely up in the caller.

    One way or the other, the function should be named according to what it does (one source or several sources) :-)

  3. +++ b/core/modules/responsive_image/tests/src/Unit/ResponsiveImageMappingConfigEntityUnitTest.php
    @@ -79,99 +79,226 @@ public function testCalculateDependencies() {
    +    $entity->addMappingDefinition('test_breakpoint', '1x', array(
    +        'image_mapping_type' => 'sizes',
    +        'image_mapping' => array(
    +          'sizes' => '(min-width:700px) 700px, 100vw',
    

    This was discussed a bit in #57 / #61, and I'm totally fine with keeping that for a followup, but I still regret that the 'sizes' in mappings have to be expressed in litteral media conditions, completely disconnected from the named breakpoints.

    'sizes' is supposed to announce what size your CSS will give to the image in the layout, so in practice your CSS/SASS will most likely express that in terms of a named same set of breakpoints. This means tweaking the breakpoint definitions in your CSS (say adjust the pix width of a breakpoint) will require you to manually find and update the corresponding media conditions in all your mapping yamls, which feels very unpractical and brittle.

    Would be nice if the mapping definition allowed me to use litteral media conditions OR my named breakpoints here ?
    As I said above, fine with a followup though.

    yched’s picture

    And the nitpicks :

    1. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
      @@ -56,28 +57,36 @@ class ResponsiveImageMapping extends ConfigEntityBase implements ResponsiveImage
      +   *     - If image_mapping_type is 'sizes', an array with following keys:
      +   *       - sizes: If image_mapping_type is 'sizes', the value for the 'sizes'
      +   *         attribute.
      

      No need to repeat "if image_mapping_type is 'sizes'" the second time, we're already in that case ?

    2. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
      @@ -89,68 +98,65 @@ public function __construct(array $values, $entity_type_id = 'responsive_image_m
      +        if (!$this->isEmptyMappingDefinition($mapping)) {
      

      the method is static, so rather static::isEmptyMappingDefinition($mapping) ?

    3. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
      @@ -89,68 +98,65 @@ public function __construct(array $values, $entity_type_id = 'responsive_image_m
      +  public function getMappingDefinitions() {
      +    return $this->get('mapping_definitions');
      ...
      -    $this->set('breakpointGroup', $breakpoint_group);
      +    $this->set('breakpoint_group', $breakpoint_group);
      
      @@ -158,15 +164,15 @@ public function setBreakpointGroup($breakpoint_group) {
         public function getBreakpointGroup() {
      -    return $this->get('breakpointGroup');
      +    return $this->get('breakpoint_group');
      

      I kind of lost track whether this now is the recommended way, or whether we still use protected properties (e.g $this->mapping_definitions) directly. I think most our current code does the latter ?

    attiks’s picture

    #320.3 sizes: In general the mdeia queries for sizes are not used inside your theme, most themes have 3-4 breakpoints to adapt the layout, but for sizes it is in fact depending on the layout of the article (or other part), so there;s no strict relation between theme breakpoints and what you use inside the sizes attribute.

    Example https://attiks.com/project/warsteiner-party-present:

    Uses sizes="(min-width: 80em) 424px, (min-width: 30em) 40vw, 50vw", none of them are used inside the layout breakpoints as defined by the theme.

    mdrummond’s picture

    Status: Reviewed & tested by the community » Needs work

    I was going to comment on #320.3 as well.

    Sizes is a tricky beast. It represents a list of media conditions and the associated size of the containing element for the image.

    This is not the same as the media queries as defined in breakpoints.yml. Breakpoints is a series of media queries which can consist of both a media condition (min-width: 30em) and a media type (screen only). A particular breakpoint can even have a series of media condition + media type pairings separated by a comma.

    So it's not possible to re-use breakpoints within a sizes definition, because the media queries are fundamentally different. Even if you could, you would still need to associate each of those media conditions with the size of that element, which is not something defined within a breakpoint. In theory you could add a size attribute to a breakpoint, but then you'd still have the potential for somebody to put something beyond just a media condition into a breakpoint definition, which wouldn't work.

    Earlier I had thought, well maybe we set up a separate file called sizes.yml where we could store all the possible sizes definitions we want. However the more I think about it, the more that seems like overkill. A particular sizes definition defines the media conditions where the width of a particular component on a page changes its widths. So in order for it to make sense to use a particular sizes definition in more than one responsive image mapping, you'd need to be defining a different set of image styles (in srcset) for the same-sized component. In theory that might happen, but it seems pretty rare. So setting up an abstraction to allow for re-use in this case seems like overkill.

    What I would like to see for sizes is an actual UI, and I'm learning towards using the UI already mocked up in #2334387: UI changes to support current responsive image standards. Again, earlier I had thought of trying to make a more complicated UI that would use a sizes.yml file, but I've come around to thinking that's overkill. So I'd rather focus on us getting this patch completed and into core so we can move on to the UI issue and get that done as quickly as possible, which seems very important for people to be able to easily use the sizes attribute.

    I'm moving this back down to needs work to get it out of the RTBC queue. I think a lot of the points yched raised are worth addressing before we get this in. Once those are resolved we'll probably be ready to move this back up to RTBC again.

    (edited for clarity)

    mdrummond’s picture

    One other note:

    As long as we're doing one more patch, it's maybe worth noting that Picturefill was updated to 2.2.1 a few days ago. Might be worth putting that new version in to take advantage of any bug fixes.

    Wim Leers’s picture

    #320:

    Great feedback :) I only have something to reply to your first point.

    1. I totally hear you regarding the confusing naming: in HEAD, "responsive image mapping" contains "mappings", and in this patch each such mapping also gets a "mapping type". That's why I asked those contained "mappings" to be renamed to "mapping definitions"… which is also what 99% of the code actually called it. At least now you don't have "mapping -> mappings" but "mapping -> "mapping definitions". This is both more consistent with the existing code and less confusing in that it at least is consistently disambiguated (everything is named consistently everywhere, and hence it's always clear what is being talked about, once you've read the config schema).
    But that doesn't mean the naming is great or that it can't be improved further, of course. I just felt that it was sufficiently clear now, hence no need to block this issue on that.
    I'd love to see an improvement there, so fully agreed in that sense. But I'm not convinced by your proposed names. Do we really want to model the data structure/config schema after <picture>? Even if we already know that <picture> is just one possible form of output, and <img srcset> is another? (See #2260061-227: Responsive image module does not support sizes/picture polyfill 2.2, #240, #244, etc.) Because then calling each mapping definition a "source" (or "source definition" or…) suddenly no longer makes a ton of sense.
    I think "responsive image mapping" also is a misnomer — it makes sense from a purely technical POV, but not from a concept POV. We already have image styles. What a "responsive image mapping" really is, is a "responsive image style". It's exactly the same concept, applied to responsive images. It even reuses image styles to achieve what it does. Then HEAD's "mappings" and the patch's "mapping definitions" could become "image style mappings"… which is exactly what it does: given a certain viewport, a certain image style must be used (or in case of a "sizes" mapping definition: a sequence of image styles… but still image styles).

    IMHO the questions are

    1. How necessary is this?
    2. It sure would be nice to improve this. If we can do it in this patch: great! But do we all agree?
    3. Is it even possible to express this fully logically/semantically, for the necessary support for sizes almost seems to introduce some unavoidable cracks in the data model?
    4. Would it be sufficient to add more comments to the existing config schema, detailing what each level will map to when rendered as a <picture>?
    Jelle_S’s picture

    mdrummond’s picture

    If you look here, there's a note about 2.2 being bumped to 2.2.1, which you can see at the the top of the picturefill.js file.

    https://github.com/scottjehl/picturefill/tree/2.2/dist

    Jelle_S’s picture

    Assigned: Unassigned » Jelle_S

    #327: Ok I missed that one. I'll use it in the next patch.
    #321.3: Seems there are mixed usages in Drupal core right now (see ImageStyle vs Block). I'll use the latter in the next patch.
    #325: I'm comfortable with "responsive image style" and "image style mappings". I discussed it with @attiks and he was as well.

    Jelle_S’s picture

    Also, for the config schema:
    responsive_image.mappings.* then becomes something like responsive_image.style.*??

    Wim Leers’s picture

    #328 + #329: sounds good to me, but let's wait for yched's feedback. I'll ping him on Twitter.

    yched’s picture

    ResponsiveImageStyle works for me. Naming this in a symmetrical way than ImageStyles makes a lot of sense (that's what I was shooting for with ResponsiveImagePreset in #320, but I forgot that it's in fact ImageStyle and not ImagePreset :-p)

    This solves the most pressing issue ("mapping has mappings"). Then how we name what's inside a ResponsiveImageStyle and how you set it up (does it have "sources" or "mappings") becomes less critical IMO.

    My personnal take would be :

    There is an official HTML spec for responsive images, it's great and feature-rich, but it's not exactly trivial to grasp.
    I see value in picking names that are close to the HTML spec, rather forcing people to additionally grasp one-off drupalisms - since, once again, the concepts and model map exactly (responsive_image_build_source_attributes() does exactly: one "mapping" in the ResponsiveImageStyle --> one <source> in the <picture>). it's just that we invent a different terminology for no clear gain.

    Getting your sources, breakpoints, sizes and srcsets right for the specific case you're designing typically take some time and back-and-forth adjustements, so having to navigate between the official terminology in your HTML and the drupalisms in your config yamls or UI feels a bit unnecessary.

    Do we really want to model the data structure/config schema after
    ? Even if we already know that
    is just one possible form of output, and Only local images are allowed. is another?

    Right, that's because I see <img srcset> as a mere degenerate case of "picture with a single source". That's how the spec or any tutorial explaining the spec describes it, and that's how our code would treat it if/when we support it (it would still be stored with the same config schema of "a list of mappings/sources" with one single element).

    I can't say I feel really strongly about it though :-)
    ResponsiveImageStyle++, then if it's only me about "mappings", feel free to ignore :-)

    mdrummond’s picture

    I actually think that the most common use case for responsive images is going to be:

    1) img with srcset AND sizes attribute

    OR

    2) picture with one source element with sizes and srcset attribute

    That's the use case that makes it easiest to provide different width images for different viewport sizes.

    Picture with multiple source elements will primarily be useful if you want to crop an image differently at different breakpoints. However since that's harder to set up, I think that will be more rare. The other big use case of multiple source elements is if you want to provide both webp and jpg versions of images.

    So however this gets named, just try to keep how often each use case will likely get used.

    If we have a responsive image style, with a single source, and that source has sizes styles defined with multiple styles, it feels like we're nearing the Xzibit threshold, but maybe that's fine. We can already put Twig blocks within Drupal blocks in D8, so if we can put styles within styles, then cool.

    Maybe we could just write down what the current naming scheme looks like as things are nested, with both the multiple MQ source use cases and the single source sizes/srcset use case? And then the same thing for any alternative proposals? Maybe just writing those down will help to make this naming debate go as quick as possible.

    Wim Leers’s picture

    So, it sounds like everybody is +1 for ResponsiveImageMappingResponsiveImageStyle.


    For the data within that (which HEAD calls mappings, and the patch calls mapping definitions), we don't really seem to have consensus, because:

    1. yched proposed sources, for consistency with <picture>
    2. I proposed image style mappings (i.e. something very similar to what the code already uses), because that's internally consistent with Drupal and I don't believe it's possible to have a naming scheme that makes sense in all situations, i.e. both for <picture> and for <img srcset
    3. mdrummond says that he thinks <picture> with multiple <source>s is actually not going to be very common (once we add the UI to configure sizes) — AFAICT this is further confirmation that it's impossible to apply names that are wholly consistent with both supported outputs (<picture> + <img srcset>)
    4. … I'm not clear on what attiks & Jelle_S's opinions are — your opinions will probably help bring clarity here

    Given that none of us so far feel the naming for the nested data is critical (mdrummond and I were fine with an earlier RTBC, yched says in #331 that it's become less important if we rename the top-level thing to ResponsiveImageStyle)… perhaps we can conclude to not change that here after all, for it is not important enough (even though it would be nice)?

    yched’s picture

    Yep, works for me.

    Once ResponsiveImageMapping → ResponsiveImageStyle is done, still leaves #320.2 and #321 ? :-)

    Jelle_S’s picture

    Assigned: Jelle_S » Unassigned
    Status: Needs work » Needs review
    FileSize
    164.08 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,376 pass(es), 7 fail(s), and 0 exception(s). View
    141.29 KB
    97.24 KB

    New patch:

    • Includes new picturefill library (2.2.1)
    • Uses $object->property rather than $object->get('property')
    • Renames ResponsiveImageMapping → ResponsiveImageStyle and "mapping definition" → "image style mapping".
    • Addresses #320.2
    • Addresses #321
    Jelle_S’s picture

    Status: Needs work » Needs review
    FileSize
    165.51 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,365 pass(es). View
    1.43 KB

    URLs have changed, thats why the test failed.

    yched’s picture

    Thanks a lot @Jelle_S !

    I think we can still streamline responsive_image_build_source_attributes() a bit, but this can totally happen in a followup, no need to further block a critical with 340 comments on this. No more remarks from me ;-)

    webchick’s picture

    So is that an RTBC? :)

    attiks’s picture

    #338 @yched I created #2423743: streamline responsive_image_build_source_attributes() for you, would love to hear about the streamlining.

    yched’s picture

    Status: Needs review » Reviewed & tested by the community

    Sure, I can do the RTBC, since I was the party pooper the previous time :-)

    @attiks : thanks ! will follow up over there

    Pages