Problem/Motivation

Responsive themes (including Drupal 8 core themes) will require using a variety of image sizes at different resolutions, so

  • Images are optimized for screen and bandwidth.
  • Anonymous page caching will work out-of-the-box.
  • Other modules can easily alter the output for.

Proposed Resolutions

  1. Implement the still to be approved picture element so Drupal is using the latest technology at the time of release, by the looks of it (http://www.w3.org/community/respimg/2012/08/04/picture-in-the-html5-spec/) this will be solved in a timely matter, well before Drupal 8 is released.
  2. Implement it as a separate module so user can decide if they need it or not. This also gives contrib the possibility to come up with something better.
  3. Picture acts as a new display formatter for existing image fields.
  4. Focus on image fields first, but make it easy to support media, inline images as well.
  5. For the moment all modern browsers are working (so not IE8-).

Remaining tasks

  • Write full documentation for picture.

Original report by [Jeff Burnz]

see #1170478: Responsive Images for the background discussion

Related discussions

Polyfills for browsers not supporting <picture>

Browser support

  • For the moment no native support, but some big companies are helping out on the proposal
  • Polyfill works in modern browsers (IE9+)
  • IE8 isn't working, because is doesn't understand media queries
  • The fallback should work in all browsers and can be configured, the default fallback is the mobile variant, but this can be changed.
Files: 
CommentFileSizeAuthor
#68 i1775530-68.patch9.65 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 47,628 pass(es). View
#68 interdiff.txt4.39 KBattiks
#62 i1775530-62.patch6.37 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 46,895 pass(es). View
#60 i1775530-60.patch7.3 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 46,880 pass(es). View
#53 picture-53.patch52.6 KBattiks
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch picture-53.patch. Unable to apply patch. See the log in the details link for more information. View
#53 interdiff.txt3.42 KBattiks
#50 picture-50.patch50.99 KBattiks
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch picture-50.patch. Unable to apply patch. See the log in the details link for more information. View
#50 interdiff.txt4.85 KBattiks
#44 picture-44.patch50.39 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 46,803 pass(es). View
#42 picture-42.patch50.47 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 46,693 pass(es), 1 fail(s), and 5 exception(s). View
#40 picture-40.patch50.56 KBattiks
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/picture/lib/Drupal/picture/Tests/PictureFieldDisplayTest.php. View
#40 interdiff.txt1.24 KBattiks
#36 picture-36.patch50.49 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 46,572 pass(es), 2 fail(s), and 0 exception(s). View
#27 Picture Mapping.JPG39.13 KBShyamala
#27 image styles.JPG33.59 KBShyamala
#27 artcle confg.JPG55.04 KBShyamala
#24 picture-24.patch50.47 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 46,504 pass(es). View
#24 interdiff_24_23.txt2.34 KBattiks
#23 picture-23.patch49.39 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 42,877 pass(es). View
#21 picture-21.patch48.36 KBattiks
PASSED: . View
#20 picture-20-combined.patch110.4 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 42,706 pass(es). View
#18 picture-18-combined.patch117.85 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 42,794 pass(es). View
#15 picture-only-do-not-test.patch47.48 KBattiks
#15 picture.patch104.78 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 42,759 pass(es). View
#14 picture.patch103.36 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 42,562 pass(es). View
#14 picture-only-do-not-test.patch46.71 KBattiks
#13 picture.patch91.82 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 42,566 pass(es). View
#12 picture.patch91.32 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 42,559 pass(es). View
#11 picture.patch90.71 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 42,570 pass(es). View
#10 picture.patch90.66 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 42,575 pass(es). View
#9 picture.patch91.6 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 42,497 pass(es). View
#6 picture.patch34.71 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 42,133 pass(es), 38 fail(s), and 2 exception(s). View

Comments

klonos’s picture

    ...
  • Polyfill works in modern browsers (IE9+)
  • IE8 isn't working, because is doesn't understand media queries
  • ...

I realize there's talk about dropping IE8 support in D8, but wouldn't this work in IE8 too if a polyfill (like matchmedia.js for example) was used?

effulgentsia’s picture

In #1465948: [meta] Drop some IE8 coddling from Drupal core and #1645494: Allow core themes to display single-column on IE8: do not compensate for lack of media query support, we decided that it is not core's responsibility to compensate for IE8's lack of media query support: that IE8 falling back to mobile layouts (and by extension, image variants) is ok. So I'd like to restate #1 as: if a contrib module like Respond.js were enabled, would the picture polyfill work on IE8, or would something else be needed, and if so, what?

attiks’s picture

#1 the polyfill uses matchmedia.js to do its magic, but mactmedia only works on browser that support media queries

#2 at least for picture the fallback can be choosen, it defaults to mobile, but can be changed. Some themes (like omega) use something comparable to fix the IE9- problem.

Respond.js has adds limited support for media queries in IE9-: "support for min-width and max-width CSS3 media queries" and it adds extra request to analyze the css, so it will not work with most media queries. AFAIK there's no proper way to add support to those browsers, unless we write something on our own.

wilto’s picture

Hey, just figured I’d chime in. I know supporting IE8 is a much larger discussion, but as far as Picturefill goes: Scott Jehl, Paul Irish, and Nicholas Zakas whipped-up an awesome matchMedia polyfill that we’ve thoroughly tested with Picturefill.

https://github.com/scottjehl/matchMedia.js

attiks’s picture

Targeting IE8 is possible if you build your media query in the right way: "screen, all and (min-width: 560px) and (max-width:850px)" can be used to target IE8/IE7

The demo is updated as follows:

  • It uses the above MQ to target IE8/IE7 (outputs the medium image)
  • The fallback is the thumbnail image
attiks’s picture

Status: Active » Needs review
FileSize
34.71 KB
FAILED: [[SimpleTest]]: [MySQL] 42,133 pass(es), 38 fail(s), and 2 exception(s). View

Status: Needs review » Needs work

The last submitted patch, picture.patch, failed testing.

attiks’s picture

Tests will keep failing since this depends on breakpoints functionality #1734642: Move breakpoint module into core

attiks’s picture

Status: Needs work » Needs review
FileSize
91.6 KB
PASSED: [[SimpleTest]]: [MySQL] 42,497 pass(es). View

New patch including the breakpoint patch so it can be tested.

attiks’s picture

FileSize
90.66 KB
PASSED: [[SimpleTest]]: [MySQL] 42,575 pass(es). View

new patch after renaming of breakpoint, also added tests for picture output

attiks’s picture

FileSize
90.71 KB
PASSED: [[SimpleTest]]: [MySQL] 42,570 pass(es). View

missed one rename

attiks’s picture

FileSize
91.32 KB
PASSED: [[SimpleTest]]: [MySQL] 42,559 pass(es). View

more renaming, feel free to review the picture part

attiks’s picture

FileSize
91.82 KB
PASSED: [[SimpleTest]]: [MySQL] 42,566 pass(es). View

new patch, removing wrapper functions

attiks’s picture

FileSize
46.71 KB
103.36 KB
PASSED: [[SimpleTest]]: [MySQL] 42,562 pass(es). View

new patch, using latest version of the breakpoint patch

attiks’s picture

FileSize
104.78 KB
PASSED: [[SimpleTest]]: [MySQL] 42,759 pass(es). View
47.48 KB

some minor fixes

Status: Needs review » Needs work

The last submitted patch, picture.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
attiks’s picture

FileSize
117.85 KB
PASSED: [[SimpleTest]]: [MySQL] 42,794 pass(es). View
attiks’s picture

Proposal can be found at http://wilto.github.com/draft-prop/ResponsiveImages.html
At the bottom of that page you can find all open issues.

attiks’s picture

FileSize
110.4 KB
PASSED: [[SimpleTest]]: [MySQL] 42,706 pass(es). View

Implemented display formatter as plugin #1785748: Field formatters as plugins

attiks’s picture

FileSize
48.36 KB
PASSED: . View

Patch against core now that breakpoint is added.

Firstly for the bot, but feedback appreciated.

attiks’s picture

+++ b/core/modules/picture/lib/Drupal/picture/PictureMapping.phpundefined
@@ -0,0 +1,123 @@
+  public function __construct(array $values = array(), $entity_type = 'picture_mapping') {

oops

+++ b/core/modules/picture/lib/Drupal/picture/PictureMapping.phpundefined
@@ -0,0 +1,123 @@
+    $duplicate = new PictureMapping();

entity_create

+++ b/core/modules/picture/lib/Drupal/picture/PictureMappingFormController.phpundefined
@@ -0,0 +1,119 @@
+    if ($this->operation == 'duplicate') {
+      $picture_mapping = $picture_mapping->createDuplicate();
+      $this->setEntity($picture_mapping, $form_state);
+    }

move to page callback

+++ b/core/modules/picture/picture.infoundefined
@@ -0,0 +1,9 @@
+configure = admin/config/media/picturemapping
\ No newline at end of file

new line

+++ b/core/modules/picture/picture.moduleundefined
@@ -0,0 +1,334 @@
+

add hook_help

+++ b/core/modules/picture/picture_mapping.admin.incundefined
@@ -0,0 +1,93 @@
+function picture_mapping_page_edit($picture_mapping) {
+  drupal_set_title(t('<em>Edit picture mapping</em> @label', array('@label' => $picture_mapping->label())), PASS_THROUGH);
+  return entity_get_form($picture_mapping);
+}

add something for duplicate as well

+++ b/core/modules/picture/picture_mapping.admin.incundefined
--- /dev/null
+++ b/core/modules/picture/picturefill/matchmedia.jsundefined

should an uncompressed copy be added as well?

attiks’s picture

FileSize
49.39 KB
PASSED: [[SimpleTest]]: [MySQL] 42,877 pass(es). View

new patch for #22

attiks’s picture

Issue summary: View changes

Updated issue summary.

attiks’s picture

FileSize
2.34 KB
50.47 KB
PASSED: [[SimpleTest]]: [MySQL] 46,504 pass(es). View

New patch using matchmedia from #1815602: Introduce a polyfill for matchMedia

Shyamala’s picture

Status: Reviewed & tested by the community » Needs review

As I understand the Picture module facilitates mapping of image caches to different groups of breakpoints.

Steps followed to configure Picture module and test the same:

  1. Created article with image
  2. Enable Breakpoint module
  3. Enabled Picture module
  4. Visited Configuration page Picture mapping admin/config/media/picturemapping
  5. Added a label 'My Article' to Group Bartik
  6. Mapped mobile to Thumbnail, narrow to Medium and wide to Large
  7. Visited Content Types settings page at admin/structure/types/manage/article/display
  8. Changed Article Image format from Image to Picture
  9. Clicked on the settings icon
  10. Mapped to Picture mapping 'My Article' created earlier & set Fall-back image to Automatic. Don't forget to click save here!
  11. Applied patch at: #1815602-43: Introduce a polyfill for matchMedia
  12. Visited article with image and resized browser, the breakpoints and image sizes mapped using Picture configuration did not apply. The full sized image was loaded and the image resized as the browser was resized. The testing was carried out in Chrome

The Configured breakpoints did not seem to apply! Is there any configuration that I have missed?

attiks’s picture

You need to apply to patch from #1815602-43: Introduce a polyfill for matchMedia that should fix it.

If that doesn't help, can you paste the output of the picture?

Thanks

Shyamala’s picture

FileSize
55.04 KB
33.59 KB
39.13 KB

Output for the picture:

<div class="field-items">
<div class="field-item even" rel="og:image rdfs:seeAlso" resource="http://192.168.99.131/drupal-8/sites/default/files/field/image/Blue%20hills.jpg">
<img typeof="foaf:Image" src="http://192.168.99.131/drupal-8/sites/default/files/field/image/Blue%20hills.jpg" width="800" height="600" alt="">
</div>
</div>

Also attached screen shot of configuration & updated steps at #25

attiks’s picture

#27 that output is the default image output, did you press 'Save' on admin/structure/types/manage/article/display after changing it to picture?

Shyamala’s picture

Looks like I did not click on save. It's working fine now.
Shouldn't there must be an alert to the user requesting him to click on save? I only used the update button!
Works perfectly on all breakpoints.

Shyamala’s picture

Status: Needs review » Reviewed & tested by the community

Works perfectly on following the steps at #25.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

#29 I think a message would be fine, but it's a problem with all field formatters, can you create a new issue for it?

BTW thanks for testing.

webchick’s picture

Status: Reviewed & tested by the community » Postponed

I believe we need #1815602: Introduce a polyfill for matchMedia in first, so marking this postponed.

attiks’s picture

Status: Postponed » Needs review

matchmedia polyfill is committed, back to NR for an in depth code review

attiks’s picture

Status: Needs review » Needs work
attiks’s picture

attiks’s picture

Status: Needs work » Needs review
FileSize
50.49 KB
FAILED: [[SimpleTest]]: [MySQL] 46,572 pass(es), 2 fail(s), and 0 exception(s). View

New patch without hook_entity_info

Status: Needs review » Needs work
Issue tags: -Usability, -mobile, -frontend performance, -media queries, -Responsive Design, -Design Initiative, -d8mux

The last submitted patch, picture-36.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#36: picture-36.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Usability, +mobile, +frontend performance, +media queries, +Responsive Design, +Design Initiative, +d8mux

The last submitted patch, picture-36.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
50.56 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/picture/lib/Drupal/picture/Tests/PictureFieldDisplayTest.php. View

New patch to fix the test use statements

Status: Needs review » Needs work

The last submitted patch, picture-40.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
50.47 KB
FAILED: [[SimpleTest]]: [MySQL] 46,693 pass(es), 1 fail(s), and 5 exception(s). View

grrrr

Status: Needs review » Needs work

The last submitted patch, picture-42.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
50.39 KB
PASSED: [[SimpleTest]]: [MySQL] 46,803 pass(es). View

we're getting there.

Status: Needs review » Needs work

The last submitted patch, picture-44.patch, failed testing.

attiks’s picture

bot failing on userpicture

attiks’s picture

Status: Needs work » Needs review

thank you bot ;-)

moshe weitzman’s picture

Status: Needs review » Needs work

Tested this and it works really nicely. This is going to be fantastic.

  1. On admin/config/media/picturemapping/add, we need better form element descriptions.
  2. Typo in s/on/one: "For each breakpoint you can select a corresponding image style, you have to select at least on image style.". Also this seems like duplicate text.
+++ b/core/modules/picture/picture.module
@@ -0,0 +1,328 @@
+/**
+ * Picture uri callback.
+ */

Duplicate Doxygen

larowlan’s picture

If you reroll for #48
+ // Create user. + $this->admin_user = $this->drupalCreateUser(array('administer pictures', 'access content', 'access administration pages', 'administer site configuration', 'administer content types', 'administer nodes', 'create article content', 'edit any article content', 'delete any article content', 'administer image styles')); + $this->drupalLogin($this->admin_user);
Can these go one per line as per coding standards
Oh and this is awesome btw!

attiks’s picture

Status: Needs work » Needs review
FileSize
4.85 KB
50.99 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch picture-50.patch. Unable to apply patch. See the log in the details link for more information. View

New patch addressing #48, #49

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks

webchick’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/picture/picture.moduleundefined
@@ -0,0 +1,329 @@
+    'website' => 'https://github.com/attiks/picturefill-proposal',

Like we did with mediaMatcher, let's point this at a Drupal URL rather than a github URL for one specific individual (especially one with "proposal" in the name :D)

+++ b/core/modules/picture/picture.moduleundefined
@@ -0,0 +1,329 @@
+function theme_picture_formatter($variables) {
...
+function theme_picture($variables) {
...
+function theme_picture_source($variables) {
...
+function picture_get_image_dimensions($variables) {

We need to make sure these functions all have PHPDoc above them, as well as @param / @return docs. Part of the docs gate.

Apart from this, great work! happy to commit it. :)

attiks’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.42 KB
52.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch picture-53.patch. Unable to apply patch. See the log in the details link for more information. View

new patch addressing #52 and some white space issues.

Back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. YAY! :D

yched’s picture

Status: Fixed » Active

Couple remarks on the code that got committed :

+++ b/core/modules/picture/lib/Drupal/picture/Plugin/Core/Entity/PictureMapping.php
@@ -0,0 +1,161 @@
+/**
+ * Defines the Picture entity.
+ * ¶
+ * @Plugin(

Whitespace slipped in.

+++ b/core/modules/picture/lib/Drupal/picture/Plugin/field/formatter/PictureFormatter.php
@@ -0,0 +1,189 @@
+          if (isset($picture_mapping->breakpointGroup->breakpoints[$breakpoint_name])) {
+          $breakpoint = $picture_mapping->breakpointGroup->breakpoints[$breakpoint_name];

indent issue - and not clear where that if block ends

+++ b/core/modules/picture/lib/Drupal/picture/Plugin/field/formatter/PictureFormatter.php
@@ -0,0 +1,189 @@
+              // Make sure the multiplier still exists.
+              //if (!empty(array_intersect($multiplier, $breakpoint->multipliers))) {

leftover commented line ?

+++ b/core/modules/picture/picture.info
@@ -0,0 +1,9 @@
+dependencies[] = config

Why should this have a dependency on config.module ? config.module is only about config UI, not about the config system

+++ b/core/modules/picture/picture.module
@@ -0,0 +1,379 @@
+    // add source tags to the output.

Sentence should be capitalized

+++ b/core/modules/picture/picture.module
@@ -0,0 +1,379 @@
+    // output the fallback image.

Ditto.

+++ b/core/modules/picture/picturefill/picturefill.js
@@ -0,0 +1,126 @@
+        // if there's no media specified, OR w.matchMedia is supported

Ditto, plus missing trailing point - plus, that's not a real sentence :-)

tim.plunkett’s picture

Also, can the picture.info description be expanded past "Picture element"? That doesn't tell me much.

Eric_A’s picture

Status: Active » Fixed

Needs a follow-up for coding standard issues.

+/**
+ * @file
+ * Definition of Drupal\picture\PictureFormController.
+ */

All of these should be like:

Contains Drupal\picture\PictureFormController.
+  /**
+   * Load breakpoint group.
+   */
+  protected function loadBreakpointGroup() {
+  /**
+   * Test picture administration functionality.
+   */
+  public function testPictureAdmin() {

More of these around that should be third person.

Eric_A’s picture

Status: Fixed » Active

Cross post. Restoring status.

attiks’s picture

Assigned: Unassigned » attiks

working on this

attiks’s picture

Status: Active » Needs review
FileSize
7.3 KB
PASSED: [[SimpleTest]]: [MySQL] 46,880 pass(es). View

New patch for #55, #56 and #57

tim.plunkett’s picture

Status: Needs review » Needs work

I don't know if the bot can read that patch, but I can't. :)

attiks’s picture

FileSize
6.37 KB
PASSED: [[SimpleTest]]: [MySQL] 46,895 pass(es). View

#61 A more readable version

attiks’s picture

Status: Needs work » Needs review
yched’s picture

Status: Needs review » Reviewed & tested by the community

looks good, thanks !
(checked with @alexpott that picture and breakpoint indeed have no reason to depend on config.module)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, i1775530-62.patch, failed testing.

attiks’s picture

retested, bot is a bit sleepy

yched’s picture

Status: Needs work » Reviewed & tested by the community

green

attiks’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.39 KB
9.65 KB
PASSED: [[SimpleTest]]: [MySQL] 47,628 pass(es). View

I reviewed the documentation with @jhodgdon

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Still RTBC if bot is happy.

tim.plunkett’s picture

Note: The interdiff is backwards :)

But the current patch looks good.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x.

attiks’s picture

FYI: Follow up created to handle AJAX requests: #1836860: Picture doesn't work with AJAX callback

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

moshe weitzman’s picture

Do we have issues open for actually using this by default? I would think we want to ship with picture enabled in standard install and picture formatter on the image fields in Article content type and on User entity (i.e. user picture).

attiks’s picture

#74 AFAIK there are no issues created yet, it would be nice to enable and use it by default.

attiks’s picture

attiks’s picture

Issue summary: View changes

Updated issue summary.