Problem/Motivation

If you have a 'crop' image style and the image is not large enough to fill the cropped area, the resulting image has a black background in the empty space. It would be useful if the default background colour were configurable.

Note that the Image Effects module provides this functionality.

Proposed resolution

Adding a background color to the options for the 'Crop' effect when creating an image style.

Remaining tasks

N/A

User interface changes

When configuring a Crop (image_crop) effect at admin/config/media/image-styles/manage/{image_style}/add/image_rotate or admin/config/media/image-styles/manage/{image_style}/effects/{image_effect} admins should be able to provide a color, in hex string format, for the additional area created by upscaling crops.

API changes

Note: These API changes assure full backward compatibility.

Original report by FiReaNG3L

ImageAPI.module sounds very interesting; I recently had problems with imagecache / image.inc putting a black background on images when scaling / cropping to a definite size (say 150x150 pixels) when the image is too wide or tall to fit the square.

Is there a way to customize the background to another color (say, white) with image API?

I tried to modify image.inc myself but the best I could achieve is a background change that worked, but bleeded in the image if black was present.

Thanks for all the awesome work on imagecache / imageAPI!

CommentFileSizeAuthor
#141 drupal-image-crop-204497-141.patch29.25 KBclaudiu.cristea
#138 drupal-image-crop-204497-138.patch27.91 KBclaudiu.cristea
#140 drupal-image-crop-204497-140.patch27.91 KBclaudiu.cristea
#130 drupal8.crop-background-204497-130.patch19.73 KBclaudiu.cristea
#124 drupal8.crop-background-204497-124.patch19.71 KBclaudiu.cristea
#122 drupal8.crop-background-204497-122.patch19.62 KBclaudiu.cristea
#120 drupal8.crop-background-204497-120.patch19.56 KBclaudiu.cristea
#116 drupal8.crop-background-204497-116.patch19.95 KBclaudiu.cristea
#114 drupal8.crop-background-204497-114.patch18.04 KBdman
#112 drupal8.crop-background-204497-112.patch18.59 KBclaudiu.cristea
#109 drupal8.crop-background-204497-109.patch17.39 KBclaudiu.cristea
#106 drupal8.crop-background-204497-106.patch16.88 KBclaudiu.cristea
#97 background.jpg62.79 KBRaulMuroc
#94 bg-white-large.png208.81 KBjantoine
#94 bg-blue-large.png208.8 KBjantoine
#94 bg-green-large.png208.8 KBjantoine
#94 bg-red-large.png208.8 KBjantoine
#94 bg-black-large.png208.8 KBjantoine
#94 bg-none-large.png208.25 KBjantoine
#92 drupal8.crop-background-204497-92.patch12.52 KBclaudiu.cristea
#91 drupal8.crop-background-204497-91.patch11.76 KBclaudiu.cristea
#88 drupal8.crop-background-204497-88.patch11.75 KBclaudiu.cristea
#86 drupal8.crop-background-204497-86.patch11.68 KBclaudiu.cristea
#83 drupal8.crop-background-204497-83.patch6.64 KBclaudiu.cristea
#74 drupal8.image-gd-crop-background.74.patch1.92 KBsun
#37 imageapibackground.patch2.03 KBRobLoach
#36 pgmatch.gif6.91 KBAnonymous (not verified)
#36 pgmatch_7.gif1.84 KBAnonymous (not verified)
#35 imageapi_background_color.patch1.9 KBedmund.kwok
#26 204497.patch2.42 KBRobLoach
#24 204497.patch2.19 KBRobLoach
#24 cropbackground.png37.73 KBRobLoach
#22 imageapi-transparent_backgrounds_02.patch3.58 KBtylor
#19 imageapi-transparent_backgrounds-20081009.patch3.57 KBdman
#17 imageapi-5.x-1.2_crop_background_1.patch4.35 KBMurz
#13 imageapi-5.x-1.1_crop_background.patch2.58 KBMurz
#9 image_crop_transparent_1.patch1.27 KBMurz
#8 Sweetheart-Leather.jpg10.9 KBidflorin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

FiReaNGeL’s picture

Small followup: a search on d.o. reveals that many users have the same problem; maybe ImageAPI.module would be a nice place to fix this since they are mostly related to imagecache.module and the new version isnt using image.inc anymore!

Non-extensive list:

Imagecache Scale Problem (Black background) : http://drupal.org/node/201163
Background in cropped images : http://drupal.org/node/123270
Changing Background Color of Thumbnails Re: CCK/VIews Image Gallery : http://drupal.org/node/190235

All the workarounds thus far involve doing some other action to avoid the crop, but if thats the result you want and nothing else, you're stuck with a black background, which is ugly if your theme is white based.

Also image_exact.module (http://drupal.org/project/image_exact) states in its description : The only limitation of this implementation is that imagecache doesn't check the image size before cropping, so if someone uploads a very small image, it will "crop" to the size specified, adding a black background to fill in the dimensions. I will submit a patch to imagecache that will make this configurable.

And here's a possible fix:

Here is a Patch to avoid black backgrounds on cropped images : http://drupal.org/node/199957

dopry’s picture

I'd really prefer you test the module rather than post feature requests right now. I would also be nice if you contributed this image.inc fix you attempted as a possible patch instead of just mentioning it. Code is gold and all..

FiReaNGeL’s picture

As I said, there's a potential fix in the last issue I linked to - image.inc should be patched, but imageAPI too because its the future of imagecache, which is the main resource using image manipulation. I will test the patch as soon as I get back home (christmas vacation and all). This issue is just to document the problem as a feature request against the dev version, so its not like the house is on fire or something :)

Murz’s picture

I have successfully change background color when cropping with easy hack of Drupal core file includes/image.inc:
In function

 function image_gd_crop($source, $destination, $x, $y, $width, $height) {
...
  imageCopy($res, $im, 0, 0, $x, $y, $width, $height);
  $result = image_gd_close($res, $destination, $info['extension']);
...
} 

add string:

 function image_gd_crop($source, $destination, $x, $y, $width, $height) {
...
  imageCopy($res, $im, 0, 0, $x, $y, $width, $height);
  imageFill($res, 0, 0, imageColorAllocate($res,255,255,255) );
  $result = image_gd_close($res, $destination, $info['extension']);
...
} 

When I try to add imageFill function before imageCopy, i see black backgrond. But when I add it after imageCopy, all works good.

Numbers 255,255,255 you can change to any RGB color values as you want.

Murz’s picture

Here is a Patch to avoid black backgrounds on cropped images : http://drupal.org/node/199957

This issue doesn't solve problem with black backgrounds, it's avoid clipping if size of clip area large than image.

Murz’s picture

Another (better) method to fill background with some color:
In function

function image_gd_crop($source, $destination, $x, $y, $width, $height) {
...
  $res = imageCreateTrueColor($width, $height);
  imageCopy($res, $im, 0, 0, $x, $y, $width, $height);
  $result = image_gd_close($res, $destination, $info['extension']);
...
}

add string:

function image_gd_crop($source, $destination, $x, $y, $width, $height) {
...
  $res = imageCreateTrueColor($width, $height);
  imagefilledrectangle($res, 0, 0, $width, $height, imagecolorallocate($res,255,255,255) );
  imageCopy($res, $im, 0, 0, $x, $y, $width, $height);
  $result = image_gd_close($res, $destination, $info['extension']);
...
}
joeboris’s picture

I've tried this, and not been able to make it work... Nothing at all happens... Any idea as to what I could be doing wrong? I'm using Imagecache 1.3 on Drupal 5.6.

I tried the other hack you posted, and that worked like a charm - that is, until I uploaded an image consisting mainly of black... Apparently, imageFill doesn't handle alpha-colors too well...

Nevertheless; if you (or anyone else for that matter) have any more ideas on how to make this work, it would be greatly appreciated! It's such a bummer going back to the black background images after having it "working" sweetly for a while ;)

Thanks
joeboris

idflorin’s picture

FileSize
10.9 KB

#4 works partially (see attachment)
#6 no change.
Now I'll try both.
--------------------------
Velvet Cushion

Murz’s picture

adresaklumea, I create another method for filling background when cropping, it should be work good with half -transparent png's too. Try my new patch and feedback please about it works.
You must set background color as HTML color in string
$bg_color = variable_get('image_crop_background', '#FF0000');
or set a Drupal variable 'image_crop_background' to HTML color you need.
This patch is for Drupal 5.7 core, not for ImageAPI module.

idflorin’s picture

Beautiful, works like a charm
10*e99 thanks

idflorin’s picture

How to patch the ImageAPI ?

reed.richards’s picture

I tried the patch in #9 but my whole site goes down and I get the following error messages:

Warning: require_once(./includes/image.inc) [function.require-once]: failed to open stream: Permission denied in drupal\includes\common.inc on line 1850

Fatal error: require_once() [function.require]: Failed opening required './includes/image.inc' (include_path='.;C:\php5\pear') in drupal\includes\common.inc on line 1850

Which is really strange I don't know where this comes from? I am using the 5.7 Drupal.

Murz’s picture

This is a patch for imageapi to set the background color for cropping.

At now you can set the color manually in source code by modifying this string:
$color=array('red'=>255,'green'=>255,'blue'=>255,'alpha'=>0);

And this patch have a workaround with wrong image size for function imagecopyresampled() when cropping.

idflorin’s picture

Thank you again.
you rock!

dopry’s picture

Status: Active » Needs work

This still isn't quite the right solution... I'm tempted to split canvas resize and crop into seperate functions and not allow oversized crops.... and have an explicit set background for canvas resizing that inherits from the image->res if unset.

regardless this breaks background transparency preservation in it current state.

.darrel.

drew reece’s picture

Subscribing,

The patch in #9 works for me, using Imagecache 5.x-1.6 and Drupal 5.9. FWIW I use scaling to outside dimensions followed by cropping slightly smaller. The background now blends nicely.

I assume the plan is to make the string value assignable in the image toolkit admin page?

Something along the lines of

<?php
function image_gd_settings() {
…

$form['image_crop_background'] = array(
  '#type' => 'textfield',
  '#title' => t('Background color'),
  '#description' => t('Define the image background color for manipulations. Values need to be hexdecimal, eg #FFFFFF is white.'),
  '#size' => 7,
  '#maxlength' => 7,
  '#default_value' => variable_get('image_crop_background', "#FFFFFF"),
  );

…
}
?>

A couple of random thoughts…
The default should be probably white, since it would sit well with default theme(s).
The # should be removed from the field and only the 6 characters are saved for the color variable, eg ff00ff I think we can use a field prefix to display the # at the start of the field.

@ dopry, I didn't realize there was transparency, my images seem to end up with a black background when cropping oversize. Is there anything I can do, this strikes me as a nice feature to get into core, I'd easily use it on all my sites.

Drew

Murz’s picture

I have edited the patch for support imageapi-5.x-1.2 and add an option in imageapi_gd settings to set background color manually. It's worked for me and, I hope, help others.

drewish’s picture

Version: 5.x-1.x-dev » 6.x-1.x-dev

this would really need to go into HEAD (6.x-1.x) and be backported.

dman’s picture

Title: Background color when scaling / cropping - customizable? » Background color when scaling / cropping. And a fix for transparent backgrounds.
Status: Needs work » Needs review
FileSize
3.57 KB

Looking at #7, It's wrong to use variable_set/get for this which should be a per-preset setting.

Anyway...

I've tweaked imageapi to :
- Properly support transparent inputs - it just wasn't doing it before. Now if you crop a PNG or GIF the background will remain transparent - even if cropped to a larger size. There was code there to support this but it seems to have rotted away at some point and stopped doing it properly.
- Allow crop to larger dimensions : behave like "set canvas size" - without creating huge black chunks. Jpegs still create white chunks, but that's less ugly usually.

It involved moving jpeg handling over next to png handling (truecolor), as the previous index transparency only works on GIFs AFAIK.

In the future, setting the preferred bg color via the UI is still a TODO

Patch is against todays HEAD/6-dev

Index: imageapi_gd.module
===================================================================
RCS file: /cvs/drupal-contrib/contributions/modules/imageapi/imageapi_gd.module,v
retrieving revision 1.12
diff -u -p -r1.12 imageapi_gd.module
--- imageapi_gd.module	9 Jul 2008 02:49:47 -0000	1.12
+++ imageapi_gd.module	9 Oct 2008 07:28:38 -0000
@@ -32,6 +32,10 @@ function imageapi_gd_settings_form() {
 
 function imageapi_gd_image_open($image) {
   if ($image->res =  _gd_open($image->source, $image->info['extension'])) {
+    // Retain transparency for PNGs
+    if ($image->info['extension'] == 'png') {
+      imagesavealpha($image->res, TRUE);
+    }
     return $image;
   }
   return false;
@@ -44,7 +48,11 @@ function imageapi_gd_image_close($image,
 function imageapi_gd_image_crop(&$image, $x, $y, $width, $height) {
   $res = _gd_create_tmp($image, $width, $height);
 
-  if (!imagecopyresampled($res, $image->res, 0, 0, $x, $y, $width, $height, $width, $height)) {
+  // Support 'cropping' to a size larger than the source - but do not copy null values into new canvas - so PNGs and GIFs can remain transparent.
+  $targetwidth = min($image->info['width'], $width);
+  $targetheight = min($image->info['height'], $height);
+
+  if (!imagecopyresampled($res, $image->res, 0, 0, $x, $y, $targetwidth, $targetheight, $targetwidth, $targetheight)) {
     return false;
   }
   // destroy the original image and return the modified image.
@@ -134,7 +142,7 @@ if (!function_exists('imagefilter')) {
 function _gd_create_tmp($image, $width, $height) { 
   $res = imagecreatetruecolor($width, $height);
   
-  if ($image->info['extension'] == 'gif' || $image->info['extension'] == 'jpg') {
+  if ($image->info['extension'] == 'gif') {
     // grab transparent color index from src.
     $transparent = imagecolortransparent($image->res);
 
@@ -142,22 +150,23 @@ function _gd_create_tmp($image, $width, 
     if ($transparent >= 0) {
       // get color(r,g,b) for index;
       $transparent = imagecolorsforindex($image->res, $transparent);
-      // allocate to new image and get new index.
-      $transparent =  (isset($color['alpha'])) 
-        ? imagecolorallocatealpha($res, $color['red'], $color['green'], $color['blue'], $color['alpha'])
-        : imagecolorallocate($res, $color['red'], $color['green'], $color['blue']);
+      // Allocate the values to the new image and get new color.
+      $transparent =  (isset($transparent['alpha'])) 
+        ? imagecolorallocatealpha($res, $transparent['red'], $transparent['green'], $transparent['blue'], $transparent['alpha'])
+        : imagecolorallocate($res, $transparent['red'], $transparent['green'], $transparent['blue']);
  
-      $transparent = imagecolorallocate($res,  $transparent['red'], $transparent['green'],  $transparent['blue']);
       // flood with our new transparent color.
       imagefill($res, 0, 0, $transparent);
       // tell the new image which color is transparent.
       imagecolortransparent($res, $transparent);
     }
   }
-  elseif ($image->info['extension'] == 'png') {
+  elseif ($image->info['extension'] == 'png' || $image->info['extension'] == 'jpg') {
+    imagesavealpha($res, true);
     imagealphablending($res, false);
-    $transparency = imagecolorallocatealpha($res, 0, 0, 0, 127);
-    imagefill($res, 0, 0, $transparency);
+    $transparency = imagecolorallocatealpha($res, 255, 255, 255, 127);
+    #imagefill($res, 0, 0, $transparency); // imagefill doesn't always work with transparent apparently
+    imagefilledrectangle($res, 0, 0, $width, $height, $transparency);
     imagealphablending($res, true);
     imagesavealpha($res, true);
   }
claudiu.cristea’s picture

Input on patch #19...

If you set the X offset to right than a black background is added to the left and the image is cropped.

Steps to reproduce:

  • Source image: 100px x 100px
  • Add a preset with a CROP action
  • Set the Crop action to...
    • With: 120px
    • Height: 120px
    • X offset: right
    • Y offset: top

The image should be surround at left & bottom by a white background. At bottom is OK but on X axis it adds white background to right and black background to left. Also it reduces the X dimension by cropping the image.

claudiu.cristea’s picture

But... it works fine with X offset set to left

tylor’s picture

Input for #19 and #20, I had the same problem but found I could make everything work (left, right, and center) by changing:

if (!imagecopyresampled($res, $image->res, 0, 0, $x, $y, $targetwidth, $targetheight, $targetwidth, $targetheight))

to

if (!imagecopyresampled($res, $image->res, 0, 0, $x*(-1), $y*(-1), $targetwidth, $targetheight, $targetwidth, $targetheight))

Attached is a new patch with this change.

VVVi’s picture

For CENTRING I have applied patch from #22 and change one row in function imageapi_gd_image_crop from

if (!imagecopyresampled($res, $image->res, 0, 0, $x*(-1), $y*(-1), $targetwidth, $targetheight, $targetwidth, $targetheight))

to

   if (!imagecopyresampled($res, $image->res, ($width-$image->info['width'])/2-$x, ($height-$image->info['height'])/2-$y, ($x)*(-1), $y*(-1), $targetwidth, $targetheight, $targetwidth, $targetheight)) { 

and all work fine for me)
I use drupal 6

RobLoach’s picture

Priority: Normal » Critical
FileSize
37.73 KB
2.19 KB

How's this? Moving this to critical since it's been an issue for so long. Here's a patch as well as a screenshot.

The patch was created from HEAD.

drewish’s picture

I'd like some extra comments so it's clear what's happening there. why are/were we passing the $width, $height parameters if we're using $image->info['height'], $image->info['width']?

RobLoach’s picture

FileSize
2.42 KB

Here we go. This patch says why we're using the source width and height instead of the destination width and height. Think we should remind people that we're cropping here and not scaling? Or is that good? I thought it made sense.....

drewish’s picture

Status: Needs review » Needs work

the comments look better... but looking at the docs for imagecopyresampled() it references them as the src and dest sizes so wouldn't the first set be $height and $width and the second be $image['height'] and $image['width']?

this has me really wishing we had some unit tests to validate some of these type of changes.

and if we're adding a feature we need to add it to the imagemagick api or at least determine that it's unsupported by IM...

dman’s picture

I've been wishing for unit tests for my image processes also ... but I just cannot imagine how to validate that the produced image is what I expected :-)

FiReaNGeL’s picture

dman: maybe check the output / md5 with http://ca.php.net/function.file-get-contents compared to an 'expected' image from a source image

dman’s picture

Byte/hash matching won't really help for JPEG algorithms against different builds, libraries, image toolkits, compression preferences, OS etc.
:-(

RobLoach’s picture

Andrew: We're using the same width and height for both the source and destination because we are cropping, not scaling. If we were scaling, we'd use different source/destination widths and heights.

drewish’s picture

Status: Needs work » Fixed

that makes sense. okay this looks pretty good to me. committed to HEAD. thanks!

RobLoach’s picture

Yeah, should've included that as part of the patch with the added documentation. Thanks a lot, Drewish.

Status: Fixed » Closed (fixed)

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

edmund.kwok’s picture

Priority: Critical » Normal
Status: Closed (fixed) » Needs review
FileSize
1.9 KB

This patch is not in the latest 1.x-dev? Updated patch for this.

Anonymous’s picture

FileSize
1.84 KB
6.91 KB

Above patch works for png and jpg, but not for gif images. Im scaling images to fit e.g. 100x100, and then cropping to 100x100. See attached gif images problem: only the top part gets the #FFF background I set in the ImageAPI config page. The bottom part is still black. Why doesnt it apply the set background color for the entire image, why only the top part?

RobLoach’s picture

FileSize
2.03 KB

Updated to latest in DRUPAL-6--1.

R.Muilwijk’s picture

Subsc

moritzz’s picture

The #37 patch works for me. Thanks a lot.

p.brouwers’s picture

Bobuido’s picture

Subscribing

jsenich’s picture

subscribing

arnieswap’s picture

Did not work for me.

blueblade’s picture

subscribing

arski’s picture

wouldn't it be nice to check image dimensions rather than filling it with whatever background on crop?

Thanks

john.kenney’s picture

#37 patch worked for me in 1.8.

very nice! thank you.

it probably says this on the thread somewhere, but you can modify color background by editing line #116 in the patched file. for example:

$background = variable_get('imageapi_crop_background', '#cccccc');

Wappie08’s picture

Status: Needs review » Active

Hello, I can confirm that the patch in #37 is working!

Will this be committed, what's the status on this issue?

Greetings Wappie

smyleeface’s picture

Status: Active » Reviewed & tested by the community

I can also confirm that patch #37 works perfectly with ImageAPI 6.x-1.8 and ImageCache 6.x-2.0-beta10.

@#46 instead of coding the color into the code you can go into the ImageAPI configuration screen and set the color in there.

And don't forget to flush the cache to see the changes. :)

...

marked rtbc

john.kenney’s picture

@#49 oh, yes. that's much easier. thanks.

arski’s picture

#49 john_kenney - June 22, 2010 - 20:35
@#49 oh, yes. that's much easier. thanks.

:))) Umm, talking to yourself there? ;)

john.kenney’s picture

right. i meant #48.

been a long day... :)

arnieswap’s picture

Confirm...the patch works fine...thanks a lot...

Firnas’s picture

Version: 6.x-1.x-dev » 6.x-1.6

#37 worked for me also on 1.6

ac’s picture

#37 works as expected. Please commit this ASAP

Agogo’s picture

#37 works very good for me as well and I agree - please commit this ASAP!

brenda003’s picture

Works great here. @#43, if it's not working for you please explain how.

zdean’s picture

subscribing

ac’s picture

Could this please be commited to HEAD (If it has been committed could the maintainer close this issue)? There seem to be enough positive community reviews here.

Leglaw’s picture

I second the motion to commit crop background color patch added to HEAD.

ayalon’s picture

works..

please commit this longstainding functionality.

RobLoach’s picture

Version: 6.x-1.6 » 6.x-1.x-dev

Herp derp? Should we push this to 7.x at this point? Might need an update to DRUPAL-6--1?

perandre’s picture

Patch in #37 works for setting bg color. Not sure about the transparent bg part.

drew reece’s picture

Are there outstanding issues with this patch? Or are we now abandoned because our sites are on Drupal 6?
This has been fixed since October 2009 (post 37), and it still isn't showing up in the dev or the 6.x-1.9 releases.

uno’s picture

What about ImageCache actions, Define Canvas action does this pretty good out of the box.

mattcasey’s picture

I manually patched dev and it worked great, thanks

hansrossel’s picture

#37 works as expected. Please commit.

drewish’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 6.x-1.x

ymeiner’s picture

I converted the change for drupal 7, do not know if there is another issue for it so please delete or move if needed. Here are some notes:

  • The file to change is image.gd.inc in /modules/system/
  • To keep the same language, the name of the field should be 'image_crop_background'
  • drupal contrib shows that there is imageapi_hex2rgba, I could not find it. in order to make it work, I added the function at the end of the file.
  • A suggestion - Add the background color to the crop form in the styles admin (image.admin.inc). In order to do it there will be a need for a change in the way that you send variables to image gd. It could be better to send an array with options than a different variable to every definition (less changes in every code enhancement, function definitions stay the same).
  • There is no validation, i added a validation to check if the number entered is hexadecimal.

my code:

function image_gd_settings() {
...

...
$form['image_crop_background'] = array(
    '#type' => 'textfield',
    '#title' => t('Crop background'),
    '#description' => t('Hex string specifying the background color to use when cropping images. If not provided, will use the default. Examples: "ABC", "ABCD", "AABBCC", "AABBCCDD".'),
    '#size' => 10,
    '#maxlength' => 8,
    '#default_value' => variable_get('image_crop_background', ''),
    '#field_prefix' => '#',
	);
...
function image_gd_settings_validate($form, &$form_state) {
  // Validate image quality range.
  $value = $form_state['values']['image_jpeg_quality'];
  if (!is_numeric($value) || $value < 0 || $value > 100) {
    form_set_error('image_jpeg_quality', t('JPEG quality must be a number between 0 and 100.'));
  }
  $value = $form_state['values']['image_crop_background'];
  if (!($value=='')&&!ctype_xdigit ($value)){
	form_set_error('image_crop_background', t('Value entered is not hexadecimal.'));
  }
}
function image_gd_crop(stdClass $image, $x, $y, $width, $height) {
  $res = image_gd_create_tmp($image, $width, $height);
  
  // Fill the background color if desired.
  $background = variable_get('image_crop_background');
  if (!empty($background)) {
    $background = imageapi_hex2rgba($background);
    $background = imagecolorallocatealpha($res, $background[0], $background[1], $background[2], $background[3]);
    imagefill($res, 0, 0, $background);
  }

  // Copy the source image to our new destination image. We use
  // $image->info['width] instead of $width because we are copying
  // using the source image's width and height, not the destination
  // width and height.
  if (!imagecopyresampled($res, $image->resource, -$x, -$y, 0, 0, $image->info['width'], $image->info['height'], $image->info['width'], $image->info['height'])) {
    return FALSE;
  }
...

link to the imageapi_hex2rgba function

Wappie08’s picture

Project: ImageAPI » Drupal core
Version: 6.x-1.x-dev » 7.x-dev
Component: ImageAPI GD » image system
Status: Fixed » Needs review

Hi ymeiner, thanks very much for this code, I can confirm that this works!

one thing: your link to imageapi_hex2rgba isn't working, this should be: http://drupalcontrib.org/api/drupal/contributions--imageapi--imageapi.mo...


I moved this issue to drupal core as this is now in core, maybe someone can comment on the need for this to be in d7, I personally think that this should be an option one should be able to set!


Greetings Wappie

Status: Needs review » Needs work

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

RobLoach’s picture

Version: 7.x-dev » 8.x-dev
Category: feature » bug

Into Drupal 8.x before hitting Drupal 7.x.

dale386’s picture

Version: 8.x-dev » 7.2
Category: bug » feature

This is a great, simple, and useful feature. Is there a formal request in to have this included in the next build of D7?

RobLoach’s picture

Version: 7.2 » 8.x-dev
Issue tags: +Needs backport to D7

Need in D8 first, then can backport to Drupal 7.

sun’s picture

Status: Needs work » Needs review
FileSize
1.92 KB

Re-rolled against D8.

jantoine’s picture

subscribe

katherinecory’s picture

subscribe for 7.x

oxydia’s picture

#26: 204497.patch queued for re-testing.

bendiy’s picture

I just applied this to Drupal 7.8. It works great.

Only issue is see is that the "Crop background color " hex string value you enter in the form at admin/config/media/image-toolkit is not displayed after saving. That hex string you enter should be displayed when you return to admin/config/media/image-toolkit in the future.

bforchhammer’s picture

For anyone looking for a quick solution (D7) to this problem: the Imagecache Actions project provides a "Define canvas" action which essentially works the same way as a "crop", but also allows to define a background color.

RaulMuroc’s picture

#37: imageapibackground.patch queued for re-testing.

xjm’s picture

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

Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

claudiu.cristea’s picture

Assigned: claudiu.cristea » Unassigned
Status: Needs review » Needs work

With all respect to the work that has been done here, my believe is that this is NOT the right approach. This patch let site admins setting a global background color which is not correct. You may have styles/presets that are integrated on a layout with white background - there you want a white background for cropped image. Other style/preset may be placed on a dark page - there you want other background color. Et cetera...

The point is that background image when cropping must be a per-style setting and NOT a global setting.

EDIT: Just like when rotating.

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea
Status: Needs work » Needs review
FileSize
6.64 KB

Here's patch made in the idea of #82.

Just a note related to #74. That patch uses a function imageapi_hex2rgba() that is not part of the core. I wonder how the patch passed the automated tests :)

+++ b/modules/system/image.gd.inc
@@ -185,9 +195,21 @@ function image_gd_rotate(stdClass $image, $degrees, $background = NULL) {
+    $background = imageapi_hex2rgba($background);

In the proposed patch I added that function to core (image.inc) because I saw that we can reuse that code, for example in image_gd_rotate() where is something similar.

xjm’s picture

The approach in #83 is interesting. Thanks for the patch.

If it passed automated tests with a function that does not exist, that indicates to me that we are missing test coverage for it. Also, let's get some manual testing with #83. Screenshots and an issue summary would be helpful too. :)

xjm’s picture

Status: Needs work » Needs review

Oops, let's leave at NR now for more patch reviews and testing.

claudiu.cristea’s picture

Title: Background color when scaling / cropping. And a fix for transparent backgrounds. » Configurable background color when cropping to larger dimensions
Issue tags: -Needs tests, -Needs issue summary update, -Novice, -Needs manual testing
FileSize
11.68 KB

I already worked on tests :) Added Issue summary and automatic tests.

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Thanks @claudiu.cristea! I read through the patch again and I have a few small suggestions.

+++ b/core/includes/image.incundefined
@@ -438,5 +444,53 @@ function image_save(stdClass $image, $destination = NULL) {
+ * Convert a hex string to its RGBA (Red, Green, Blue, Alpha) integer
+ * components.

Let's try to shorten this slightly so it fits on one 80-character line. How about, "Converts a hex string to RGBA (Red, Green, Blue, Alpha) integer values."

+++ b/core/includes/image.incundefined
@@ -438,5 +444,53 @@ function image_save(stdClass $image, $destination = NULL) {
+ * @return
+ *   An array with four elements for red, green, blue, and alpha.
...
+  return array($r, $g, $b, $a);

Let's use associative keys for these similar to those used elsewhere ('red', etc.)

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -458,3 +458,130 @@ class ImageToolkitGdTestCase extends DrupalWebTestCase {
+ * Test if cropping to larger dimensions add
+ * the correct color in the uncovered area.

Let's change this to something like "Tests image cropping functionality." (That way it's a good container for additional tests that could use this setup.) In general, function/class/method summaries should be a single line, 80 chars or less, and begin with a verb in the 3rd person present (e.g. "Tests"). (Reference: http://drupal.org/node/1354#functions)

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -458,3 +458,130 @@ class ImageToolkitGdTestCase extends DrupalWebTestCase {
+   * Testing crop backgound color.

For this one, how about: "Tests that image cropping uses the correct background color."

claudiu.cristea’s picture

OK. Attached together with other small changes.

Status: Needs review » Needs work

The last submitted patch, drupal8.crop-background-204497-88.patch, failed testing.

xjm’s picture

Ah, looks like the test needs a little adjustment:

Undefined variable: r	Notice	image.inc	485	image_hex2rgba()	
Undefined variable: g	Notice	image.inc	486	image_hex2rgba()	
Undefined variable: b	Notice	image.inc	487	image_hex2rgba()	
Undefined variable: a	Notice	image.inc	488	image_hex2rgba()	
Pixel from (5, 5) is red (#FF0000)	Other	image.test	583	ImageCropBackgroundTestCase->testCropBackgroundColor()	
claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
11.76 KB

Bypassing local tests it's a bad behavior :-)

claudiu.cristea’s picture

Some comments and typo fixes.

xjm’s picture

Issue tags: +Novice, +Needs manual testing

Code in #92 looks great to me.

Now let's get a couple different people doing that manual testing:

  1. Apply #92
  2. Create various image styles using the new background color property
  3. Verify that images are created correctly.
  4. Report your results here. Screenshots would be good (but crop them to only show the relevant part of the screen).
jantoine’s picture

Wow... I have got to say, great work!!!

Rather than providing screen shots, I have attached the actual images that were generated. My only questions is should this be added to the "Scale and Crop" effect as well?

Attached are images that were generated using the following background color values:

#000000
#FF0000
#00FF00
#0000FF
#FFFFFF

claudiu.cristea’s picture

should this be added to the "Scale and Crop" effect as well?

@AntoineSolutions, "Scale and Crop" doesn't need it while every time is cropping to a smaller area. "Scale and Crop" never creates additional new area. At least this is my understanding.

jantoine’s picture

@claudiu.cristea, You are correct. Not sure what I was thinking. In light of that, I would like to confirm that the patch is working as expected. Not sure how many tests are needed before RTBC, so I'll hold off on updating the status.

RaulMuroc’s picture

FileSize
62.79 KB

"Scale and Crop" doesn't need it while every time is cropping to a smaller area. "Scale and Crop" never creates additional new area. At least this is my understanding.

But with "Scale & Smart Crop" + Canvas the problem exists. For instance, I have defined to Scale & Crop to 540x340 and added Canvas background #969696 center,center to it and it returns a black background where it should be gray.

I tried to solve it without success.

Thx.

claudiu.cristea’s picture

@RaulMuroc, we are not dealing here with "Canvas" effect.

tanitani’s picture

#82 Claudiu -- Absolutely. Second that!

Gábor

P:S: thank you all for the work.
BTW subscribe for D7

Kvart’s picture

Looks awesome! Subscribe for D7

xjm’s picture

You don't need to "subscribe" anymore. There's a green "Follow" button in the upper-right corner of the issue. :)

Niklas Fiekas’s picture

Status: Needs review » Needs work

The form element isn't properly validated. Maybe we could even make it show the color, like the color widgets do, when modifying a theme?

claudiu.cristea’s picture

Status: Needs work » Needs review

That is not in the scope. It may be an improvement, subject of a new ticket. Right now the rotate effect is collecting the background in the same way. If works for rotate than must work also for crop.

Niklas Fiekas’s picture

While "Maybe we could even make it show the color, like the color widgets do, when modifying a theme?" is optional, the other point isn't: "The form element isn't properly validated."

We need to check that. (And the rotate effect checks it, too.) Try, for example #zzzzzz as the color code.

claudiu.cristea’s picture

Status: Needs review » Needs work

@Niklas Fiekas, you're right. But we need to unify the way how rotate and crop are validating colors. Right now, rotate is not accepting colors like #RGB but only #RRGGBB.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
16.88 KB

Here's the patch with color validation. Note that now crop & rotate background colors use the same validation mechanism and storage.

Status: Needs review » Needs work

The last submitted patch, drupal8.crop-background-204497-106.patch, failed testing.

Niklas Fiekas’s picture

@claudia.cristea: Yes, I'd really like a unified color widget in core. But you're right. Probably we shouldn't block this issue on that. We can clean up that later.

Tests failing now?

claudiu.cristea’s picture

Tests failed because were not adapted after rotation color validation changes. Fixed now.

claudiu.cristea’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal8.crop-background-204497-109.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
18.59 KB

Fixed tests...

dman’s picture

Status: Needs review » Needs work

I can confirm that patch #112 applies well and fixes the (original) described problem.
The UI for color selection works fine (though is not fancy). Includes Alpha, seems OK.
Validation seems to work.

I find it *not perfect* that is allows either '#ffffff' or 'ffffff' as values.
I think it SHOULD *allow* either input, so that's good. But I'd prefer it for consistency if it either always prepended or always truncated the # when saving. It leaves the "what is the correct syntax" question open to interpretation.
For example, someones screenshot may differ from someone elses settings (one with #, one without) and that can look like a source of error. When it's not really.

I suggest that the validation hook should correct and rewrite the input early on. Also probably putting the hex to uppercase ... to reduce inconsistencies in the UI. I don't want this to be getting into the color widget feature request question again, but I think we can do this right here and now. To me this is a validation issue for UX.

I wanted to flag this 'reviewed' ... but now I'm wondering if in light of this UI inconsistency, perhaps I've gotta 'needs work' on it. Almost there. I'll try a re-roll with my suggestion

dman’s picture

Status: Needs work » Needs review
FileSize
18.04 KB

I'm just putting this here as a suggestion. If nobody likes the idea, then call #112 'reviewed'

This patch (a tiny adjustment to #112) means that the color values will be tidied consistently if you enter :
"#123456" => "123456"
"#abcdef" => "ABCDEF"
"abcdef" => "ABCDEF"

Personally, I'd prefer to ALWAYS have # in the value, but the UI currently places a # outside the form element. And that looked odd.

If we were to make this improvement, I'd maybe think about instead running
image_rgba2hex(image_hex2rgba($value));
as that sort of approach always has the side effect of fixing inconsistent formats like this.
But for now, it's just string fixes.

claudiu.cristea’s picture

Status: Needs review » Needs work

While this patch will be backported to Drupal 7, my suggestion is to keep the existing color storage format used now for rotate effect. In this way existing sites will not need any upgrade path - no hook_update_N(). So, you're right, we need store the value prefixed with '#'.

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
19.95 KB

This patch is fixing the input and backend for the color hex string as follows:

  1. The user may enter '#' as color prefix. The #field_prefix has been removed.
  2. Alphabetic characters may be entered as lower or upper case.
  3. The argument of image_hex2rgba(&$hex) is passed now by reference and the function is tidying up the $hex string resulting a consolidated value, prefixed by '#' and with uppercase chars. Examples:
    • #fa89aE => #FA89AE
    • FA89AE => #FA89AE
    • fa89aE => #FA89AE
  4. The color is stored prefixed by '#' and with uppercase chars. This assures compatibility with the D7 existing image_rotate color storage format and will make no need for an update path on backport.
Niklas Fiekas’s picture

Status: Needs review » Needs work

Ok. Since we're down to nitpicking:

+++ b/core/includes/image.incundefined
@@ -292,10 +292,9 @@ function image_resize(stdClass $image, $width, $height) {
+ *   uncovered area of the image after the rotation. Examples: "#RGB", "#RGBA",
+ *   "#RRGGBB", "#RRGGBBAA".  For images that support transparency, this will
+ *   default to transparent. Otherwise it will be white.

One might argue, that RGB isn't really an "example".

+++ b/core/includes/image.incundefined
@@ -320,6 +319,11 @@ function image_rotate(stdClass $image, $degrees, $background = NULL) {
+ *   Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA". For images that support

"Examples".

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+ * Converts a hex string to RGBA (Red, Green, Blue, Alpha) integer values and

Capitalize the colors?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+ *   A string specifing an RGB color in the formats: '#ABC', '#ABC', '#ABCD',
+ *   '#ABCD','#AABBCC','#AABBCC','#AABBCCDD','#AABBCCDD'. The color is passed

Do we have some formats twice here? Maybe RGBA instead of ABCD?

Missing spaces after commas: , .

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+ * @return
+ *   An associative array having 'red', 'green', 'blue' and 'alpha' as keys and
+ *   corresponding color decimal integer as values.

Document FALSE, too?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+function image_hex2rgba(&$hex) {
+
+  // Assure consistency for the color string.

Remove the empty line.

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+  if (preg_match('/^#[0-9A-F]{3}$/', $hex)) {

Maybe loop through the patterns and actually capture something in the regexes?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+  // Convert colors to decimal integers.
+  $red = hexdec($red);
+  $green = hexdec($green);
+  $blue = hexdec($blue);
+  $alpha = hexdec($alpha);
+  return array('red' => $red, 'green' => $green, 'blue' => $blue, 'alpha' => $alpha);

Maybe(!):

return array(
  'red' => hexdec($red),
  ...
+++ b/core/modules/image/image.admin.incundefined
@@ -587,6 +592,15 @@ function image_crop_form($data) {
+    '#description' => t('Hex string specifying the background color to use when cropping the image to dimensions that are exceeding the source dimensions. Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA". Leave blank for transparency on image types that support it.'),

"Examples".

+++ b/core/modules/image/image.testundefined
@@ -307,13 +307,13 @@ class ImageEffectsUnitTest extends ImageToolkitTestCase {
+    $this->assertTrue(image_rotate_effect($this->image, array('degrees' => 90, 'bgcolor' => '#FFF')), t('Function returned the expected value.'));

While we're touching it: Don't translate assertion messages.

+++ b/core/modules/image/image.testundefined
@@ -307,13 +307,13 @@ class ImageEffectsUnitTest extends ImageToolkitTestCase {
+    $this->assertEqual($calls['rotate'][0][2], '#FFF', t('Background color was passed correctly'));

Again, no t(), when we touch it anyway.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    $this->assertTrue(is_file($this->image->source), t('The source image (50x25) filled with blue (#0000FF) has been created: %url.', array('%url' => $this->image->source)));

No t().

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    $style1 = image_style_load(NULL, $this->isid1);
+    $style1['description'] = t('Cropping center-to-center from 50x20 to 50x50 with red (#FF0000) color on additional area.');
+    $style1['expected_color'] = t('red (#FF0000)');
+    $style1['expected_color_map'] = array('red' => 255, 'green' => 0, 'blue' => 0, 'alpha' => 0);
+    $style2 = image_style_load(NULL, $this->isid2);
+    $style2['description'] = t('Cropping center-to-center from 50x20 to 50x50 with no color specified for additional area.');
+    $style2['expected_color'] = t('transparent');
+    $style2['expected_color_map'] = array('red' => 0, 'green' => 0, 'blue' => 0, 'alpha' => 127);

No t() just for test cases.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+      $this->assert(TRUE, t('Checking a pixel color from the new area, at (5, 5) in "%file".<br />Expecting %color.', $color + array('%file' => $uri)));

t(). <br /> in the message?

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+      $this->assertIdentical($colors, $style['expected_color_map'], t('Pixel from (5, 5) is %color', $color));

t().

+++ b/core/modules/system/image.gd.incundefined
@@ -105,10 +105,10 @@ function image_gd_resize(stdClass $image, $width, $height) {
+ *   uncovered area of the image after the rotation. Examples: "#RGB", "#RGBA",

"Examples".

+++ b/core/modules/system/image.gd.incundefined
@@ -105,10 +105,10 @@ function image_gd_resize(stdClass $image, $width, $height) {
+ *   default to transparent. Otherwise it will be white.
+ *   ¶
  * @return

Trailing space.

 

What are the arguments for using strings to store color values instead of integers like 0x11223300?

claudiu.cristea’s picture

+++ b/core/includes/image.incundefined
@@ -292,10 +292,9 @@ function image_resize(stdClass $image, $width, $height) {
+ *   uncovered area of the image after the rotation. Examples: "#RGB", "#RGBA",
+ *   "#RRGGBB", "#RRGGBBAA".  For images that support transparency, this will
+ *   default to transparent. Otherwise it will be white.

One might argue, that RGB isn't really an "example".

It's not really an example except the place and number of chars for each format. I would let it in that form.

+++ b/core/includes/image.incundefined
@@ -320,6 +319,11 @@ function image_rotate(stdClass $image, $degrees, $background = NULL) {
+ *   Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA". For images that support

"Examples".

Don't understand. You suggest to remove the colon (:)? Or?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+ * Converts a hex string to RGBA (Red, Green, Blue, Alpha) integer values and

Capitalize the colors?

OK. Changed to lowercase.

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+ *   A string specifing an RGB color in the formats: '#ABC', '#ABC', '#ABCD',
+ *   '#ABCD','#AABBCC','#AABBCC','#AABBCCDD','#AABBCCDD'. The color is passed

Do we have some formats twice here? Maybe RGBA instead of ABCD?

Missing spaces after commas: , .

OK. Changed to "RGB" format and correct the missing spaces.

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+ * @return
+ *   An associative array having 'red', 'green', 'blue' and 'alpha' as keys and
+ *   corresponding color decimal integer as values.

Document FALSE, too?

OK.

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+function image_hex2rgba(&$hex) {
+
+  // Assure consistency for the color string.

Remove the empty line.

OK.

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+  if (preg_match('/^#[0-9A-F]{3}$/', $hex)) {

Maybe loop through the patterns and actually capture something in the regexes?

I thought also to validate and extract all colors in a single regexp step. Do you know some magic regexp snippet?

Maybe(!):

return array(
  'red' => hexdec($red),
  ...

OK.

+++ b/core/modules/image/image.testundefined
@@ -307,13 +307,13 @@ class ImageEffectsUnitTest extends ImageToolkitTestCase {
+    $this->assertTrue(image_rotate_effect($this->image, array('degrees' => 90, 'bgcolor' => '#FFF')), t('Function returned the expected value.'));

While we're touching it: Don't translate assertion messages.

Why? I see translations in all assertion in the .test file. There's no explanation in docs http://api.drupal.org/api/drupal/core!modules!simpletest!drupal_web_test.... I saw http://drupal.org/node/500866 but as per http://drupal.org/node/500866#comment-5564970 it's not very clear what and why should not be translated. If we go without t() we should consider cleaning up all the .test file.

+++ b/core/modules/system/image.gd.incundefined
@@ -105,10 +105,10 @@ function image_gd_resize(stdClass $image, $width, $height) {
+ *   default to transparent. Otherwise it will be white.
+ *   ¶
  * @return

Trailing space.

OK.

What are the arguments for using strings to store color values instead of integers like 0x11223300?

Read past comments. In Drupal 7, for image_rotate, colors are stored in hex string formatt (#RRGGBB). While this patch will be backported to D7 we don't want to add an upgrade path (hook_update_N() to convert existing stored colors from string to integers.

Niklas Fiekas’s picture

Thank you.

Don't understand. You suggest to remove the colon (:)? Or?

No, I guess that's fine. Just arguing against the word "Example" there, too. But - why not - leave it as it is.

One more:

+++ b/core/modules/image/image.admin.incundefined
@@ -475,10 +475,15 @@ function image_effect_integer_validate($element, &$form_state) {
+      form_error($element, t('!name must be a hexadecimal color value. Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA".', array('!name' => $element['#title'])));

We shouldn't use !name but either @name or %name here.

If we go without t() we should consider cleaning up all the .test file.

Yep, that's the plan. (Obviously not in this issue). I encountered the t() thing, too. "But it's all around here!". sun and xjm clarified it in IRC: Yes, don't translate stuff just for test cases.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
19.56 KB

Compacted the image_hex2rgba() function but still cannot process in a single regexp step.

claudiu.cristea’s picture

Status: Needs review » Needs work

Forgot to test locally.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
19.62 KB

Fixed test errors.

claudiu.cristea’s picture

Status: Needs review » Needs work
+++ b/core/includes/image.inc
@@ -292,10 +292,9 @@ function image_resize(stdClass $image, $width, $height) {
  *   An hexadecimal integer specifying the background color to use for the

This should be "An hexadecimal string", not integer. Fixing...

18 days to next Drupal core point release.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
19.71 KB

Final patch...

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

Niklas Fiekas’s picture

Status: Needs review » Needs work
+++ b/core/includes/image.incundefined
@@ -432,5 +436,45 @@ function image_save(stdClass $image, $destination = NULL) {
+ * Converts a hex string to RGBA (red, green, blue, alpha) integer values and
+ * tidy up the input hex color string.

Converts and tidies?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,45 @@ function image_save(stdClass $image, $destination = NULL) {
+ *   by reference in order to return a tidy up color string for consistency.

tidied up color string?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,45 @@ function image_save(stdClass $image, $destination = NULL) {
+    $hex_color = preg_replace('|([0-9A-F])|', "\\1\\1", $hex_color);

Or '\1\1' having a single quoted string.
Btw. I like this step of normalizing. It's probably clearer than any magic long regex doing everything at once.

+++ b/core/modules/image/image.admin.incundefined
@@ -475,10 +475,15 @@ function image_effect_integer_validate($element, &$form_state) {
+    if ($hex != $element['#value']) {

Do we need to have this $hex variable? Couldn't we just do form_set_value() regardless of what happened?

+++ b/core/modules/image/image.effects.incundefined
@@ -258,26 +259,14 @@ function image_rotate_effect(&$image, $data) {
+  $background_color = empty($data['bgcolor']) ? NULL : $data['bgcolor'];

I notice we have $data['bgcolor'] here and $data['background_color'] above.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+class ImageCropBackgroundTestCase extends DrupalWebTestCase {

Maybe use protected $profile = 'testing'; and parent:setUp(array('required module 1', ...)); just the required modules?

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+
+    // Invoke parent method.
+    parent::setUp();

Superflous empty line and comment.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    // Source image width & height.

I'd go with and.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    // Create 2 dummy styles.

two, maybe.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+      // Check if the Image Style has been created.

Image Style?

 

Edit: Some more:

  • When I save #FFF the next time I load #FFF is still shown? Shouldn't that have been normalized to #FFFFFF?
  • When having #FF0000FF the border is black, whereas #FF000055 is reddish-transparent. Is that expected?
claudiu.cristea’s picture

+++ b/core/includes/image.incundefined
@@ -432,5 +436,45 @@ function image_save(stdClass $image, $destination = NULL) {
+ * Converts a hex string to RGBA (red, green, blue, alpha) integer values and
+ * tidy up the input hex color string.

Converts and tidies?

OK.

+++ b/core/includes/image.incundefined
@@ -432,5 +436,45 @@ function image_save(stdClass $image, $destination = NULL) {
+ *   by reference in order to return a tidy up color string for consistency.

tidied up color string?

OK.

+++ b/core/includes/image.incundefined
@@ -432,5 +436,45 @@ function image_save(stdClass $image, $destination = NULL) {
+    $hex_color = preg_replace('|([0-9A-F])|', "\\1\\1", $hex_color);

Or '\1\1' having a single quoted string.

OK.

+++ b/core/modules/image/image.admin.incundefined
@@ -475,10 +475,15 @@ function image_effect_integer_validate($element, &$form_state) {
+    if ($hex != $element['#value']) {

Do we need to have this $hex variable? Couldn't we just do form_set_value() regardless of what happened

Well. It is faster when the color is correct entered. I suggest to leave it in this way... it gives a good message that $hex was tidied up.

+++ b/core/modules/image/image.effects.incundefined
@@ -258,26 +259,14 @@ function image_rotate_effect(&$image, $data) {
+  $background_color = empty($data['bgcolor']) ? NULL : $data['bgcolor'];

I notice we have $data['bgcolor'] here and $data['background_color'] above.

Yes. I saw that but I decided to left it as 'bcolor'. That was there at image_rotate. What you think, is a change (to 'background_color') that can impact other modules?

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+class ImageCropBackgroundTestCase extends DrupalWebTestCase {

Maybe use protected $profile = 'testing'; and parent:setUp(array('required module 1', ...)); just the required modules?

I don't get it :) Please explain.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+
+    // Invoke parent method.
+    parent::setUp();

Superflous empty line and comment.

OK.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    // Source image width & height.

I'd go with and.

OK.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    // Create 2 dummy styles.

two, maybe.

OK.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+      // Check if the Image Style has been created.

Image Style?

Yes. If "Style" creation failed in setUp() it cannot be loaded. So we check if was successfully loaded and ready to be applied.

When I save #FFF the next time I load #FFF is still shown? Shouldn't that have been normalized to #FFFFFF?

Well this needs a discussion. If user enters #F00 he will expect to get the same. So, i would not normalize that in storage. It's his choice.

When having #FF0000FF the border is black, whereas #FF000055 is reddish-transparent. Is that expected?

That I was checking after submitting the patch. Alpha is in range 0 => 127 (7F). So I will limit to 7F.

Thanks.

Niklas Fiekas’s picture

Yes. I saw that but I decided to left it as 'bcolor'. That was there at image_rotate. What you think, is a change (to 'background_color') that can impact other modules?

Right, I think we shouldn't change that here. Maybe the other way around, background_color to bgcolor, to have it consistent? Or we just leave that as it is in the patch.

I don't get it :) Please explain.

Admittedly that was too much for too few words :D
So ... DrupalWebTestCase is going to do a Drupal installation. The default installation profile has lot's of stuff we aren't going to need, which makes tests runs longer. So the suggestion is to select the testing profile by setting

  protected $profile = 'testing';

at the top of the class. That also means some of the modules we do need are not going to be enabled. Those will have to be passed as an array to parent::setUp().

Well. It is faster when the color is correct entered. I suggest to leave it in this way... it gives a good message that $hex was tidied up.

Agreed.

Yes. If "Style" creation failed in setUp() it cannot be loaded. So we check if was successfully loaded and ready to be applied.

Ok ... the bold first letters we're a little hard to see. I wanted to know why the first letters of "Image Style" were capital.

Well this needs a discussion. If user enters #F00 we will expect to get the same. So, i would not normalize that in storage. It's his choice.

Ok. I don't feel it should be one way or another - just to bring it up. Well ... one could indeed argue it should stay as it is.

claudiu.cristea’s picture

About...

  • 'bgcolor'. We need to keep it for D7 backport in order to avoid an update path. That array key is seralized and stored in backlend. I would keep also the new 'background_color' as it's a better spelling. Maybe we will unify it in the future...
  • Test profile. Makes sense... It seems that Image module is enabled by default. Checking...
Niklas Fiekas’s picture

'bgcolor'. We need to keep it for D7 backport in order to avoid an update path. That array key is seralized and stored in backlend. I would keep also the new 'background_color' as it's a better spelling. Maybe we will unify it in the future...

I see. Fair enough.

testing profile will have none but the absolutely required core modules enabled by default.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
19.73 KB

Latest updates...

claudiu.cristea’s picture

ImageMagick toolkit implemented in ImageAPI needs a patch after backporting this to D7. Opened e placeholder issue there: #1437036: API change for image_crop effect.

Niklas Fiekas’s picture

Status: Needs review » Needs work

That I was checking after submitting the patch. Alpha is in range 0 => 127 (7F). So I will limit to 7F.

Yay, works now :) The only remaining problem now is this: Imagine a user enters #F0FE. The error message is: Background color must be a hexadecimal color value. Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA". How can I guess what's wrong, from that?

 

Other places in core seam to use @name instead of %name. (For example width and height of the crop effect.)

 

+++ b/core/includes/image.incundefined
@@ -432,5 +436,44 @@ function image_save(stdClass $image, $destination = NULL) {
+  // Save a uppercase version without leading "#" for later processing.

an uppercase version.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    // Invoke parent method.

I think this states the obvious.

 

Now some stuff you could ignore, as well:

+++ b/core/includes/image.incundefined
@@ -320,6 +319,11 @@ function image_rotate(stdClass $image, $degrees, $background = NULL) {
+ *   Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA". For images that support

When you're rerolling it anyway maybe standardize on single quotes as below?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,44 @@ function image_save(stdClass $image, $destination = NULL) {
+ *   'RGBA', '#RRGGBB', 'RRGGBB', '#RRGGBBAA', 'RRGGBBAA'. The color is passed

(For reference: Single quotes used here.)

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    // Attach a single "image_crop" effect to the first style that enlarges the

Single quotes?

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    // Attach a single "image_crop" effect to the second style that enlarges the

Single quotes?

+++ b/core/modules/system/image.gd.incundefined
@@ -179,15 +177,32 @@ function image_gd_rotate(stdClass $image, $degrees, $background = NULL) {
+ *   Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA". For images that support

Single quotes?

gabrielu’s picture

I fixed my problem using Canvas from http://drupal.org/project/imagecache_actions

There are several ways to approach this issue, and this seamed the simplest at the moment.

Gabriel

claudiu.cristea’s picture

The only remaining problem now is this: Imagine a user enters #F0FE. The error message is: Background color must be a hexadecimal color value. Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA". How can I guess what's wrong, from that?

That I wouldn't change the validation function because it mean to explain how a hex color string must be composed. #HF0 may be another error that must be explained, no? In prefer to enrich a little bit the error message - maybe with a link to some Wikipedia definition.

What you think?

claudiu.cristea’s picture

Error message proposal:

Crop background color must be a hexadecimal string color value taking one of the next possible formats: '#RRGGBB', '#RRGGBBAA', '#RGB', '#RGBA', where:

  • 'RR' (shorthand 'R'): Red component. Range: '00' to 'FF'.
  • 'GG' (shorthand 'G'): Green component. Range: '00' to 'FF'.
  • 'BB' (shorthand 'B'): Blue component. Range: '00' to 'FF'.
  • 'AA' (shorthand 'A'): Optional alpha transparency. Range: '00' to '7F'. Caution on using the shorthand version, you cannot enter a value greater than '7'!
xjm’s picture

I really don't think we should support so many options for validation. It actually makes for worse UX. I'd suggest only supporting what the HTML 5 color element does (since we will want to convert the D8 patch here once #1445224: Add new HTML5 FAPI element: color is in). That is, only support longhand values--and make alpha a separate field. The hex range for alpha is just weird. We can standardize case name, etc. silently.

dinis1’s picture

Can you atach all you changed file, or send file to email modestas@irankiubaze.com
Thank you

claudiu.cristea’s picture

What's new:

  • This is a BUG, not a feature request.
  • The patch reflects @xjm comment in #136.
  • Reworked the patch to use the new HTML5 color form element.
  • The HTML5 color picker (with textfield fallback) doesn't allow to pass an empty value to the color. So, I had to add an additional checkbox.
  • The image_rotate effect was changed too in order to provide an unified way to pass the "new areas" color.
  • New API changes are backward compatible, so they don't break D8 API freeze.

Didn't provide an interdiff.txt as is not relevant due to the big number of changes.

Category: feature » bug

The last submitted patch, drupal-image-crop-204497-138.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
27.91 KB

Removed trailing spaces.

claudiu.cristea’s picture

Reworked on top of last changes to image.

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Updated issue summary.

claudiu.cristea’s picture

Issue summary: View changes

Summary changed.

tim.plunkett’s picture

I don't understand why this is a bug, sounds like a feature request.

Also this should wait on #2033669: Image file objects should be classed

claudiu.cristea’s picture

This issue hangs from the good times of Image Cache module and was moved to core starting with D6. It's a bug because it renders in black the new areas but you don't expect that. Rotate and crop are the only core effects that may generate new areas. On crop there's no way to control the color or transparency (when applicable) of the new areas and you'll get unexpected black new areas. Rotate it's not buggy from this point of view. Basically all we want to do here is a simple fix to crop that is always present in rotate (but in other way).

I agree that this should go after #2033669: Image file objects should be classed. I'm following that issue as I'm following all related to image & stuff.

fietserwin’s picture

#141: drupal-image-crop-204497-141.patch queued for re-testing.

Looking at the timestamps, the last test took place just before #2033669: Image file objects should be classed was committed.

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +Needs backport to D7

The last submitted patch, drupal-image-crop-204497-141.patch, failed testing.

claudiu.cristea’s picture

Hey @fietserwin, the patch doesn't apply. I'm already working on reroll but also on much more better approach.

claudiu.cristea’s picture

@fietserwin, #2063373: Cannot save image created from scratch is blocking this. Can you help reviewing that? Thanks!

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

Anybody’s picture

The problem still exists in D6 and D7. How can we proceed here?

thirdender’s picture

@Anybody, see the ImageCache Actions module. Install and enable the "Imagecache Canvas Actions" module. It provides a "Define canvas" image style action which will let you set the background color and image dimensions.

mgifford’s picture

Version: 8.0.x-dev » 8.1.x-dev
Assigned: claudiu.cristea » Unassigned

This still a concern in D8? Unassigned issue too.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tregismoreira’s picture

There's a contrib module for that: https://www.drupal.org/project/image_effects

awasson’s picture

+1 for image effects. It seems to provide the ability on a per image style basis.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

pameeela’s picture

Title: Configurable background color when cropping to larger dimensions » Make the background fill color configurable where image styles result in blank space
Category: Bug report » Feature request
Issue summary: View changes
Issue tags: +Bug Smash Initiative

Tempted to close since this functionality is provided in contrib by Image Effects module. But I'm updating to a feature request since it is something that could be added to core.

I don't feel strongly that it should be added to core, so will let others decide. I don't think I ever use the crop-only effect, but if I did I probably would try to set a minimum size for the image to avoid having empty space anyway.

Edit: I also updated the issue summary to remove the outdated code references and simplified the description. It definitely still happens in D9.

Anybody’s picture

Thank you, @pameeela, well I think the functionality simply isn't complete without that option as filling with black (current / default behavior) is not an option for most real world scenarios. I'd still tend to say it can be rated as bug, but let's see what the others say. So the whole functionality doesn't really make sense if this is missing. It shouldn't need an extra module for that.
My 2 cent ;)

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.