Follow up for #1883526: Decide on the picture polyfill to use

Problem/Motivation

If you don't want to output an image on for instance mobile phones, you now need to use display:none; to hide it, but there's still a request for the specified image.

Proposed resolution

The Drupal 7 version of the picture module has an option to map a breakpoint to an empty image. If you select this option the image source is replaced by 'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7'. So it adds like 50 bytes to the request, but it will not fire a new HTTP request.

Remaining tasks

  1. Decide if we want this
  2. Write a patch
  3. Test

User interface changes

A new option is added on admin/settings/picture/group

API changes

None

Files: 
CommentFileSizeAuthor
#21 1999312-21.patch3.68 KBlokapujya
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,403 pass(es).
[ View ]
#17 picture-1999312-17.patch3.36 KBbotanic_spark
PASSED: [[SimpleTest]]: [MySQL] 63,181 pass(es).
[ View ]
#9 Screen Shot 2013-05-24 at 1.13.00 PM.png131.52 KBevanmwillhite
#9 Screen Shot 2013-05-24 at 1.13.16 PM.png50.38 KBevanmwillhite
#5 i1999312-5.patch12.1 KBattiks
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1999312-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Priority:Normal» Major

This has the potential to have massive, huge, epic performance impact for Drupal on mobile devices. Many saved requests, kilobytes or even megabytes… means that this patch can potentially bring a site from loading in 20 seconds on a mobile device to 2 seconds. It depends on the site of course.

Increasing priority — even though that's likely to be contentious. This issue deserves more attention.

Pretty sweet, huh. Attiks is the man.

Back in March, I presented this base64 empty image idea to him and by the next day it was in 7.x-dev. Tested it and worked perfectly from the start. I'd be more than happy to test it on D8 if it gets patched in.

On mobile, there are times when certain images are just not necessary and using this method saves an http request as well as the image download. I've used this method in production and it does work as you'd imagine for increasing performance.

I'm sure the front-end geniuses will come up with an answer on whether or not there's a better way to handle this.

#2 Thanks for the idea and your help on D7 version.

I had a quick word with Morten and there's definitely a need for this in Drupal 8.

I'll roll a patch later today/tomorrow

+1 to that as well.

Status:Active» Needs review
StatusFileSize
new12.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1999312-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Combined patch so people can test this, this patch also contains the one from #1883526: Decide on the picture polyfill to use

The last submitted patch, i1999312-5.patch, failed testing.

#5: i1999312-5.patch queued for re-testing.

Issue tags:+Needs manual testing

Tagging

StatusFileSize
new50.38 KB
new131.52 KB

Patch applied fine. Added picture mapping for 1x mobile and 1x narrow and large image style for 1x wide. Image replacement occurred with no issues/console errors. (see screenshots). Tested in Chrome/Firefox on Mac

#9 Thank you

Issue tags:-Needs manual testing

The last submitted patch, i1999312-5.patch, failed testing.

Category:feature» task

Nice idea. Not really a feature, this is a straight performance improvement.

Does this need a reroll?

After seeing #9 could someone clarify if the display:none; CSS code remains, or is it removed in favour of displaying a 1px x 1px empty image?

If it remains then good.

But if it is removed then not so good because this is more the expected behaviour of visibility:hidden; rather than display:none;.

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new3.36 KB
PASSED: [[SimpleTest]]: [MySQL] 63,181 pass(es).
[ View ]

Modification of the patch #5.
Modification of the image_cache functions.

I also saw that in patch #5 picture tag was converted to span. Not sure why this was done, but i used the picture tag in this patch.

Status:Needs review» Needs work

We should be using span elements, not picture elements. The picture element has not yet been standardized and implemented in browsers. It looks highly likely that will happen, but at this point we should be using a polyfill based on spans rather than picture elements. Otherwise if the picture element changes, then we have code written against the old version of the standard, which is bad.

As the issue linked in #5 explains, if picture is standardized and implemented prior to d8 launch, we can switch to the picture element. Otherwise that's a switch that could be considered for 8.1 or beyond.

Issue tags:-revisit before beta

Removing 'revisit' tag - could happen any time I think?

Issue tags:+Needs reroll

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

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new3.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,403 pass(es).
[ View ]

Rerolled. Renamed the define and _picture_image_style_url() to _responsive_image_image_style_url().

Status:Needs review» Needs work

The last submitted patch, 21: 1999312-21.patch, failed testing.

Status:Needs work» Needs review

21: 1999312-21.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Nice job!

Title:Add an 'empty image' option for pictureAdd an 'empty image' option for responsive image
Status:Reviewed & tested by the community» Needs work
  1. +++ b/core/modules/responsive_image/lib/Drupal/responsive_image/ResponsiveImageMappingFormController.php
    @@ -71,6 +71,7 @@ public function form(array $form, array &$form_state) {
    +    $image_styles[RESPONSIVE_IMAGE_EMPTY_IMAGE] = t('- empty image -');

    $this->t()

  2. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -8,6 +8,8 @@
    +define('RESPONSIVE_IMAGE_EMPTY_IMAGE', '_empty image_');

    Why are we using define() and not const?

  3. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -353,7 +355,25 @@ function responsive_image_get_image_dimensions($variables) {
    +    return 'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7';

    This could probably use a comment.

Component:picture.module» responsive_image.module