Default crop does not apply

Hello @miro,

I think this is not responsibility of IWC. When the user decides not to apply to particular crop it is his responsibility to provide an effect like "scale" in order not to display a distorted image. In the case of style pictures for "Simple example harrows" we define a size height / fixed width for example, but if we decide not to distort the image so just change the configuration and imageStyle put a scale on the width for example. In this case, if we do not have crop defined then the image will not be distorted.

See examples :
Not define any crop

The image is distorded by scale configuration of ImageStyle

I go change the configuration of ImageStyle to set only width to 320 (homotectic rezise)

My same image is not distorded and any crop is applied

If i decide to set a crop i my image is okay too

I did some research and found that we can apply default crop in two ways:

1. Just send crop values to hidden inputs and message "Cropping applied” remains because crop is always applied, or
2. Send crop values to hidden inputs and remove “Cropping applied” which complicates code a lot in ImageCropWidget::process() (which is already complicated enough), because that value we use to determine crop in various cases.

But IMO if the crop is reset or didn’t set custom on initializing (and message "Cropping applied" doesn't show) it's logical that any crop shouldn't be applied, and therefore I fully agree with @woprrr and I wouldn’t add default crop applying at all.

Work in progress.

Workaround for this is to have manual crop followed by resize and crop in image style.

Still work in progress.

Yesterday I discussed this with @miro_dietiker. We came to the following conclusion.

There are two viable approaches here:
1. Don't apply crop by default at all. This also means crop box should not be displayed until user has selected something.
2. Treat default crop as normal crop (display crop box, display "Crop applied" in vertical tabs). In this case we have to make sure crop is saved correctly even if user didn't modify it at all.

Both approaches make sense in some cases, which means we need to make this configurable on the widget level. It needs to be configurable on a crop type basis as one might want to use one approach for one and other approach for the other. Example: option 1 being used on crop types that don't specify aspect ratio limits and option 2 on the ones that do.

I think we should save crop anyway. From image effect's standpoint it is the crop entity which stores all information needed to create an image derivative. If we end up not saving default crop that isn't true any more. It will most likely need to load crop type and field configurations as well. This causes tight coupling of crop UI (IWC in this case) and image effects. It also makes it much harder to replace UI component. Without that we steer away from the idea of the architecture of independent components.

Assuming UI component simply displays how default crop looks is also strange in my opinion. That basically means that field widget knows how image style configuration looks like. In general this isn't an assumption we can make. What happens when/if image style configuration changes? What do we display if more then one image style use the same crop type?

Selected crop area vs. actual image style image are not related 1 to 1. Crop just provides some information for image effect. It is up to image effect to decide how to use that information. I've seen cases in the past where we had two image styles with different aspect rations using same crop type. Crop type was 4:3 and the two styles were 4:3 and 16:9. First style actually ended up looking pretty much the same as crop itself. The other style didn't, but the crop still provided enough information for style to create an image that made sense.

Here is the patch, from @sanja_m and me, based on the idea from #8 but for field config, not on a crop type basis.

Fixing failing test.

Looks very good. We still need to decide on #8, but here are my comment about current patch.

1. +++ b/image_widget_crop.module
   @@ -407,6 +407,12 @@ function image_widget_crop_form_file_form_alter(&$form, \Drupal\Core\Form\FormSt
   +        $container[$crop_type_id][$element_wrapper_name]['values']['crop_default'] = [
   +          '#type' => 'hidden',
   +          '#attributes' => ['class' => ["crop-default"]],
   +          '#value' => 1,
   +        ];
   +
   

   I'd rather pass this configuration via drupalSettings.

2. +++ b/js/imageWidgetCrop.js
   @@ -186,12 +186,33 @@
   +      options.autoCrop = true;
   +    }
   +    else if (parseInt($values.find('.crop-default').val()) === 0) {
   +      options.autoCrop = false;
   

   If we do ^ we won't need this code any more.

3. +++ b/js/imageWidgetCrop.js
   @@ -186,12 +186,33 @@
   +    var $valuesDefault = $element.siblings(cropperValuesSelector);
   +    var dataDefault = $element.cropper('getData');
   +    // Calculate delta between original and thumbnail images.
   +    var deltaDefault = $element.data('original-height') / $element.prop('naturalHeight');
   

   Can we move this hunk inside if?

4. +++ b/js/imageWidgetCrop.js
   @@ -186,12 +186,33 @@
   +      $valuesDefault.find('.crop-x').val(Math.round(dataDefault.x * deltaDefault));
   +      $valuesDefault.find('.crop-y').val(Math.round(dataDefault.y * deltaDefault));
   +      $valuesDefault.find('.crop-width').val(Math.round(dataDefault.width * deltaDefault));
   +      $valuesDefault.find('.crop-height').val(Math.round(dataDefault.height * deltaDefault));
   +      $valuesDefault.find('.crop-applied').val(1);
   +      Drupal.imageWidgetCrop.updateCropSummaries($element);
   

   This code look very similar to the one in Drupal.imageWidgetCrop.reset(). Can we use helper function and get rid of redundant code?

5. +++ b/js/imageWidgetCrop.js
   @@ -186,12 +186,33 @@
   +    }
   
   +++ b/src/Plugin/Field/FieldWidget/ImageCropWidget.php
   @@ -678,8 +692,10 @@ class ImageCropWidget extends ImageWidget {
   +    $preview[] = $this->t('Apply default crop: @bool', ['@bool' => ($show_default_crop) ? 'Yes' : 'No']);
   

   Let's use same label as in the form itself. Consistency FTW.

6. +++ b/src/Plugin/Field/FieldWidget/ImageCropWidget.php
   @@ -258,6 +260,12 @@ class ImageCropWidget extends ImageWidget {
   +            $container[$crop_type_id][$element_wrapper_name]['values']['crop_default'] = [
   +              '#type' => 'hidden',
   +              '#attributes' => ['class' => ["crop-default"]],
   +              '#value' => $crop_default,
   +            ];
   +
   

   Same as above.

Fixed all bugs from #14.

Looks very good @sanja_m, i see you fixed all tasks from latest review, also i tested a little and it's works as expected.

Great job all ! Thanks @sanja_m works well ! This default comportement is verry better with that patch :).

Not so fast....

1. +++ b/config/schema/image_widget_crop.schema.yml
   @@ -24,6 +24,9 @@ field.widget.settings.image_widget_crop:
   +    show_default_crop:
   +      type: boolean
   +      label: 'Show default crop area'
   

   I mentioned in #8 that we should support configuring this on a per crop type basis.

2. +++ b/image_widget_crop.module
   @@ -407,6 +407,8 @@ function image_widget_crop_form_file_form_alter(&$form, \Drupal\Core\Form\FormSt
   +        $form['#attached']['drupalSettings']['crop_default'] = true;
   +
   

   Boolean constants should be uppercase by Drupal coding standards.

3. +++ b/js/imageWidgetCrop.js
   @@ -186,12 +189,44 @@
          };
   +      options.autoCrop = true;
   +    }
   +    else if (parseInt($values.find('.crop-default').val()) === 0) {
   +      options.autoCrop = false;
   

   Is this hunk still relevant after we started using drupalSettings? I don't think so.

Fixed 2. and 3. from #19, and for point 1. created follow up issue: https://www.drupal.org/node/2662438 as @slashrsm suggested.

Looks OK. Thanks!

Just one return about this we need to increment the version of librarie

cropper.integration:
  version: 1.3

If w not add this the js of existant instance are not updated.

Sorry to fast previous commit :s ! Now is perfect i ve increment the libraries version. Thanks for this awesome job!!!!