Dragging the crop selection to the "Hard Limit" size results in an error when node is saved

Yes, We have an subject here !!

That case need to be specified more because I think that is an case a bit complex. Why ? Because we can solve that easily but with 'N' approach and only one are the BEST.

Differents solution :

1 : Display another Error message to inform user that he upload an image smaller than specified limit in settings.
2 : Add a test in ImageCrop::validateHardLimit() to ensure we don't display these message if image are smaller than setting.
3 : Block upload of Picture smaller than Limit.
4 : Display an warning (not blocker of validate element) to inform user that he upload an image smaller than specified limit in settings and he understand possible distorsion of images avoided by these limite set by Admin.

IMHO the fourth solution are better, inform users, not a blocker of validation and permit user to have more flexibility.

What you think about ?

We need to speak more during Media Sprints DDD Sevilla about this subject.

Fixing event tag

Hi @meichr,

I need you help on that issue during a big refactor/upcrease of Js integration of module we have solve few problem of that kind. Can we confirm me if current dev branche work better or solve that issue ? Refactoring issue => #2803407: Refactor form element and JS to work with any theme

I have tested all comportements describe previously. All seems good now, I attach a screenshot with a test with my cropType configured to have hard limits to 500 (height & width). When I crop my image with crop maximum close to 500 (501 in my case). I save and all works but if I crop an zone (one of twos zones behind 500 a message are displayed).

Feel free to re-open with more precise details and (2.x version) installed.

I found same bug testing latest version (2.0) on simplytest.me

Hii @minnoce thank you for your feedback. Can you describe more the reproduction path of your test ? That can be usefulll to found a solution. Thanks :)

There seems to be a bug with the module.

For me it works sometimes, but in other cases breaks. I'm not sure, but it might be when you use a very large image (over 2000px for example). Could it be something wrong with the calculations when using the cropper on a large resolution image but the preview is much smaller?

Here is the image I used:

Here you can see I have cropped it to the hard limit:

And finally, you can see when I debug the code that despite the hard limit being 1200x1200, it has allowed it to be 747x747. No crop is saved as a result.

Hii @Rob230

I have tested again with fresh install all functionalities and all seem's good except an error margin of 3 pixels before stop the crop zone.

I attach a video to show you mes usecases.

https://youtu.be/jyU1LGnhdAY

I have more informations about the possible error margin that's is due to ratio vs Hard limit ratio by example if you define a real 16:9 like : (Width : 1024 and Height : 576 ) you can't have a crop zone lower than 1024x576 precisely but if you define hard limits like (576x576) the crop zone stop when width are in 576 and height can be lower than 576px but validation stop submission after because you don't respect hard limits.

Specifically you case with hard limit seem's to be a cache problem I guess :/ I need more information about that.

But in your video it fails because it's 3px smaller. This kind of failure is happening to me all the time with large crop sizes and large images. Surely something is not right with it.

I have finnaly identify what's the problem !! all of our problems is due to

  /**
   * Sets a single crop value.
   *
   * @param {'applied'|'height'|'width'|'x'|'y'} name
   *   The name of the crop value to set.
   * @param {Number} value
   *   The value to set.
   * @param {Number} [delta]
   *   A delta to modify the value with.
   */
  Drupal.ImageWidgetCropType.prototype.setValue = function (name, value, delta) {
    if (!this.values.hasOwnProperty(name) || !this.values[name][0]) {
      return;
    }
    value = value ? parseInt(value, 10) : 0;
    if (delta && name !== 'applied') {
      value = Math.round(value * delta);
    }
    this.values[name][0].value = value;
    this.values[name].trigger('change.iwc, input.iwc');
  };

And specially ==> value = value ? parseInt(value, 10) : 0; When value arrived by recover cropper values we have a string "1200" and when parseInt("1200", 10) recover integer we have (int) 1997 ... All of next calculations are based on 1997 and not 1200...

We have two approach possible understand why parseInt() not works as excepted (My skill are limited in JS) or understand why we need to use parseInt() and not using this if not necessary ?! I add marcarver to have more specialist opinion about this.

With following debug :

  /**
   * Sets a single crop value.
   *
   * @param {'applied'|'height'|'width'|'x'|'y'} name
   *   The name of the crop value to set.
   * @param {Number} value
   *   The value to set.
   * @param {Number} [delta]
   *   A delta to modify the value with.
   */
  Drupal.ImageWidgetCropType.prototype.setValue = function (name, value, delta) {
    if (!this.values.hasOwnProperty(name) || !this.values[name][0]) {
      return;
    }
    console.log('before : ' + value);
    value = value ? parseInt(value, 10) : 0;
      console.log('after : ' + value);
      if (delta && name !== 'applied') {
      value = value * delta;
          console.log('before Math : ' + value);
          value = Math.round(value);
          console.log('after Math : ' + value);

      }
    this.values[name][0].value = value;
    this.values[name].trigger('change.iwc, input.iwc');
  };

Result :

(X value)
before : 0
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:803 after : 0
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:806 before Math : 0
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:808 after Math : 0

(Y value)
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:801 before : 12
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:803 after : 12
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:806 before Math : 76.80000000000001
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:808 after Math : 77

(Width value)
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:801 before : 187.5
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:803 after : 187
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:806 before Math : 1196.8
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:808 after Math : 1197

(Height value)
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:801 before : 105.46875
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:803 after : 105
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:806 before Math : 672
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:808 after Math : 672

(Is Applied value)
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:801 before : 1
js_XrKVvRZzXs5i6HkiFeBJOIDdStZa9pk8QmGKYImCmvc.js:803 after : 1

we can see parseInt() convert string with float to interger that convert introduce ~3px errors.

The problem isn't parseInt, that behaves as expected.

The reason the value is changing is due to:

    if (delta && name !== 'applied') {
      value = Math.round(value * delta);
    }

Just did some quick math and it seems the delta is roughly .9975 (1200 * .9975 = 1197).

IIRC, delta has something to do with the ratio of the original image dimensions vs. the real container size (e.g. picture is bigger than the space allotted for the screen/container).

I cannot attest to what that actually means, however. I know you tagged me on this issue, but I never really fully understood the full math behind this module and how exactly delta was properly calculated (think it's provided by the cropper).

Regardless, I only rearranged the existing code into a better JS structure. I didn't alter the logic behind the existing code.

Regardless, I only rearranged the existing code into a better JS structure. I didn't alter the logic behind the existing code.

Yes, and again Thank you for that !! I add you to this issue because I'm really disappointed by this errors and I have added more precisions about parseInt() that seems convers string 187.5 to 187 systematically and with that we can explain the problem EG

with correct values : with : 187.5 * 6.4 = 1200 (Ok !)
with parseInt() value : 187 * 6.4 = 1996.8 (and Math convert this to 1997)

This patch does solve the problems. Anyone can review that to tell me if we have better way to do that ?

Only available for 2.x branch.

Hm. I guess there could be floats. No need to check for either or, you can just use parseFloat instead.

Also, the above patch unnecessarily increased the indentation of the method block.

You right @markcarver :) ! Changes applied Thanks

+++ b/js/ImageWidgetCropType.js
@@ -647,12 +647,15 @@
-      return;
+        return;
...
+
...
+
...
-      value = Math.round(value * delta);
+        value = Math.round(value * delta);

Still some unnecessary whitespace additions and indentations.

This should only be a one-line two line patch.

Edit: forgot the version bump.

Oops ! Sorry markcarver I change that now that's good.

LGTM

Unfortunately that patch has not fixed it for me. With the 1200x1200 crop it still fails.

I think you fixed the 3px problem, but not the problem where it's the wrong size by hundreds of pixels.

Hii @Rob230 Thank you for tests :)

The validation fails are normal because cropper block the crop zone only when width are smaller than your fixed hard limit.

By example : If you set 1200 x 1200 for Hard limits and you use a crop with 16:9 ratio. In that case cropper block crop zone only when width are in 1200 but you should have a validation warning because in 16:9 aspect ratio your crop area are (1200x~675) the height 675 not pass the validation and that's is normal and excepted.

If you define a crop limits not respect your aspect ratio cropper choose to fixed width only and not height. This is why I consider this issue as fixed because before patch we can have a width smaller than hard crop limits and that's is bad because we can have width AND height not valid.

If you define a crop ratio 1:1 and try hard limits with 1200 x 1200 all works to you.

This respond to your problematic ?

I agree that you have fixed the original problem reported, but I am still having a similar but different problem. I don't know why.

Here you can see the crop type:

Here you can see the image style using the crop type:

Here is me cropping the image I gave before (which is 2560x1600):

It is letting me choose an area which looks much smaller than 1200x1200.

And then it doesn't save the crop, because the crop size is smaller than 1200x1200. I have gone over these settings again and again. I'm not sure if there is some mistake, but I don't see one.

@Rob230 Ok for original problem solved by patch ;) Thank for your feedback !

Specially for your problem I think this is a missConfiguration problem. I just tried to reproduce and configure all in video to show you my trying to reproduce your issue.

In my test I have my Image style using 1:1 crop Type with aspect ratio 1:1 and hard limit set as 1200 x 1200.

I configure a basic Node 'article' using image field provided by standard edition.

I go add an Article to my new site and upload an image.

My crop 1:1 appear and I select the minimum crop zone as possible 1200 x 1200 I save and all works as excepted.

@see my tests https://youtu.be/U6RDBn4R9Z0

Your preview crop thumbnail it's module provided style or you have customized it ? Can you show me all your configuration of Image Style / Crop type / field Widget ?

I think it is misconfiguration (and an entirely separate issue) on @Rob230's part.

The patch in #26 is good to go as it fixes the OP.

Merged :) Thanks all ! Special thank @markcarver for your following :)

Small fix for comment#3 error

patch for version 8.x-2.1
previous one was for current 8.x-2.x